Skip to content

Commit

Permalink
Re-enable should_replace_current_entry check in most cases
Browse files Browse the repository at this point in the history
The check was removed in crrev.com/c/2818538 because it doesn't work for
some iframe cases, especially after document.open(). This CL re-enables
the check for cases that we think should still work (main frames or
when has_committed_real_load() == true)

Bug: 1204981
Change-Id: I72d11b21995e4ac5e9fd73efec5cab5bebd3ca56
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2873627
Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
Cr-Commit-Position: refs/heads/master@{#880330}
  • Loading branch information
rakina authored and Chromium LUCI CQ committed May 7, 2021
1 parent c0f9c2a commit ed21504
Showing 1 changed file with 33 additions and 1 deletion.
34 changes: 33 additions & 1 deletion content/browser/renderer_host/render_frame_host_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10437,6 +10437,7 @@ void RenderFrameHostImpl::
// - http_status_code
// - should_update_history
// - gesture
// - should_replace_current_entry
// TODO(crbug.com/1131832): Verify more params.
// We can know if we're going to be in an error page after this navigation
// if the net error code is not net::OK, or if we're doing a same-document
Expand Down Expand Up @@ -10484,6 +10485,21 @@ void RenderFrameHostImpl::
const bool renderer_gesture =
(params.gesture == NavigationGesture::NavigationGestureUser);

const bool browser_should_replace_current_entry =
CalculateShouldReplaceCurrentEntry(
request, same_document_params.Clone(), frame_tree_node_,
last_committed_url_, is_loaded_from_load_data_with_base_url_,
last_base_url_);
// Currently it's not possible to correctly predict the value of
// should_replace_current_entry in the browser for iframes with
// has_committed_real_load == false because some cases like document.open()
// will affect the result in the renderer (through EmptyDocumentStatus) but
// the browser don't have a way to know that it happened.
// See https://crbug.com/1204981 and https://crrev.com/c/2818538.
const bool ignore_should_replace_current_entry_difference =
(!frame_tree_node_->IsMainFrame() &&
!frame_tree_node_->has_committed_real_load());

if ((!ShouldVerify("intended_as_new_entry") ||
request->commit_params().intended_as_new_entry ==
params.intended_as_new_entry) &&
Expand All @@ -10497,7 +10513,11 @@ void RenderFrameHostImpl::
browser_http_status_code == params.http_status_code) &&
(!ShouldVerify("should_update_history") ||
browser_should_update_history == params.should_update_history) &&
(!ShouldVerify("gesture") || browser_gesture == renderer_gesture)) {
(!ShouldVerify("gesture") || browser_gesture == renderer_gesture) &&
(!ShouldVerify("should_replace_current_entry") ||
ignore_should_replace_current_entry_difference ||
browser_should_replace_current_entry ==
params.should_replace_current_entry)) {
return;
}

Expand Down Expand Up @@ -10572,6 +10592,11 @@ void RenderFrameHostImpl::
SCOPED_CRASH_KEY_BOOL("VerifyDidCommit", "gesture_renderer",
renderer_gesture);

SCOPED_CRASH_KEY_BOOL("VerifyDidCommit", "replace_browser",
browser_should_replace_current_entry);
SCOPED_CRASH_KEY_BOOL("VerifyDidCommit", "replace_renderer",
params.should_replace_current_entry);

SCOPED_CRASH_KEY_BOOL("VerifyDidCommit", "is_same_document",
is_same_document_navigation);
SCOPED_CRASH_KEY_BOOL("VerifyDidCommit", "is_history_api",
Expand Down Expand Up @@ -10673,6 +10698,8 @@ void RenderFrameHostImpl::
DCHECK_EQ(browser_http_status_code, params.http_status_code);
DCHECK_EQ(browser_should_update_history, params.should_update_history);
DCHECK_EQ(browser_gesture, renderer_gesture);
DCHECK_EQ(browser_should_replace_current_entry,
params.should_replace_current_entry);

// Log histograms to trigger Chrometto slow reports, allowing us to see traces
// to analyze what happened in these navigations.
Expand Down Expand Up @@ -10709,6 +10736,11 @@ void RenderFrameHostImpl::
LogVerifyDidCommitParamsDifference(
VerifyDidCommitParamsDifference::kGesture);
}
if (browser_should_replace_current_entry !=
params.should_replace_current_entry) {
LogVerifyDidCommitParamsDifference(
VerifyDidCommitParamsDifference::kShouldReplaceCurrentEntry);
}

base::debug::DumpWithoutCrashing();
}
Expand Down

0 comments on commit ed21504

Please sign in to comment.