chromium/src
0
Fork 0
Code Activity

Revert "Add DumpWithoutCrashing code for beforeunload investigation [2]"

Browse Source 
This reverts commit db38b3890d.

Reason for revert:
LUCI Bisection has identified this change as the cause of a test failure. See the analysis: https://ci.chromium.org/ui/p/chromium/bisection/test-analysis/b/6268986028195840

Sample build with failed test: https://ci.chromium.org/b/8718548243020660529
Affected test(s):
[ninja://components:components_browsertests/NetErrorAutoReloaderBrowserTest.NavigationCancelsAutoReload](https://ci.chromium.org/ui/test/chromium/ninja:%2F%2Fcomponents:components_browsertests%2FNetErrorAutoReloaderBrowserTest.NavigationCancelsAutoReload?q=VHash%3Acfbec673441b805c)
[ninja://components:components_browsertests/NetErrorAutoReloaderBrowserTest.StopCancelsAutoReload](https://ci.chromium.org/ui/test/chromium/ninja:%2F%2Fcomponents:components_browsertests%2FNetErrorAutoReloaderBrowserTest.StopCancelsAutoReload?q=VHash%3Acfbec673441b805c)
[ninja://components:components_browsertests/SourceUrlRecorderWebContentsObserverDownloadBrowserTest.IgnoreDownload](https://ci.chromium.org/ui/test/chromium/ninja:%2F%2Fcomponents:components_browsertests%2FSourceUrlRecorderWebContentsObserverDownloadBrowserTest.IgnoreDownload?q=VHash%3Acfbec673441b805c)
[ninja://extensions:extensions_browsertests/WebViewAPITest.TestDeclarativeWebRequestAPI](https://ci.chromium.org/ui/test/chromium/ninja:%2F%2Fextensions:extensions_browsertests%2FWebViewAPITest.TestDeclarativeWebRequestAPI?q=VHash%3Acd1faec67993975e)
[ninja://extensions:extensions_browsertests/WebViewAPITest.TestDeclarativeWebRequestAPISendMessage](https://ci.chromium.org/ui/test/chromium/ninja:%2F%2Fextensions:extensions_browsertests%2FWebViewAPITest.TestDeclarativeWebRequestAPISendMessage?q=VHash%3Acd1faec67993975e)
and 19 more ...

If this is a false positive, please report it at http://b.corp.google.com/createIssue?component=1199205&description=Analysis%3A+https%3A%2F%2Fci.chromium.org%2Fui%2Fp%2Fchromium%2Fbisection%2Ftest-analysis%2Fb%2F6268986028195840&format=PLAIN&priority=P3&title=Wrongly+blamed+https%3A%2F%2Fchromium-review.googlesource.com%2Fc%2Fchromium%2Fsrc%2F%2B%2F6400556&type=BUG

Original change's description:
> Add DumpWithoutCrashing code for beforeunload investigation [2]
>
> In theory, navigation can synchronously continue if the frame being
> navigated (and all child frames) do not have beforeunload handlers. But,
> synchronously continuing with navigation can lead to reentrancy in
> content, which is not supported (triggers CHECKs). Android WebView is
> known to cause reentrancy for such synchronous navigations.
>
> This CL adds DumpWithoutCrashing to investigate the actual cases where
> the reentrancy cases happen.
>
> Android WebView is explicitly opted out from this investigation via
> AwContentBrowserClient::SupportsAvoidUnnecessaryBeforeUnloadCheckSync().
>
> Bug: 396998476
> Bug: 40361673
> Bug: 406397217
> Change-Id: I0e5c9f51a5bdf8fdf7dce64a58a58fb65c1122c0
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6400556
> Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
> Reviewed-by: Charlie Reis <creis@chromium.org>
> Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
> Commit-Queue: Minoru Chikamune <chikamune@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1442563}
>

Bug: 396998476
Bug: 40361673
Bug: 406397217
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Change-Id: I5868655d91d900ee10f996ed7e75923fb98dcb9f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6432541
Commit-Queue: Yuki Shiino <yukishiino@chromium.org>
Owners-Override: Yuki Shiino <yukishiino@chromium.org>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#1442597}
This commit is contained in:
luci-bisection@appspot.gserviceaccount.com
2025-04-03 23:47:58 -07:00
committed by Chromium LUCI CQ
parent 81ac3b71d0
commit 844eb967a3
4 changed files with 3 additions and 116 deletions

17
content/browser/renderer_host/navigation_controller_impl.cc

@ -2613,9 +2613,6 @@ void NavigationControllerImpl::DiscardPendingEntry(bool was_failure) {
// when the tab is being destroyed for shutdown, since it won't return to
// NavigateToEntry in that case.) http://crbug.com/347742.
CHECK(!in_navigate_to_pending_entry_ || frame_tree_->IsBeingDestroyed());
if (!frame_tree_->IsBeingDestroyed()) {
CheckPotentialNavigationReentrancy();
}
if (was_failure && pending_entry_) {
failed_pending_entry_id_ = pending_entry_->GetUniqueID();
@ -3383,7 +3380,6 @@ NavigationControllerImpl::NavigateToExistingPendingEntry(
// This call does not support re-entrancy. See http://crbug.com/347742.
CHECK(!in_navigate_to_pending_entry_);
in_navigate_to_pending_entry_ = true;
CheckPotentialNavigationReentrancy();
// If the navigation-reentrancy is caused by calling
// NavigateToExistingPendingEntry twice, this will note the previous call's
@ -3807,7 +3803,6 @@ base::WeakPtr<NavigationHandle> NavigationControllerImpl::NavigateWithoutEntry(
// This call does not support re-entrancy. See http://crbug.com/347742.
CHECK(!in_navigate_to_pending_entry_);
in_navigate_to_pending_entry_ = true;
CheckPotentialNavigationReentrancy();
// It is not possible to delete the pending NavigationEntry while navigating
// to it. Grab a reference to delay potential deletion until the end of this
@ -5032,16 +5027,4 @@ void NavigationControllerImpl::DidChangeReferrerPolicy(
ShouldProtectUrlInNavigationApi(referrer_policy));
}
void NavigationControllerImpl::CheckPotentialNavigationReentrancy() {
// DumpWithoutCrashing will be reported just once in one Chrome session since
// we want to avoid too many reports.
static bool has_dumped_without_crashing = false;
if (can_be_in_navigate_to_pending_entry_ && !has_dumped_without_crashing) {
has_dumped_without_crashing = true;
// This DumpWithoutCrashing is an investigation code for
// https://crbug.com/396998476.
base::debug::DumpWithoutCrashing();
}
}
} // namespace content

30
content/browser/renderer_host/navigation_controller_impl.h

@ -474,15 +474,6 @@ class CONTENT_EXPORT NavigationControllerImpl : public NavigationController {
return in_navigate_to_pending_entry_;
}
// This flag is set from RenderFrameHostImpl::SendBeforeUnload() to
// investigate whether kAvoidUnnecessaryBeforeUnloadCheckSync feature is safe
// to enable or not (see: https://crbug.com/40361673,
// https://crbug.com/396998476).
void set_can_be_in_navigate_to_pending_entry(
const bool can_be_in_navigate_to_pending_entry) {
can_be_in_navigate_to_pending_entry_ = can_be_in_navigate_to_pending_entry;
}
// Whether to maintain a session history with just one entry.
//
// This returns true for a prerendering page and for fenced frames.
@ -907,16 +898,6 @@ class CONTENT_EXPORT NavigationControllerImpl : public NavigationController {
FrameNavigationEntry* target_entry,
const std::string& navigation_api_key);
// When navigation starts, the `can_be_in_navigate_to_pending_entry` flag has
// to be false. This is because kAvoidUnnecessaryBeforeUnloadCheckSync feature
// will stop using PostTask for the legacy beforeunload code in the near
// future. When kAvoidUnnecessaryBeforeUnloadCheckSync is enabled,
// `RenderFrameHostImpl::ProcessBeforeUnloadCompletedFromFrame()` and
// `Navigator::BeforeUnloadCompleted()` can run in the scope of
// `in_navigate_to_pending_entry_` == true, and it might end up crashing on
// CHECK(!in_navigate_to_pending_entry_).
void CheckPotentialNavigationReentrancy();
// ---------------------------------------------------------------------------
// The FrameTree this instance belongs to. Each FrameTree gets its own
@ -984,17 +965,6 @@ class CONTENT_EXPORT NavigationControllerImpl : public NavigationController {
// Prevent unsafe re-entrant calls to NavigateToPendingEntry.
bool in_navigate_to_pending_entry_ = false;
// A flag to investigate whether kAvoidUnnecessaryBeforeUnloadCheckSync
// feature is safe to enable or not (see: https://crbug.com/40361673,
// https://crbug.com/396998476).
//
// This flag is true if the above `in_navigate_to_pending_entry_` flag is true
// when RenderFrameHostImpl::SendBeforeUnload() runs, and on top of that, when
// we intend to continue navigation synchronously without posting a task when
// the kRunBeforeUnloadClosureOnStack or
// kAvoidUnnecessaryBeforeUnloadCheckSync is enabled.
bool can_be_in_navigate_to_pending_entry_ = false;
// Used to find the appropriate SessionStorageNamespace for the storage
// partition of a NavigationEntry.
//

64
content/browser/renderer_host/render_frame_host_impl.cc

@ -354,10 +354,6 @@ BASE_FEATURE(kDoNotEvictOnAXLocationChange,
"DoNotEvictOnAXLocationChange",
base::FEATURE_ENABLED_BY_DEFAULT);
BASE_FEATURE(kRunBeforeUnloadClosureOnStackInvestigation,
"RunBeforeUnloadClosureOnStackInvestigation",
base::FEATURE_DISABLED_BY_DEFAULT);
} // namespace features
namespace content {
@ -997,16 +993,6 @@ GURL GetLastDocumentURL(
return params.url;
}
bool IsRunBeforeUnloadClosureOnStackInvestigationEnabled() {
static const bool kEnabled =
GetContentClient()
->browser()
->SupportsAvoidUnnecessaryBeforeUnloadCheckSync() &&
base::FeatureList::IsEnabled(
features::kRunBeforeUnloadClosureOnStackInvestigation);
return kEnabled;
}
// Returns true if `host` has the Window Management permission granted.
bool IsWindowManagementGranted(RenderFrameHost* host) {
content::PermissionController* permission_controller =
@ -16363,37 +16349,6 @@ void RenderFrameHostImpl::SendBeforeUnload(
},
rfh, for_legacy);
if (for_legacy) {
// We would like to synchronously continue navigation without the following
// PostTask in the future to improve performance if the frame being
// navigated (and all child frames) do not have beforeunload handlers.
// However, as described in
// `ContentBrowserClient::SupportsAvoidUnnecessaryBeforeUnloadCheckSync()`,
// this can result in re-entrancy issues on navigation. The re-entrancy is
// checked by NavigationController's `in_navigate_to_pending_entry` flag.
// While this flag is true, we prohibit starting another navigation
// synchronously while the existing navigation is still being processed on
// the stack (CHECK(!in_navigate_to_pending_entry_)).
//
// The following `is_eligible_for_avoid_unnecessary_beforeunload` flag is
// used to allow synchronous continuation of navigation when the
// kRunBeforeUnloadClosureOnStack or kAvoidUnnecessaryBeforeUnloadCheckSync
// feature is enabled.
//
// The following `can_be_in_navigate_to_pending_entry` flag is used to
// investigate whether it is safe to do so, by checking whether the CHECK
// would've failed if we continue synchronously instead of posting a task.
// This flag is only used when both kRunBeforeUnloadClosureOnStack and
// kAvoidUnnecessaryBeforeUnloadCheckSync are disabled.
const bool is_eligible_for_avoid_unnecessary_beforeunload =
is_waiting_for_beforeunload_completion_ &&
unload_ack_is_for_navigation_ &&
GetContentClient()
->browser()
->SupportsAvoidUnnecessaryBeforeUnloadCheckSync();
const bool can_be_in_navigate_to_pending_entry =
IsRunBeforeUnloadClosureOnStackInvestigationEnabled() &&
is_eligible_for_avoid_unnecessary_beforeunload &&
frame_tree()->controller().in_navigate_to_pending_entry();
// Use a high-priority task to continue the navigation. This is safe as it
// happens early in the navigation flow and shouldn't race with any other
// tasks associated with this navigation.
@ -16402,33 +16357,18 @@ void RenderFrameHostImpl::SendBeforeUnload(
FROM_HERE,
base::BindOnce(
[](blink::mojom::LocalFrame::BeforeUnloadCallback callback,
base::TimeTicks start_time, base::TimeTicks end_time,
base::WeakPtr<NavigationControllerImpl>
navigation_controller,
const bool can_be_in_navigate_to_pending_entry) {
base::TimeTicks start_time, base::TimeTicks end_time) {
// Measures the time a posted task spends in the queue before
// execution. Recorded only when `for_legacy` is true.
base::UmaHistogramTimes(
"Navigation.OnBeforeUnloadOverheadTime."
"NoBeforeUnloadHandlerRegistered",
base::TimeTicks::Now() - end_time);
if (can_be_in_navigate_to_pending_entry &&
navigation_controller) {
navigation_controller
->set_can_be_in_navigate_to_pending_entry(true);
}
std::move(callback).Run(/*proceed=*/true, start_time,
end_time);
if (can_be_in_navigate_to_pending_entry &&
navigation_controller) {
navigation_controller
->set_can_be_in_navigate_to_pending_entry(false);
}
},
std::move(before_unload_closure),
send_before_unload_start_time_, base::TimeTicks::Now(),
frame_tree()->controller().GetWeakPtr(),
can_be_in_navigate_to_pending_entry));
send_before_unload_start_time_, base::TimeTicks::Now()));
return;
}
auto scope = MakeUrgentMessageScopeIfNeeded();

8
testing/variations/fieldtrial_testing_config.json

@ -20561,13 +20561,7 @@
{
"platforms": [
"android",
"android_webview",
"chromeos",
"chromeos_lacros",
"fuchsia",
"linux",
"mac",
"windows"
"android_webview"
],
"experiments": [
{