New issue
Advanced search Search tips

Issue 854598 link

Starred by 2 users

Issue metadata

Status: Started
Owner:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocked on:
issue 857472
issue 857471



Sign in to add a comment

Late-arriving PageSignalGenerator events mishandled

Project Member Reported by siggi@chromium.org, Jun 20 2018

Issue description

As-is, late-arriving PSG events can be interpreted and handled against a WebContents (et al) that has been re-navigated. This can lead to premature state transitions in the TabLoadTracker for the re-navigated WC, and may cause performance and characteristics data to be persisted against the wrong origin.

The fix is to
- give navigations an identity, and to
- adorn all PSG notifications with the associated navigation identity, and to
- match arriving events against the current navigation identity, and to
- drop events that don't match the current navigation.

For the persistence case, it might be of short term benefit to funnel the origin through as well, as that will allow persisting data even post re-navigation. The long-term plan is probably to push the persistence into the RC sequence by endowing the graph with a suitable writer on navigation commit.
 
I can take care of fixing the non-persistent notification signal. To properly fix this I'll need to funnel the origin and the tab visibility, does it make sense?

Another way to fix this would be to not use the Mojo interface for this and to send a message via IPC from RenderFrameImpl to the corresponding WebContentsImpl (it's how we get notified for all the other feature usage events)

Comment 2 by siggi@chromium.org, Jun 20 2018

Status: Started (was: Assigned)
I have a CL in the works to plumb the data around.
Project Member

Comment 3 by bugdroid1@chromium.org, Jun 26 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f86540e8551b98c3d0cb859bb8547dc463e9ea8e

commit f86540e8551b98c3d0cb859bb8547dc463e9ea8e
Author: Sigurdur Asgeirsson <siggi@chromium.org>
Date: Tue Jun 26 19:27:29 2018

RC: Switch PSG notifications to use a PageNavigationIdentifier.

By propagating the unique ID of the navigation handle and the url
of the navigation, it's now possible to distinguish whether
notifications pertain to the current navigation or an earlier one.
If data relating to the notification is to be persisted, it's now
also possible to persist it against the correct (original) origin
the event pertains to.

Bug: 854598
Change-Id: I1b8687bd1153890edc995adbebac9b6ceb78acfe
Reviewed-on: https://chromium-review.googlesource.com/1108361
Commit-Queue: Sigurður Ásgeirsson <siggi@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Reviewed-by: Will Harris <wfh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570492}
[modify] https://crrev.com/f86540e8551b98c3d0cb859bb8547dc463e9ea8e/chrome/browser/resource_coordinator/page_signal_receiver.cc
[modify] https://crrev.com/f86540e8551b98c3d0cb859bb8547dc463e9ea8e/chrome/browser/resource_coordinator/page_signal_receiver.h
[modify] https://crrev.com/f86540e8551b98c3d0cb859bb8547dc463e9ea8e/chrome/browser/resource_coordinator/page_signal_receiver_unittest.cc
[modify] https://crrev.com/f86540e8551b98c3d0cb859bb8547dc463e9ea8e/chrome/browser/resource_coordinator/tab_helper.cc
[modify] https://crrev.com/f86540e8551b98c3d0cb859bb8547dc463e9ea8e/services/resource_coordinator/coordination_unit/page_coordination_unit_impl.cc
[modify] https://crrev.com/f86540e8551b98c3d0cb859bb8547dc463e9ea8e/services/resource_coordinator/coordination_unit/page_coordination_unit_impl.h
[modify] https://crrev.com/f86540e8551b98c3d0cb859bb8547dc463e9ea8e/services/resource_coordinator/coordination_unit/page_coordination_unit_impl_unittest.cc
[modify] https://crrev.com/f86540e8551b98c3d0cb859bb8547dc463e9ea8e/services/resource_coordinator/observers/ipc_volume_reporter_unittest.cc
[modify] https://crrev.com/f86540e8551b98c3d0cb859bb8547dc463e9ea8e/services/resource_coordinator/observers/metrics_collector_unittest.cc
[modify] https://crrev.com/f86540e8551b98c3d0cb859bb8547dc463e9ea8e/services/resource_coordinator/observers/page_signal_generator_impl.cc
[modify] https://crrev.com/f86540e8551b98c3d0cb859bb8547dc463e9ea8e/services/resource_coordinator/observers/page_signal_generator_impl.h
[modify] https://crrev.com/f86540e8551b98c3d0cb859bb8547dc463e9ea8e/services/resource_coordinator/observers/page_signal_generator_impl_unittest.cc
[modify] https://crrev.com/f86540e8551b98c3d0cb859bb8547dc463e9ea8e/services/resource_coordinator/public/cpp/coordination_unit.typemap
[modify] https://crrev.com/f86540e8551b98c3d0cb859bb8547dc463e9ea8e/services/resource_coordinator/public/cpp/coordination_unit_mojom_traits.cc
[modify] https://crrev.com/f86540e8551b98c3d0cb859bb8547dc463e9ea8e/services/resource_coordinator/public/cpp/coordination_unit_mojom_traits.h
[add] https://crrev.com/f86540e8551b98c3d0cb859bb8547dc463e9ea8e/services/resource_coordinator/public/cpp/page_navigation_identity.h
[modify] https://crrev.com/f86540e8551b98c3d0cb859bb8547dc463e9ea8e/services/resource_coordinator/public/cpp/page_resource_coordinator.cc
[modify] https://crrev.com/f86540e8551b98c3d0cb859bb8547dc463e9ea8e/services/resource_coordinator/public/cpp/page_resource_coordinator.h
[modify] https://crrev.com/f86540e8551b98c3d0cb859bb8547dc463e9ea8e/services/resource_coordinator/public/mojom/coordination_unit.mojom
[modify] https://crrev.com/f86540e8551b98c3d0cb859bb8547dc463e9ea8e/services/resource_coordinator/public/mojom/page_signal.mojom

Project Member

Comment 4 by bugdroid1@chromium.org, Jun 27 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/def824e91db16ba95375cd0a98813607c3630802

commit def824e91db16ba95375cd0a98813607c3630802
Author: Sigurdur Asgeirsson <siggi@chromium.org>
Date: Wed Jun 27 20:32:03 2018

RC: Push PageNavigationID through PageSignalReceiver.

Also maintain a map of WebContents to the latest navigation ID in the
PageSignalReceiver. This allows observers to filter out late
notifications.
Update a couple of observers that were mis-processing late
notifications.

Bug: 854598
Change-Id: Idac49a675e306ae6d79c74b7c658c8393d8aefad
Reviewed-on: https://chromium-review.googlesource.com/1117128
Commit-Queue: Sigurður Ásgeirsson <siggi@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570886}
[modify] https://crrev.com/def824e91db16ba95375cd0a98813607c3630802/chrome/browser/resource_coordinator/local_site_characteristics_webcontents_observer.cc
[modify] https://crrev.com/def824e91db16ba95375cd0a98813607c3630802/chrome/browser/resource_coordinator/local_site_characteristics_webcontents_observer.h
[modify] https://crrev.com/def824e91db16ba95375cd0a98813607c3630802/chrome/browser/resource_coordinator/local_site_characteristics_webcontents_observer_unittest.cc
[modify] https://crrev.com/def824e91db16ba95375cd0a98813607c3630802/chrome/browser/resource_coordinator/page_signal_receiver.cc
[modify] https://crrev.com/def824e91db16ba95375cd0a98813607c3630802/chrome/browser/resource_coordinator/page_signal_receiver.h
[modify] https://crrev.com/def824e91db16ba95375cd0a98813607c3630802/chrome/browser/resource_coordinator/page_signal_receiver_unittest.cc
[modify] https://crrev.com/def824e91db16ba95375cd0a98813607c3630802/chrome/browser/resource_coordinator/tab_helper.cc
[modify] https://crrev.com/def824e91db16ba95375cd0a98813607c3630802/chrome/browser/resource_coordinator/tab_lifecycle_unit_source.cc
[modify] https://crrev.com/def824e91db16ba95375cd0a98813607c3630802/chrome/browser/resource_coordinator/tab_lifecycle_unit_source.h
[modify] https://crrev.com/def824e91db16ba95375cd0a98813607c3630802/chrome/browser/resource_coordinator/tab_manager_resource_coordinator_signal_observer.cc
[modify] https://crrev.com/def824e91db16ba95375cd0a98813607c3630802/chrome/browser/resource_coordinator/tab_manager_resource_coordinator_signal_observer.h
[modify] https://crrev.com/def824e91db16ba95375cd0a98813607c3630802/chrome/browser/ui/bloated_renderer/bloated_renderer_tab_helper.cc
[modify] https://crrev.com/def824e91db16ba95375cd0a98813607c3630802/chrome/browser/ui/bloated_renderer/bloated_renderer_tab_helper.h
[modify] https://crrev.com/def824e91db16ba95375cd0a98813607c3630802/chrome/browser/ui/bloated_renderer/bloated_renderer_tab_helper_browsertest.cc
[modify] https://crrev.com/def824e91db16ba95375cd0a98813607c3630802/services/resource_coordinator/public/cpp/coordination_unit_mojom_traits.h
[modify] https://crrev.com/def824e91db16ba95375cd0a98813607c3630802/services/resource_coordinator/public/cpp/page_navigation_identity.h

Comment 5 by siggi@chromium.org, Jun 28 2018

Blockedon: 857471

Comment 6 by siggi@chromium.org, Jun 28 2018

Blockedon: 857472
Project Member

Comment 7 by bugdroid1@chromium.org, Jun 28 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/49a048bd18eb94e30cf51b6e5db734da6964b710

commit 49a048bd18eb94e30cf51b6e5db734da6964b710
Author: Sigurdur Asgeirsson <siggi@chromium.org>
Date: Thu Jun 28 17:07:55 2018

RC: Tiny cleanup in TabLoadTracker.

Bug: 854598
Change-Id: I457e8940a4ad5ff4aa89cfae4415f0addd1e6a64
Reviewed-on: https://chromium-review.googlesource.com/1118621
Commit-Queue: François Doray <fdoray@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571171}
[modify] https://crrev.com/49a048bd18eb94e30cf51b6e5db734da6964b710/chrome/browser/resource_coordinator/tab_load_tracker.cc

Owner: fdoray@chromium.org
Assigning over to Francois, as I don't think there's any work left here but the blocker he owns.

Sign in to add a comment