Skip to content

Commit

Permalink
Add traces and Chrometto triggers for crbug.com/838348
Browse files Browse the repository at this point in the history 
This and cl/359499417 will help us get the traces for the crash cases
and verify that speculative RFH deletion leads to the crash.

Bug: 838348
Change-Id: I8dac670a7f405581344dd2ba544efc7847651ec3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2719982
Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
Reviewed-by: Eric Seckler <eseckler@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#861849}
  • Loading branch information
@rakina
rakina authored and Chromium LUCI CQ committed Mar 11, 2021
1 parent 3bbe6fe commit b634336
Show file tree
Hide file tree
Showing 7 changed files with 236 additions and 4 deletions.
75 changes: 74 additions & 1 deletion base/tracing/protos/chrome_track_event.proto
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,75 @@ message ChromeMessagePumpForUI {
optional uint32 message_id = 1;
}

// An enumeration specifying the reason of the RenderFrame deletion.
// This is copied from content/common/frame.mojom.
enum FrameDeleteIntention {
// The frame being deleted isn't a (speculative) main frame.
FRAME_DELETE_INTENTION_NOT_MAIN_FRAME = 0;

// The frame being deleted is a speculative main frame, and it is being
// deleted as part of the shutdown for that WebContents. The entire RenderView
// etc will be destroyed by a separate IPC sent later.
FRAME_DELETE_INTENTION_SPECULATIVE_MAIN_FRAME_FOR_SHUTDOWN = 1;

// The frame being deleted is a speculative main frame, and it is being
// deleted because the speculative navigation was cancelled. This is not part
// of shutdown.
FRAME_DELETE_INTENTION_SPECULATIVE_MAIN_FRAME_FOR_NAVIGATION_CANCELLED = 2;
}

message RenderFrameImplDeletion {
// The intent for the deletion.
optional FrameDeleteIntention intent = 1;

// Whether the frame that's about to be deleted has a pending navigation
// commit.
optional bool has_pending_commit = 2;

// Whether the frame that's about to be deleted has a pending cross-document
// navigation commit.
optional bool has_pending_cross_document_commit = 3;

// The FrameTreeNode ID of the frame that's about to be deleted.
optional uint64 frame_tree_node_id = 4;
}

enum ShouldSwapBrowsingInstance {
// No BrowsingInstance swap.
SHOULD_SWAP_BROWSING_INSTANCE_NO = 0;

// Forced BrowsingInstance swap.
SHOULD_SWAP_BROWSING_INSTANCE_YES_FORCE_SWAP = 1;

// Proactive BrowsingInstance swap for cross-site navigation.
SHOULD_SWAP_BROWSING_INSTANCE_YES_CROSS_SITE_PROACTIVE_SWAP = 2;

// Proactive BrowsingInstance swap for same-site navigation.
SHOULD_SWAP_BROWSING_INSTANCE_YES_SAME_SITE_PROACTIVE_SWAP = 3;
}

message ShouldSwapBrowsingInstancesResult {
// The FrameTreeNode ID.
optional uint64 frame_tree_node_id = 1;

// Whether a navigation will do a BrowsingInstance swap or not.
optional ShouldSwapBrowsingInstance result = 2;
}

message FrameTreeNodeInfo {
// The FrameTreeNode ID.
optional uint64 frame_tree_node_id = 1;

// Whether the frame is a main frame or not.
optional bool is_main_frame = 2;

// Whether there's a speculative RenderFrameHost or not.
optional bool has_speculative_render_frame_host = 3;
}

message ChromeTrackEvent {
// Extension range for Chrome: 1000-1999
// Next ID: 1008
// Next ID: 1011
extend TrackEvent {
optional ChromeAppState chrome_app_state = 1000;

Expand All @@ -80,5 +146,12 @@ message ChromeTrackEvent {
optional ChromeTaskGraphRunner chrome_task_graph_runner = 1006;

optional ChromeMessagePumpForUI chrome_message_pump_for_ui = 1007;

optional RenderFrameImplDeletion render_frame_impl_deletion = 1008;

optional ShouldSwapBrowsingInstancesResult
should_swap_browsing_instances_result = 1009;

optional FrameTreeNodeInfo frame_tree_node_info = 1010;
}
}
36 changes: 36 additions & 0 deletions content/browser/renderer_host/render_frame_host_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
#include "base/task/post_task.h"
#include "base/threading/thread_task_runner_handle.h"
#include "base/time/time.h"
#include "base/trace_event/base_tracing.h"
#include "base/trace_event/optional_trace_event.h"
#include "base/trace_event/trace_conversion_helper.h"
#include "base/trace_event/traced_value.h"
Expand Down Expand Up @@ -834,6 +835,37 @@ bool ValidateCSPAttribute(const std::string& value) {
return true;
}

perfetto::protos::pbzero::FrameDeleteIntention FrameDeleteIntentionToProto(
mojom::FrameDeleteIntention intent) {
using ProtoLevel = perfetto::protos::pbzero::FrameDeleteIntention;
switch (intent) {
case mojom::FrameDeleteIntention::kNotMainFrame:
return ProtoLevel::FRAME_DELETE_INTENTION_NOT_MAIN_FRAME;
case mojom::FrameDeleteIntention::kSpeculativeMainFrameForShutdown:
return ProtoLevel::
FRAME_DELETE_INTENTION_SPECULATIVE_MAIN_FRAME_FOR_SHUTDOWN;
case mojom::FrameDeleteIntention::
kSpeculativeMainFrameForNavigationCancelled:
return ProtoLevel::
FRAME_DELETE_INTENTION_SPECULATIVE_MAIN_FRAME_FOR_NAVIGATION_CANCELLED;
}
// All cases should've been handled by the switch case above.
NOTREACHED();
return ProtoLevel::FRAME_DELETE_INTENTION_NOT_MAIN_FRAME;
}

void WriteRenderFrameImplDeletion(perfetto::EventContext& ctx,
RenderFrameHostImpl* rfh,
mojom::FrameDeleteIntention intent) {
auto* event = ctx.event<perfetto::protos::pbzero::ChromeTrackEvent>();
auto* data = event->set_render_frame_impl_deletion();
data->set_has_pending_commit(rfh->HasPendingCommitNavigation());
data->set_has_pending_cross_document_commit(
rfh->HasPendingCommitForCrossDocumentNavigation());
data->set_frame_tree_node_id(rfh->GetFrameTreeNodeId());
data->set_intent(FrameDeleteIntentionToProto(intent));
}

} // namespace

#if BUILDFLAG(ENABLE_PLUGINS)
Expand Down Expand Up @@ -2393,6 +2425,10 @@ void RenderFrameHostImpl::SetMojomFrameRemote(

void RenderFrameHostImpl::DeleteRenderFrame(
mojom::FrameDeleteIntention intent) {
TRACE_EVENT("navigation", "RenderFrameHostImpl::DeleteRenderFrame",
[&](perfetto::EventContext ctx) {
WriteRenderFrameImplDeletion(ctx, this, intent);
});
if (IsPendingDeletion())
return;

Expand Down
83 changes: 81 additions & 2 deletions content/browser/renderer_host/render_frame_host_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include "base/memory/ptr_util.h"
#include "base/notreached.h"
#include "base/stl_util.h"
#include "base/trace_event/base_tracing.h"
#include "base/trace_event/trace_event.h"
#include "build/build_config.h"
#include "content/browser/child_process_security_policy_impl.h"
Expand Down Expand Up @@ -198,6 +199,46 @@ void AppendReason(std::string* reason, const char* value) {
static_cast<size_t>(base::debug::CrashKeySize::Size256));
}

void WriteFrameTreeNodeInfo(perfetto::EventContext& ctx,
FrameTreeNode* frame_tree_node) {
auto* event = ctx.event<perfetto::protos::pbzero::ChromeTrackEvent>();
auto* ftn_info = event->set_frame_tree_node_info();
ftn_info->set_is_main_frame(frame_tree_node->IsMainFrame());
ftn_info->set_frame_tree_node_id(frame_tree_node->frame_tree_node_id());
ftn_info->set_has_speculative_render_frame_host(
!!frame_tree_node->render_manager()->speculative_frame_host());
}

perfetto::protos::pbzero::ShouldSwapBrowsingInstance
ShouldSwapBrowsingInstanceToProto(ShouldSwapBrowsingInstance result) {
using ProtoLevel = perfetto::protos::pbzero::ShouldSwapBrowsingInstance;
switch (result) {
case ShouldSwapBrowsingInstance::kYes_ForceSwap:
return ProtoLevel::SHOULD_SWAP_BROWSING_INSTANCE_YES_FORCE_SWAP;
case ShouldSwapBrowsingInstance::kYes_CrossSiteProactiveSwap:
return ProtoLevel::
SHOULD_SWAP_BROWSING_INSTANCE_YES_CROSS_SITE_PROACTIVE_SWAP;
case ShouldSwapBrowsingInstance::kYes_SameSiteProactiveSwap:
return ProtoLevel::
SHOULD_SWAP_BROWSING_INSTANCE_YES_SAME_SITE_PROACTIVE_SWAP;
default:
return ProtoLevel::SHOULD_SWAP_BROWSING_INSTANCE_NO;
}
}

void TraceShouldSwapBrowsingInstanceResult(int frame_tree_node_id,
ShouldSwapBrowsingInstance result) {
TRACE_EVENT_INSTANT(
"navigation",
"RenderFrameHostManager::GetSiteInstanceForNavigation_ShouldSwapResult",
[&](perfetto::EventContext ctx) {
auto* event = ctx.event<perfetto::protos::pbzero::ChromeTrackEvent>();
auto* data = event->set_should_swap_browsing_instances_result();
data->set_frame_tree_node_id(frame_tree_node_id);
data->set_result(ShouldSwapBrowsingInstanceToProto(result));
});
}

} // namespace

RenderFrameHostManager::RenderFrameHostManager(FrameTreeNode* frame_tree_node,
Expand Down Expand Up @@ -639,8 +680,11 @@ bool RenderFrameHostManager::DeleteFromPendingList(

void RenderFrameHostManager::RestoreFromBackForwardCache(
std::unique_ptr<BackForwardCacheImpl::Entry> entry) {
TRACE_EVENT0("navigation",
"RenderFrameHostManager::RestoreFromBackForwardCache");
TRACE_EVENT("navigation",
"RenderFrameHostManager::RestoreFromBackForwardCache",
[&](perfetto::EventContext ctx) {
WriteFrameTreeNodeInfo(ctx, frame_tree_node_);
});
// Matched in CommitPending().
entry->render_frame_host->GetProcess()->AddPendingView();

Expand Down Expand Up @@ -685,6 +729,11 @@ bool RenderFrameHostManager::HasPendingCommitForCrossDocumentNavigation()

void RenderFrameHostManager::DidCreateNavigationRequest(
NavigationRequest* request) {
TRACE_EVENT("navigation",
"RenderFrameHostManager::DidCreateNavigationRequest",
[&](perfetto::EventContext ctx) {
WriteFrameTreeNodeInfo(ctx, frame_tree_node_);
});
const bool force_use_current_render_frame_host =
// Since the frame from the back-forward cache is being committed to the
// SiteInstance we already have, it is treated as current.
Expand Down Expand Up @@ -731,6 +780,11 @@ void RenderFrameHostManager::DidCreateNavigationRequest(
RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
NavigationRequest* request,
std::string* reason) {
TRACE_EVENT("navigation", "RenderFrameHostManager::GetFrameHostForNavigation",
[&](perfetto::EventContext ctx) {
WriteFrameTreeNodeInfo(ctx, frame_tree_node_);
});

DCHECK(!request->common_params().url.SchemeIs(url::kJavaScriptScheme))
<< "Don't call this method for JavaScript URLs as those create a "
"temporary NavigationRequest and we don't want to reset an ongoing "
Expand Down Expand Up @@ -1035,6 +1089,10 @@ void RenderFrameHostManager::MaybeCleanUpNavigation() {
}

void RenderFrameHostManager::CleanUpNavigation() {
TRACE_EVENT("navigation", "RenderFrameHostManager::CleanUpNavigation",
[&](perfetto::EventContext ctx) {
WriteFrameTreeNodeInfo(ctx, frame_tree_node_);
});
if (speculative_render_frame_host_) {
bool was_loading = speculative_render_frame_host_->is_loading();
DiscardUnusedFrame(UnsetSpeculativeRenderFrameHost());
Expand All @@ -1055,6 +1113,11 @@ void RenderFrameHostManager::CleanUpNavigation() {

std::unique_ptr<RenderFrameHostImpl>
RenderFrameHostManager::UnsetSpeculativeRenderFrameHost() {
TRACE_EVENT("navigation",
"RenderFrameHostManager::UnsetSpeculativeRenderFrameHost",
[&](perfetto::EventContext ctx) {
WriteFrameTreeNodeInfo(ctx, frame_tree_node_);
});
speculative_render_frame_host_->GetProcess()->RemovePendingView();
speculative_render_frame_host_->DeleteRenderFrame(
frame_tree_node_->parent()
Expand Down Expand Up @@ -1655,6 +1718,10 @@ RenderFrameHostManager::GetSiteInstanceForNavigation(
is_failure, is_reload, is_same_document,
cross_origin_opener_policy_mismatch, was_server_redirect,
should_replace_current_entry, is_speculative);

TraceShouldSwapBrowsingInstanceResult(frame_tree_node_->frame_tree_node_id(),
should_swap_result);

bool proactive_swap =
(should_swap_result ==
ShouldSwapBrowsingInstance::kYes_CrossSiteProactiveSwap ||
Expand Down Expand Up @@ -2480,6 +2547,12 @@ std::unique_ptr<RenderFrameHostImpl>
RenderFrameHostManager::CreateSpeculativeRenderFrame(
SiteInstance* instance,
bool recovering_without_early_commit) {
TRACE_EVENT("navigation",
"RenderFrameHostManager::CreateSpeculativeRenderFrame",
[&](perfetto::EventContext ctx) {
WriteFrameTreeNodeInfo(ctx, frame_tree_node_);
});

CHECK(instance);
// This DCHECK is going to be fully removed as part of RenderDocument [1].
//
Expand Down Expand Up @@ -3468,6 +3541,12 @@ void RenderFrameHostManager::EnsureRenderFrameHostPageFocusConsistent() {
}

void RenderFrameHostManager::CreateNewFrameForInnerDelegateAttachIfNecessary() {
TRACE_EVENT(
"navigation",
"RenderFrameHostManager::CreateNewFrameForInnerDelegateAttachIfNecessary",
[&](perfetto::EventContext ctx) {
WriteFrameTreeNodeInfo(ctx, frame_tree_node_);
});
DCHECK(is_attaching_inner_delegate());
// Remove all navigations and any speculative frames which might interfere
// with the loading state.
Expand Down
2 changes: 1 addition & 1 deletion content/public/browser/content_browser_client.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
// declarations instead of including more headers. If that is infeasible, adjust
// the limit. For more info, see
// https://chromium.googlesource.com/chromium/src/+/HEAD/docs/wmax_tokens.md
#pragma clang max_tokens_here 851000
#pragma clang max_tokens_here 852000

#include <utility>

Expand Down
26 changes: 26 additions & 0 deletions content/renderer/render_frame_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
#include "base/task_runner_util.h"
#include "base/threading/thread_task_runner_handle.h"
#include "base/time/time.h"
#include "base/trace_event/base_tracing.h"
#include "base/trace_event/trace_event.h"
#include "base/unguessable_token.h"
#include "build/build_config.h"
Expand Down Expand Up @@ -1134,6 +1135,25 @@ std::string GetUniqueNameOfWebFrame(WebFrame* web_frame) {
->unique_name();
}

perfetto::protos::pbzero::FrameDeleteIntention FrameDeleteIntentionToProto(
mojom::FrameDeleteIntention intent) {
using ProtoLevel = perfetto::protos::pbzero::FrameDeleteIntention;
switch (intent) {
case mojom::FrameDeleteIntention::kNotMainFrame:
return ProtoLevel::FRAME_DELETE_INTENTION_NOT_MAIN_FRAME;
case mojom::FrameDeleteIntention::kSpeculativeMainFrameForShutdown:
return ProtoLevel::
FRAME_DELETE_INTENTION_SPECULATIVE_MAIN_FRAME_FOR_SHUTDOWN;
case mojom::FrameDeleteIntention::
kSpeculativeMainFrameForNavigationCancelled:
return ProtoLevel::
FRAME_DELETE_INTENTION_SPECULATIVE_MAIN_FRAME_FOR_NAVIGATION_CANCELLED;
}
// All cases should've been handled by the switch case above.
NOTREACHED();
return ProtoLevel::FRAME_DELETE_INTENTION_NOT_MAIN_FRAME;
}

} // namespace

RenderFrameImpl::AssertNavigationCommits::AssertNavigationCommits(
Expand Down Expand Up @@ -2330,6 +2350,12 @@ void RenderFrameImpl::Unload(
}

void RenderFrameImpl::Delete(mojom::FrameDeleteIntention intent) {
TRACE_EVENT(
"navigation", "RenderFrameImpl::Delete", [&](perfetto::EventContext ctx) {
auto* event = ctx.event<perfetto::protos::pbzero::ChromeTrackEvent>();
auto* data = event->set_render_frame_impl_deletion();
data->set_intent(FrameDeleteIntentionToProto(intent));
});
// The main frame (when not provisional) is owned by the renderer's frame tree
// via WebViewImpl. When a provisional main frame is swapped in, the ownership
// moves from the browser to the renderer, but this happens in the renderer
Expand Down
7 changes: 7 additions & 0 deletions third_party/blink/renderer/core/page/chrome_client_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -893,6 +893,13 @@ void ChromeClientImpl::NotifyPresentationTime(LocalFrame& frame,

void ChromeClientImpl::RequestBeginMainFrameNotExpected(LocalFrame& frame,
bool request) {
if (!frame.GetWidgetForLocalRoot()) {
// We're about to crash due to crbug.com/838348. Record metrics to trigger
// Chrometto. We only expect this to be recorded on Android in any
// significant numbers (because we CHECK in RenderFrameImpl::Delete).
UMA_HISTOGRAM_BOOLEAN("Navigation.RequestBeginMainFrameNotExpectedCrash",
frame.IsMainFrame());
}
frame.GetWidgetForLocalRoot()->RequestBeginMainFrameNotExpected(request);
}

Expand Down
11 changes: 11 additions & 0 deletions tools/metrics/histograms/histograms_xml/navigation/histograms.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -974,6 +974,17 @@ reviews. Googlers can read more about this at go/gwsq-gerrit.
</summary>
</histogram>

<histogram name="Navigation.RequestBeginMainFrameNotExpectedCrash"
enum="BooleanPresent" expires_after="2021-08-22">
<owner>rakina@chromium.org</owner>
<owner>dcheng@chromium.org</owner>
<summary>
Logs cases where ChromeClientImpl::RequestBeginMainFrameNotExpected() is
about to crash, which will trigger crbug.com/838348. This is used to trigger
traces to be uploaded to analyze what happened in these traces.
</summary>
</histogram>

<histogram name="Navigation.RequiresDedicatedProcess"
enum="NavigationRequiresDedicatedProcess" expires_after="2021-12-21">
<owner>alexmos@chromium.org</owner>
Expand Down

0 comments on commit b634336

Please sign in to comment.