Commit 582fdbe0 authored by Francois Doray's avatar Francois Doray Committed by Commit Bot

PM: Fix invalid access to DocumentResourceCoordinator on freeze.

Currently, LocalFrame::DidFreeze performs these steps:

1. Obtain the DocumentResourceCoordinator.
2. Dispatch the beforeunload event.
3. Invoke SetHasNonEmptyBeforeUnload on the DocumentResourceCoordinator.

However, it is believed* that dispatching the beforeunload event can
invalidate the DocumentResourceCoordinator (by shutting down the
document), which causes an invalid access at step 3.

With this CL, the latest DocumentResourceCoordinator is obtained
just before calling SetHasNonEmptyBeforeUnload.

*Ideally, a Blink expert could confirm if that's a reasonable
 assumption.

Bug: 991380
Change-Id: Ie542b34a67900d427fbdc720f0d362522eba5da2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1749531Reviewed-by: default avatarKentaro Hara <haraken@chromium.org>
Commit-Queue: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#686587}
parent c2793fbe
......@@ -494,9 +494,7 @@ void LocalFrame::DidChangeVisibilityState() {
void LocalFrame::DidFreeze() {
if (GetDocument()) {
auto* document_resource_coordinator =
GetDocument()->GetResourceCoordinator();
if (document_resource_coordinator) {
if (GetDocument()->GetResourceCoordinator()) {
// Determine if there is a beforeunload handler by dispatching a
// beforeunload that will *not* launch a user dialog. If
// |proceed| is false then there is a non-empty beforeunload
......@@ -504,13 +502,21 @@ void LocalFrame::DidFreeze() {
bool unused_did_allow_navigation = false;
bool proceed = GetDocument()->DispatchBeforeUnloadEvent(
nullptr, false /* is_reload */, unused_did_allow_navigation);
document_resource_coordinator->SetHasNonEmptyBeforeUnload(!proceed);
// Running the beforeunload event may invalidate the
// DocumentResourceCoordinator. Because of that, it can't be stored in a
// local variable that is reused throughout the method.
// https://crbug.com/991380.
auto* document_resource_coordinator =
GetDocument()->GetResourceCoordinator();
if (document_resource_coordinator)
document_resource_coordinator->SetHasNonEmptyBeforeUnload(!proceed);
}
GetDocument()->DispatchFreezeEvent();
// TODO(fmeawad): Move the following logic to the page once we have a
// PageResourceCoordinator in Blink. http://crbug.com/838415
if (document_resource_coordinator) {
if (auto* document_resource_coordinator =
GetDocument()->GetResourceCoordinator()) {
document_resource_coordinator->SetLifecycleState(
resource_coordinator::mojom::LifecycleState::kFrozen);
}
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment