Skip to content

Commit

Permalink
Don't dispatch unload if we've dispatched pagehide with persisted == …
Browse files Browse the repository at this point in the history 
…true

If we've dispatched the 'pagehide' event with the 'persisted' property
set to true, we should not dispatch the unload event after that. This is
because the 'persisted' property is set to the 'salvageable' status of
the page, which also determines whether the unload event is fired.

If 'salvageable' is true, 'persisted' will be true and 'unload' won't
be dispatched. If 'salvageable' is false, 'persisted' will be false
and 'unload' should be dispatched.

Relevant spec PR: whatwg/html#5889
More context: https://groups.google.com/a/google.com/g/chrome-bfcache/c/L-ZreZDY4n0/m/jna_jQJkCQAJ

Bug: 1110744
Change-Id: I54b44cfdcb6c2922ca57071d695df6c4c2d77d77
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2455510
Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#815898}
GitOrigin-RevId: 67d49f2adf8c7b9dbe6166b152a7901addb02d5c
  • Loading branch information
@rakina @copybara-github
rakina authored and copybara-github committed Oct 10, 2020
1 parent 7ec753d commit 0a6a98d
Show file tree
Hide file tree
Showing 3 changed files with 94 additions and 73 deletions.
158 changes: 85 additions & 73 deletions blink/renderer/core/dom/document.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4087,83 +4087,95 @@ void Document::DispatchUnloadEvents(
if (parser_)
parser_->StopParsing();

if (load_event_progress_ == kLoadEventNotRun)
return;

if (load_event_progress_ <= kUnloadEventInProgress) {
Element* current_focused_element = FocusedElement();
if (auto* input = DynamicTo<HTMLInputElement>(current_focused_element))
input->EndEditing();
if (load_event_progress_ < kPageHideInProgress) {
load_event_progress_ = kPageHideInProgress;
LocalDOMWindow* window = domWindow();
if (window && !GetPage()->DispatchedPagehideAndStillHidden()) {
const base::TimeTicks pagehide_event_start = base::TimeTicks::Now();
window->DispatchEvent(
*PageTransitionEvent::Create(event_type_names::kPagehide, false),
this);
const base::TimeTicks pagehide_event_end = base::TimeTicks::Now();
DEFINE_STATIC_LOCAL(
CustomCountHistogram, pagehide_histogram,
("DocumentEventTiming.PageHideDuration", 0, 10000000, 50));
pagehide_histogram.CountMicroseconds(pagehide_event_end -
pagehide_event_start);
}
if (!dom_window_)
return;
if (load_event_progress_ == kLoadEventNotRun ||
load_event_progress_ > kUnloadEventInProgress) {
return;
}

// This must be queried before |load_event_progress_| is changed to
// kUnloadVisibilityChangeInProgress because that would change the result.
bool page_visible = IsPageVisible();
load_event_progress_ = kUnloadVisibilityChangeInProgress;
if (page_visible) {
// Dispatch visibilitychange event, but don't bother doing
// other notifications as we're about to be unloaded.
const base::TimeTicks pagevisibility_hidden_event_start =
base::TimeTicks::Now();
DispatchEvent(
*Event::CreateBubble(event_type_names::kVisibilitychange));
const base::TimeTicks pagevisibility_hidden_event_end =
base::TimeTicks::Now();
DEFINE_STATIC_LOCAL(CustomCountHistogram, pagevisibility_histogram,
("DocumentEventTiming.PageVibilityHiddenDuration",
0, 10000000, 50));
pagevisibility_histogram.CountMicroseconds(
pagevisibility_hidden_event_end -
pagevisibility_hidden_event_start);
DispatchEvent(
*Event::CreateBubble(event_type_names::kWebkitvisibilitychange));
}
if (!dom_window_)
return;
Element* current_focused_element = FocusedElement();
if (auto* input = DynamicTo<HTMLInputElement>(current_focused_element))
input->EndEditing();

GetFrame()->Loader().SaveScrollAnchor();

load_event_progress_ = kUnloadEventInProgress;
Event& unload_event = *Event::Create(event_type_names::kUnload);
const base::TimeTicks unload_event_start = base::TimeTicks::Now();
dom_window_->DispatchEvent(unload_event, this);
const base::TimeTicks unload_event_end = base::TimeTicks::Now();

if (unload_timing) {
// Record unload event timing when navigating cross-document.
DEFINE_STATIC_LOCAL(
CustomCountHistogram, unload_histogram,
("DocumentEventTiming.UnloadDuration", 0, 10000000, 50));
unload_histogram.CountMicroseconds(unload_event_end -
unload_event_start);

// Fill in the unload timing if the new document origin has access to
// them.
if (committing_origin->CanRequest(Url())) {
auto& timing = unload_timing->emplace();
timing.unload_event_start = unload_event_start;
timing.unload_event_end = unload_event_end;
}
}
}
// If we've dispatched the pagehide event with 'persisted' set to true, it
// means we've dispatched the visibilitychange event before too. Also, we
// shouldn't dispatch the unload event because that event should only be
// fired when the pagehide event's 'persisted' bit is set to false.
bool dispatched_pagehide_persisted =
GetPage() && GetPage()->DispatchedPagehidePersistedAndStillHidden();

if (load_event_progress_ >= kPageHideInProgress ||
dispatched_pagehide_persisted) {
load_event_progress_ = kUnloadEventHandled;
return;
}

load_event_progress_ = kPageHideInProgress;
LocalDOMWindow* window = domWindow();
// We check for DispatchedPagehideAndStillHidden() here because it's possible
// to dispath pagehide with 'persisted' set to false before this and pass the
// |dispatched_pagehide_persisted| above, if we enable same-site
// ProactivelySwapBrowsingInstance but not BackForwardCache.
if (window && !GetPage()->DispatchedPagehideAndStillHidden()) {
const base::TimeTicks pagehide_event_start = base::TimeTicks::Now();
window->DispatchEvent(
*PageTransitionEvent::Create(event_type_names::kPagehide, false), this);
const base::TimeTicks pagehide_event_end = base::TimeTicks::Now();
DEFINE_STATIC_LOCAL(
CustomCountHistogram, pagehide_histogram,
("DocumentEventTiming.PageHideDuration", 0, 10000000, 50));
pagehide_histogram.CountMicroseconds(pagehide_event_end -
pagehide_event_start);
}
if (!dom_window_)
return;

// This must be queried before |load_event_progress_| is changed to
// kUnloadVisibilityChangeInProgress because that would change the result.
bool page_visible = IsPageVisible();
load_event_progress_ = kUnloadVisibilityChangeInProgress;
if (page_visible) {
// Dispatch visibilitychange event, but don't bother doing
// other notifications as we're about to be unloaded.
const base::TimeTicks pagevisibility_hidden_event_start =
base::TimeTicks::Now();
DispatchEvent(*Event::CreateBubble(event_type_names::kVisibilitychange));
const base::TimeTicks pagevisibility_hidden_event_end =
base::TimeTicks::Now();
DEFINE_STATIC_LOCAL(
CustomCountHistogram, pagevisibility_histogram,
("DocumentEventTiming.PageVibilityHiddenDuration", 0, 10000000, 50));
pagevisibility_histogram.CountMicroseconds(
pagevisibility_hidden_event_end - pagevisibility_hidden_event_start);
DispatchEvent(
*Event::CreateBubble(event_type_names::kWebkitvisibilitychange));
}
if (!dom_window_)
return;

GetFrame()->Loader().SaveScrollAnchor();

load_event_progress_ = kUnloadEventInProgress;
Event& unload_event = *Event::Create(event_type_names::kUnload);
const base::TimeTicks unload_event_start = base::TimeTicks::Now();
dom_window_->DispatchEvent(unload_event, this);
const base::TimeTicks unload_event_end = base::TimeTicks::Now();

if (unload_timing) {
// Record unload event timing when navigating cross-document.
DEFINE_STATIC_LOCAL(
CustomCountHistogram, unload_histogram,
("DocumentEventTiming.UnloadDuration", 0, 10000000, 50));
unload_histogram.CountMicroseconds(unload_event_end - unload_event_start);

// Fill in the unload timing if the new document origin has access to
// them.
if (committing_origin->CanRequest(Url())) {
auto& timing = unload_timing->emplace();
timing.unload_event_start = unload_event_start;
timing.unload_event_end = unload_event_end;
}
}
load_event_progress_ = kUnloadEventHandled;
}

void Document::DispatchFreezeEvent() {
Expand Down
5 changes: 5 additions & 0 deletions blink/renderer/core/page/page.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -558,6 +558,11 @@ bool Page::DispatchedPagehideAndStillHidden() {
mojom::blink::PagehideDispatch::kNotDispatched;
}

bool Page::DispatchedPagehidePersistedAndStillHidden() {
return lifecycle_state_->pagehide_dispatch ==
mojom::blink::PagehideDispatch::kDispatchedPersisted;
}

void Page::OnSetPageFrozen(bool frozen) {
if (frozen_ == frozen)
return;
Expand Down
4 changes: 4 additions & 0 deletions blink/renderer/core/page/page.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,10 @@ class CORE_EXPORT Page final : public GarbageCollected<Page>,
// still hidden (possibly preserved in the back-forward cache, or unloaded).
bool DispatchedPagehideAndStillHidden();

// Similar to above, but will only return true if we've dispatched 'pagehide'
// with the 'persisted' property set to 'true'.
bool DispatchedPagehidePersistedAndStillHidden();

static void PrepareForLeakDetection();

private:
Expand Down

0 comments on commit 0a6a98d

Please sign in to comment.