Skip to content

Commit

Permalink
Revert "Reland "Migrate RenderProcessHost trace events to typed protos""
Browse files Browse the repository at this point in the history 
This reverts commit 0d540e7.

Reason for revert: Still crashing: https://test-results.appspot.com/dashboards/flakiness_dashboard.html#testType=content_browsertests%20(with%20patch)&tests=All%2FSitePerProcessSSLBrowserTest.UnloadHandlersArePowerfulGrandChild

Original change's description:
> Reland "Migrate RenderProcessHost trace events to typed protos"
>
> This reverts commit 87c5394.
>
> Reason for revert: Fixed the crash
>
> The crash was due to adding trace event after the impl() was
> destroyed in the destructor. Moved the event earlier. This
> was missed in the tests because none of the tests covered 
> shutdown tracing. Added test coverage.
>
> Original change's description:
> > Revert "Migrate RenderProcessHost trace events to typed protos"
> >
> > This reverts commit feb18e2.
> >
> > Reason for revert: Dev release blocker
> >
> > Original change's description:
> > > Migrate RenderProcessHost trace events to typed protos
> > >
> > > Converts shutdown and render_host TRACE_EVENTs to typed protos, in order
> > > to ensure privacy safe trace data.
> > >
> > > Bug: b/165405144
> > > Change-Id: Ia37bdfe6195f1bdd474dc9b33b6e538e862fb87b
> > > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2812422
> > > Commit-Queue: ssid <[email protected]>
> > > Reviewed-by: Eric Seckler <[email protected]>
> > > Reviewed-by: Nasko Oskov <[email protected]>
> > > Reviewed-by: Alexander Timin <[email protected]>
> > > Cr-Commit-Position: refs/heads/master@{#883169}
> >
> > Bug: b/165405144
> > Change-Id: Ic27030af42ed11d24d5f846fc06b6d7e0736d697
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2900765
> > Reviewed-by: Nasko Oskov <[email protected]>
> > Owners-Override: Srinivas Sista <[email protected]>
> > Commit-Queue: Tommy Nyquist <[email protected]>
> > Cr-Commit-Position: refs/heads/master@{#883715}
>
> Bug: b/165405144
> Change-Id: Ibf00467a2c01a470867e2aeb2217906df93eb4fa
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2923877
> Commit-Queue: ssid <[email protected]>
> Reviewed-by: Nasko Oskov <[email protected]>
> Reviewed-by: Tommy Nyquist <[email protected]>
> Cr-Commit-Position: refs/heads/master@{#888597}

Bug: b/165405144
Change-Id: I4284060f73359a06c9dbd33f57e927c88b370ac8
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2935238
Reviewed-by: Yi Gu <[email protected]>
Reviewed-by: Xida Chen <[email protected]>
Owners-Override: Yi Gu <[email protected]>
Bot-Commit: Rubber Stamper <[email protected]>
Auto-Submit: Yi Gu <[email protected]>
Commit-Queue: Xida Chen <[email protected]>
Cr-Commit-Position: refs/heads/master@{#888863}
  • Loading branch information
@yi-gu
yi-gu authored and Chromium LUCI CQ committed Jun 3, 2021
1 parent 00aaea0 commit 990b238
Show file tree
Hide file tree
Showing 12 changed files with 76 additions and 251 deletions.
6 changes: 2 additions & 4 deletions base/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2297,13 +2297,11 @@ component("base") {
"tracing/tracing_tls.h",
]

public_deps += [
"//base/tracing/protos:chrome_track_event_zero",
"//third_party/perfetto:libperfetto",
]
public_deps += [ "//third_party/perfetto:libperfetto" ]

deps += [
"//base/tracing/protos:chrome_track_event",
"//base/tracing/protos:chrome_track_event_zero",
"//third_party/perfetto/include/perfetto/protozero",
]

Expand Down
54 changes: 2 additions & 52 deletions base/tracing/protos/chrome_track_event.proto
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,7 @@ message ChromeTaskAnnotator {
}

message ChromeBrowserContext {
reserved 1;
optional string id = 2;
optional fixed64 ptr = 1;
}

message ChromeProfileDestroyer {
Expand Down Expand Up @@ -143,50 +142,6 @@ message ResourceBundle {
optional uint32 resource_id = 1;
}

// Information about RenderProcessHost.
message RenderProcessHost {
// Unique Id to identify the RenderProcessHost. This is the browser-side,
// persistent id for this RenderProcessHost that stays constant even across OS
// layer processes managed by this RenderProcessHost.
optional uint32 id = 1;
// See ProcessLock::ToString().
optional string process_lock = 2;
// The PID of the child process.
optional int32 child_process_id = 3;

// Details about the associated browser context.
optional ChromeBrowserContext browser_context = 4;
}

message RenderProcessHostListener {
// Routing ID of the listener to the RenderProcessHost, recorded when a new ID
// is added or when an ID is removed.
optional uint32 routing_id = 1;
}

message RenderProcessHostCleanup {
// Number of IPC listeners registered to the host when Cleanup() was called.
optional uint32 listener_count = 1;
// Number of "keep alive" references active in the RenderProcessHost, recorded
// when Cleanup() was called.
optional uint32 keep_alive_ref_count = 2;
}

message ChildProcessLauncherPriority {
// True if the new priority set to background.
optional bool is_backgrounded = 1;
// True if the renderer proecss has pending views.
optional bool has_pending_views = 2;

// Importance of the child process in Android.
enum Importance {
IMPORTANCE_NORMAL = 1;
IMPORTANCE_MODERATE = 2;
IMPORTANCE_IMPORTANT = 3;
}
optional Importance importance = 3;
}

message ChromeTrackEvent {
// Extension range for Chrome: 1000-1999
// Next ID: 1017
Expand Down Expand Up @@ -218,12 +173,7 @@ message ChromeTrackEvent {

optional ChromeHashedPerformanceMark chrome_hashed_performance_mark = 1011;

optional RenderProcessHost render_process_host = 1012;
optional RenderProcessHostCleanup render_process_host_cleanup = 1013;
optional RenderProcessHostListener render_process_host_listener_changed =
1014;
optional ChildProcessLauncherPriority child_process_launcher_priority =
1015;
// reserved 1012 to 1015.

optional ResourceBundle resource_bundle = 1016;
}
Expand Down
38 changes: 20 additions & 18 deletions content/browser/browser_context.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,6 @@ namespace content {

namespace {

using perfetto::protos::pbzero::ChromeBrowserContext;
using perfetto::protos::pbzero::ChromeTrackEvent;

void SaveSessionStateOnIOThread(AppCacheServiceImpl* appcache_service) {
appcache_service->set_force_keep_session_state();
}
Expand All @@ -80,23 +77,33 @@ base::WeakPtr<storage::BlobStorageContext> BlobStorageContextGetterForBrowser(
} // namespace

BrowserContext::BrowserContext() {
impl_ = std::make_unique<Impl>(this);
TRACE_EVENT("shutdown", "BrowserContext::BrowserContext",
ChromeTrackEvent::kChromeBrowserContext, *this);
TRACE_EVENT_BEGIN("shutdown", "Browser.BrowserContext",
perfetto::Track::FromPointer(this),
ChromeTrackEvent::kChromeBrowserContext, *this);
[&](perfetto::EventContext ctx) {
auto* event =
ctx.event<perfetto::protos::pbzero::ChromeTrackEvent>();
event->set_chrome_browser_context()->set_ptr(
reinterpret_cast<uint64_t>(this));
});
TRACE_EVENT_NESTABLE_ASYNC_BEGIN1("shutdown", "Browser.BrowserContext", this,
"browser_context",
static_cast<void*>(this));

impl_ = std::make_unique<Impl>(this);
}

BrowserContext::~BrowserContext() {
TRACE_EVENT("shutdown", "BrowserContext::~BrowserContext",
ChromeTrackEvent::kChromeBrowserContext, *this);

// End for ASYNC event "Browser.BrowserContext".
TRACE_EVENT_END("shutdown", perfetto::Track::FromPointer(this),
ChromeTrackEvent::kChromeBrowserContext, *this);
[&](perfetto::EventContext ctx) {
auto* event =
ctx.event<perfetto::protos::pbzero::ChromeTrackEvent>();
event->set_chrome_browser_context()->set_ptr(
reinterpret_cast<uint64_t>(this));
});

impl_.reset();

TRACE_EVENT_NESTABLE_ASYNC_END1("shutdown", "Browser.BrowserContext", this,
"browser_context", static_cast<void*>(this));
}

DownloadManager* BrowserContext::GetDownloadManager() {
Expand Down Expand Up @@ -340,11 +347,6 @@ void BrowserContext::WriteIntoTrace(perfetto::TracedValue context) {
dict.Add("id", impl()->UniqueId());
}

void BrowserContext::WriteIntoTrace(
perfetto::TracedProto<ChromeBrowserContext> proto) {
proto->set_id(impl()->UniqueId());
}

//////////////////////////////////////////////////////////////////////////////
// The //content embedder can override the methods below to change or extend
// how the //content layer interacts with a BrowserContext. The code below
Expand Down
2 changes: 1 addition & 1 deletion content/browser/browser_main_runner_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ void BrowserMainRunnerImpl::Shutdown() {
main_loop_->PreShutdown();

// Finalize the startup tracing session if it is still active.
StartupTracingController::GetInstance().ShutdownAndWaitForStopIfNeeded();
StartupTracingController::GetInstance().WaitUntilStopped();

{
// The trace event has to stay between profiler creation and destruction.
Expand Down
23 changes: 0 additions & 23 deletions content/browser/child_process_launcher.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
#include "base/process/launch.h"
#include "base/process/process_metrics.h"
#include "base/time/time.h"
#include "base/tracing/protos/chrome_track_event.pbzero.h"
#include "build/build_config.h"
#include "content/public/browser/child_process_launcher_utils.h"
#include "content/public/common/content_features.h"
Expand All @@ -30,28 +29,6 @@ namespace content {

using internal::ChildProcessLauncherHelper;

void ChildProcessLauncherPriority::WriteIntoTrace(
perfetto::TracedProto<
perfetto::protos::pbzero::ChildProcessLauncherPriority> proto) {
proto->set_is_backgrounded(is_background());
proto->set_has_pending_views(boost_for_pending_views);

#if defined(OS_ANDROID)
using PriorityProto = perfetto::protos::pbzero::ChildProcessLauncherPriority;
switch (importance) {
case ChildProcessImportance::IMPORTANT:
proto->set_importance(PriorityProto::IMPORTANCE_IMPORTANT);
break;
case ChildProcessImportance::NORMAL:
proto->set_importance(PriorityProto::IMPORTANCE_NORMAL);
break;
case ChildProcessImportance::MODERATE:
proto->set_importance(PriorityProto::IMPORTANCE_MODERATE);
break;
}
#endif
}

#if defined(OS_ANDROID)
bool ChildProcessLauncher::Client::CanUseWarmUpConnection() {
return true;
Expand Down
13 changes: 0 additions & 13 deletions content/browser/child_process_launcher.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
#include "content/public/browser/child_process_termination_info.h"
#include "content/public/common/result_codes.h"
#include "mojo/public/cpp/system/invitation.h"
#include "third_party/perfetto/include/perfetto/tracing/traced_proto.h"

#if defined(OS_ANDROID)
#include "content/public/browser/android/child_process_importance.h"
Expand All @@ -33,14 +32,6 @@ namespace base {
class CommandLine;
}

namespace perfetto {
namespace protos {
namespace pbzero {
class ChildProcessLauncherPriority;
}
} // namespace protos
} // namespace perfetto

namespace content {

class SandboxedProcessLauncherDelegate;
Expand Down Expand Up @@ -97,10 +88,6 @@ struct ChildProcessLauncherPriority {
return !(*this == other);
}

void WriteIntoTrace(
perfetto::TracedProto<
perfetto::protos::pbzero::ChildProcessLauncherPriority> proto);

// Prefer |is_background()| to inspecting these fields individually (to ensure
// all logic uses the same notion of "backgrounded").

Expand Down
Loading

0 comments on commit 990b238

Please sign in to comment.