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}

chrome/browser/resource_coordinator/local_site_characteristics_webcontents_observer.cc

@@ -179,11 +179,15 @@ void LocalSiteCharacteristicsWebContentsObserver::OnLoadingStateChange(
 }
 
 void LocalSiteCharacteristicsWebContentsObserver::
-    OnNonPersistentNotificationCreated(content::WebContents* contents) {
+    OnNonPersistentNotificationCreated(
+        content::WebContents* contents,
+        const PageNavigationIdentity& page_navigation_id) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   if (web_contents() != contents)
     return;
 
+  // TODO(sebmarchand): The URL from the page_navigation_id wants to be the
+  //     base of the origin this is saved against.
   MaybeNotifyBackgroundFeatureUsage(
       &SiteCharacteristicsDataWriter::NotifyUsesNotificationsInBackground);
 }

chrome/browser/resource_coordinator/local_site_characteristics_webcontents_observer.h

@@ -54,7 +54,8 @@ class LocalSiteCharacteristicsWebContentsObserver
 
   // PageSignalObserver:
   void OnNonPersistentNotificationCreated(
-      content::WebContents* web_contents) override;
+      content::WebContents* web_contents,
+      const PageNavigationIdentity& page_navigation_id) override;
 
   SiteCharacteristicsDataWriter* GetWriterForTesting() const {
     return writer_.get();

chrome/browser/resource_coordinator/local_site_characteristics_webcontents_observer_unittest.cc

@@ -120,6 +120,7 @@ class LocalSiteCharacteristicsWebContentsObserverTest
 
   const GURL kTestUrl1 = GURL("http://foo.com");
   const GURL kTestUrl2 = GURL("http://bar.com");
+  const PageNavigationIdentity kNavId = {CoordinationUnitID(), 0, ""};
 
   LocalSiteCharacteristicsWebContentsObserver* observer() {
     return observer_.get();
@@ -191,14 +192,13 @@ TEST_F(LocalSiteCharacteristicsWebContentsObserverTest,
 
   // Test that the feature usage events get forwarded to the writer when the
   // tab is in background.
-
   observer()->DidUpdateFaviconURL({});
   ::testing::Mock::VerifyAndClear(mock_writer);
   observer()->TitleWasSet(nullptr);
   ::testing::Mock::VerifyAndClear(mock_writer);
   observer()->OnAudioStateChanged(true);
   ::testing::Mock::VerifyAndClear(mock_writer);
-  observer()->OnNonPersistentNotificationCreated(web_contents());
+  observer()->OnNonPersistentNotificationCreated(web_contents(), kNavId);
   ::testing::Mock::VerifyAndClear(mock_writer);
 
   EXPECT_CALL(*mock_writer,
@@ -216,7 +216,7 @@ TEST_F(LocalSiteCharacteristicsWebContentsObserverTest,
   observer()->OnAudioStateChanged(true);
   ::testing::Mock::VerifyAndClear(mock_writer);
   EXPECT_CALL(*mock_writer, NotifyUsesNotificationsInBackground());
-  observer()->OnNonPersistentNotificationCreated(web_contents());
+  observer()->OnNonPersistentNotificationCreated(web_contents(), kNavId);
   ::testing::Mock::VerifyAndClear(mock_writer);
 
   EXPECT_CALL(*mock_writer, OnDestroy());
@@ -245,7 +245,7 @@ TEST_F(LocalSiteCharacteristicsWebContentsObserverTest,
   ::testing::Mock::VerifyAndClear(mock_writer);
   observer()->OnAudioStateChanged(true);
   ::testing::Mock::VerifyAndClear(mock_writer);
-  observer()->OnNonPersistentNotificationCreated(web_contents());
+  observer()->OnNonPersistentNotificationCreated(web_contents(), kNavId);
   ::testing::Mock::VerifyAndClear(mock_writer);
 
   EXPECT_CALL(*mock_writer, OnDestroy());

chrome/browser/resource_coordinator/page_signal_receiver.cc

@@ -76,8 +76,7 @@ void PageSignalReceiver::OnLoadTimePerformanceEstimate(
     uint64_t private_footprint_kb_estimate) {
   NotifyObserversIfKnownCu(page_navigation_id,
                            &PageSignalObserver::OnLoadTimePerformanceEstimate,
-                           page_navigation_id.url, cpu_usage_estimate,
-                           private_footprint_kb_estimate);
+                           cpu_usage_estimate, private_footprint_kb_estimate);
 }
 
 void PageSignalReceiver::AddObserver(PageSignalObserver* observer) {
@@ -112,9 +111,47 @@ void PageSignalReceiver::AssociateCoordinationUnitIDWithWebContents(
   cu_id_web_contents_map_[cu_id] = web_contents;
 }
 
+void PageSignalReceiver::SetNavigationID(content::WebContents* web_contents,
+                                         int64_t navigation_id) {
+  DCHECK_NE(nullptr, web_contents);
+  web_contents_navigation_id_map_[web_contents] = navigation_id;
+}
+
 void PageSignalReceiver::RemoveCoordinationUnitID(
     const CoordinationUnitID& cu_id) {
-  cu_id_web_contents_map_.erase(cu_id);
+  auto it = cu_id_web_contents_map_.find(cu_id);
+  DCHECK(it != cu_id_web_contents_map_.end());
+
+  web_contents_navigation_id_map_.erase(it->second);
+  cu_id_web_contents_map_.erase(it);
+}
+
+int64_t PageSignalReceiver::GetNavigationIDForWebContents(
+    content::WebContents* web_contents) {
+  DCHECK_NE(nullptr, web_contents);
+  auto it = web_contents_navigation_id_map_.find(web_contents);
+  if (it == web_contents_navigation_id_map_.end())
+    return 0;
+
+  return it->second;
+}
+
+template <typename Method, typename... Params>
+void PageSignalReceiver::NotifyObserversIfKnownCu(
+    const PageNavigationIdentity& page_navigation_id,
+    Method m,
+    Params... params) {
+  auto web_contents_iter =
+      cu_id_web_contents_map_.find(page_navigation_id.page_cu_id);
+  if (web_contents_iter == cu_id_web_contents_map_.end())
+    return;
+  // An observer can make web_contents_iter invalid by mutating
+  // the cu_id_web_contents_map_.
+  content::WebContents* web_contents = web_contents_iter->second;
+  for (auto& observer : observers_) {
+    (observer.*m)(web_contents, page_navigation_id,
+                  std::forward<Params>(params)...);
+  }
 }
 
 }  // namespace resource_coordinator

chrome/browser/resource_coordinator/page_signal_receiver.h

@@ -25,21 +25,33 @@ class PageSignalObserver {
   // PageSignalReceiver will deliver signals with a |web_contents| even it's not
   // managed by the client. Thus the clients are responsible for checking the
   // passed |web_contents| by themselves.
-  virtual void OnPageAlmostIdle(content::WebContents* web_contents) {}
-  virtual void OnRendererIsBloated(content::WebContents* web_contents) {}
+  // Also note that because these event notifications are asynchronous to
+  // navigation, it's possible that |web_contents| has navigated to another site
+  // by the time the notification arrives.
+  // The |page_navigation_id| parameter allows comparing the navigation_id
+  // against the current navigation handle of |web_contents|, and the url
+  // in the struct can be used post-navigation to rendezvous to the origin
+  // the event relates to.
+  virtual void OnPageAlmostIdle(
+      content::WebContents* web_contents,
+      const PageNavigationIdentity& page_navigation_id) {}
+  virtual void OnRendererIsBloated(
+      content::WebContents* web_contents,
+      const PageNavigationIdentity& page_navigation_id) {}
   virtual void OnExpectedTaskQueueingDurationSet(
       content::WebContents* web_contents,
+      const PageNavigationIdentity& page_navigation_id,
       base::TimeDelta duration) {}
-  virtual void OnLifecycleStateChanged(content::WebContents* web_contents,
-                                       mojom::LifecycleState state) {}
+  virtual void OnLifecycleStateChanged(
+      content::WebContents* web_contents,
+      const PageNavigationIdentity& page_navigation_id,
+      mojom::LifecycleState state) {}
   virtual void OnNonPersistentNotificationCreated(
-      content::WebContents* web_contents) {}
-  // Note that because performance measurement is asynchronous to navigation,
-  // it's possible that |web_contents| has navigated to another site by the time
-  // this notification arrives - hence the |site| parameter.
+      content::WebContents* web_contents,
+      const PageNavigationIdentity& page_navigation_id) {}
   virtual void OnLoadTimePerformanceEstimate(
       content::WebContents* web_contents,
-      const std::string& url,
+      const PageNavigationIdentity& page_navigation_id,
       base::TimeDelta cpu_usage_estimate,
       uint64_t private_footprint_kb_estimate) {}
 };
@@ -82,9 +94,14 @@ class PageSignalReceiver : public mojom::PageSignalReceiver {
   void AssociateCoordinationUnitIDWithWebContents(
       const CoordinationUnitID& cu_id,
       content::WebContents* web_contents);
-
+  void SetNavigationID(content::WebContents* web_contents,
+                       int64_t navigation_id);
   void RemoveCoordinationUnitID(const CoordinationUnitID& cu_id);
 
+  // Retrieves the unique ID of the last navigation observed for |web_contents|
+  // or 0 if no navigation has been observed yet.
+  int64_t GetNavigationIDForWebContents(content::WebContents* web_contents);
+
  private:
   FRIEND_TEST_ALL_PREFIXES(PageSignalReceiverUnitTest,
                            NotifyObserversThatCanRemoveCoordinationUnitID);
@@ -92,20 +109,11 @@ class PageSignalReceiver : public mojom::PageSignalReceiver {
   void NotifyObserversIfKnownCu(
       const PageNavigationIdentity& page_navigation_id,
       Method m,
-      Params... params) {
-    auto web_contents_iter =
-        cu_id_web_contents_map_.find(page_navigation_id.page_cu_id);
-    if (web_contents_iter == cu_id_web_contents_map_.end())
-      return;
-    // An observer can make web_contents_iter invalid by mutating
-    // the cu_id_web_contents_map_.
-    content::WebContents* web_contents = web_contents_iter->second;
-    for (auto& observer : observers_)
-      (observer.*m)(web_contents, std::forward<Params>(params)...);
-  }
+      Params... params);
 
   mojo::Binding<mojom::PageSignalReceiver> binding_;
   std::map<CoordinationUnitID, content::WebContents*> cu_id_web_contents_map_;
+  std::map<content::WebContents*, int64_t> web_contents_navigation_id_map_;
   base::ObserverList<PageSignalObserver> observers_;
   DISALLOW_COPY_AND_ASSIGN(PageSignalReceiver);
 };

chrome/browser/resource_coordinator/page_signal_receiver_unittest.cc

@@ -45,6 +45,7 @@ class TestPageSignalObserver : public PageSignalObserver {
   }
   // PageSignalObserver:
   void OnLifecycleStateChanged(content::WebContents* contents,
+                               const PageNavigationIdentity& page_navigation_id,
                                mojom::LifecycleState state) override {
     if (action_ == Action::kRemoveCoordinationUnitID)
       page_signal_receiver_->RemoveCoordinationUnitID(page_cu_id_);
@@ -104,4 +105,26 @@ TEST_F(PageSignalReceiverUnitTest, ConstructMojoChannelOnce) {
   content::ServiceManagerConnection::DestroyForProcess();
 }
 
+TEST_F(PageSignalReceiverUnitTest, GetNavigationIDForWebContents) {
+  page_signal_receiver_->AssociateCoordinationUnitIDWithWebContents(
+      page_cu_id_, web_contents());
+
+  // Starts unset.
+  EXPECT_EQ(
+      0, page_signal_receiver_->GetNavigationIDForWebContents(web_contents()));
+
+  // Follows updates.
+  page_signal_receiver_->SetNavigationID(web_contents(), 10);
+  EXPECT_EQ(
+      10, page_signal_receiver_->GetNavigationIDForWebContents(web_contents()));
+  page_signal_receiver_->SetNavigationID(web_contents(), 11);
+  EXPECT_EQ(
+      11, page_signal_receiver_->GetNavigationIDForWebContents(web_contents()));
+
+  // Unset on removal.
+  page_signal_receiver_->RemoveCoordinationUnitID(page_cu_id_);
+  EXPECT_EQ(
+      0, page_signal_receiver_->GetNavigationIDForWebContents(web_contents()));
+}
+
 }  // namespace resource_coordinator

chrome/browser/resource_coordinator/tab_helper.cc

@@ -131,10 +131,9 @@ void ResourceCoordinatorTabHelper::DidFinishNavigation(
     return;
   }
 
-  content::RenderFrameHost* render_frame_host =
-      navigation_handle->GetRenderFrameHost();
-
   if (page_resource_coordinator_) {
+    content::RenderFrameHost* render_frame_host =
+        navigation_handle->GetRenderFrameHost();
     // Make sure the hierarchical structure is constructed before sending signal
     // to Resource Coordinator.
     auto* frame_resource_coordinator =
@@ -146,6 +145,13 @@ void ResourceCoordinatorTabHelper::DidFinishNavigation(
     process_resource_coordinator->AddFrame(*frame_resource_coordinator);
 
     if (navigation_handle->IsInMainFrame()) {
+      if (auto* page_signal_receiver =
+              resource_coordinator::PageSignalReceiver::GetInstance()) {
+        // Update the last observed navigation ID for this WebContents.
+        page_signal_receiver->SetNavigationID(
+            web_contents(), navigation_handle->GetNavigationId());
+      }
+
       UpdateUkmRecorder(navigation_handle->GetNavigationId());
       ResetFlag();
       page_resource_coordinator_->OnMainFrameNavigationCommitted(

chrome/browser/resource_coordinator/tab_lifecycle_unit_source.cc

@@ -220,11 +220,13 @@ void TabLifecycleUnitSource::OnBrowserNoLongerActive(Browser* browser) {
 
 void TabLifecycleUnitSource::OnLifecycleStateChanged(
     content::WebContents* web_contents,
+    const PageNavigationIdentity& page_navigation_id,
     mojom::LifecycleState state) {
   TabLifecycleUnit* lifecycle_unit = GetTabLifecycleUnit(web_contents);
 
   // Some WebContents aren't attached to a tab, so there is no corresponding
   // TabLifecycleUnit.
+  // TODO(fdoray): This may want to filter for the navigation_id.
   if (lifecycle_unit)
     lifecycle_unit->UpdateLifecycleState(state);
 }

chrome/browser/resource_coordinator/tab_lifecycle_unit_source.h

@@ -113,6 +113,7 @@ class TabLifecycleUnitSource : public BrowserListObserver,
 
   // PageSignalObserver:
   void OnLifecycleStateChanged(content::WebContents* web_contents,
+                               const PageNavigationIdentity& page_navigation_id,
                                mojom::LifecycleState state) override;
 
   // Tracks the BrowserList and all TabStripModels.

chrome/browser/resource_coordinator/tab_manager_resource_coordinator_signal_observer.cc

@@ -35,30 +35,53 @@ TabManager::ResourceCoordinatorSignalObserver::
 }
 
 void TabManager::ResourceCoordinatorSignalObserver::OnPageAlmostIdle(
-    content::WebContents* web_contents) {
-  TabManagerResourceCoordinatorSignalObserverHelper::OnPageAlmostIdle(
-      web_contents);
+    content::WebContents* web_contents,
+    const PageNavigationIdentity& page_navigation_id) {
+  auto* page_signal_receiver = PageSignalReceiver::GetInstance();
+  DCHECK_NE(nullptr, page_signal_receiver);
+
+  // Only dispatch the event if it pertains to the current navigation.
+  if (page_signal_receiver->GetNavigationIDForWebContents(web_contents) ==
+      page_navigation_id.navigation_id) {
+    TabManagerResourceCoordinatorSignalObserverHelper::OnPageAlmostIdle(
+        web_contents);
+  }
 }
 
 void TabManager::ResourceCoordinatorSignalObserver::
-    OnExpectedTaskQueueingDurationSet(content::WebContents* web_contents,
-                                      base::TimeDelta duration) {
+    OnExpectedTaskQueueingDurationSet(
+        content::WebContents* web_contents,
+        const PageNavigationIdentity& page_navigation_id,
+        base::TimeDelta duration) {
+  auto* page_signal_receiver = PageSignalReceiver::GetInstance();
+  DCHECK_NE(nullptr, page_signal_receiver);
+
+  if (page_signal_receiver->GetNavigationIDForWebContents(web_contents) !=
+      page_navigation_id.navigation_id) {
+    // |web_contents| has been re-navigated, drop this notification rather than
+    // recording it against the wrong origin.
+    return;
+  }
+
   g_browser_process->GetTabManager()
       ->stats_collector()
       ->RecordExpectedTaskQueueingDuration(web_contents, duration);
 }
 
 void TabManager::ResourceCoordinatorSignalObserver::
-    OnNonPersistentNotificationCreated(content::WebContents* web_contents) {
+    OnNonPersistentNotificationCreated(
+        content::WebContents* web_contents,
+        const PageNavigationIdentity& page_navigation_id) {
   // TODO(sebmarchand): Add the wiring to forward this signal where it should be
   // used.
 }
 
 void TabManager::ResourceCoordinatorSignalObserver::
-    OnLoadTimePerformanceEstimate(content::WebContents* web_contents,
-                                  const std::string& url,
-                                  base::TimeDelta cpu_usage_estimate,
-                                  uint64_t private_footprint_kb_estimate) {
+    OnLoadTimePerformanceEstimate(
+        content::WebContents* web_contents,
+        const PageNavigationIdentity& page_navigation_id,
+        base::TimeDelta cpu_usage_estimate,
+        uint64_t private_footprint_kb_estimate) {
   // TODO(siggi): Persist the measurement associated to |url|'s site.
 }
 

chrome/browser/resource_coordinator/tab_manager_resource_coordinator_signal_observer.h

@@ -22,14 +22,19 @@ class TabManager::ResourceCoordinatorSignalObserver
   ~ResourceCoordinatorSignalObserver() override;
 
   // PageSignalObserver implementation.
-  void OnPageAlmostIdle(content::WebContents* web_contents) override;
-  void OnExpectedTaskQueueingDurationSet(content::WebContents* web_contents,
-                                         base::TimeDelta duration) override;
+  void OnPageAlmostIdle(
+      content::WebContents* web_contents,
+      const PageNavigationIdentity& page_navigation_id) override;
+  void OnExpectedTaskQueueingDurationSet(
+      content::WebContents* web_contents,
+      const PageNavigationIdentity& page_navigation_id,
+      base::TimeDelta duration) override;
   void OnNonPersistentNotificationCreated(
-      content::WebContents* web_contents) override;
+      content::WebContents* web_contents,
+      const PageNavigationIdentity& page_navigation_id) override;
   void OnLoadTimePerformanceEstimate(
       content::WebContents* web_contents,
-      const std::string& url,
+      const PageNavigationIdentity& page_navigation_id,
       base::TimeDelta cpu_usage_estimate,
       uint64_t private_footprint_kb_estimate) override;
 

chrome/browser/ui/bloated_renderer/bloated_renderer_tab_helper.cc

@@ -93,11 +93,21 @@ bool BloatedRendererTabHelper::CanReloadBloatedTab() {
 }
 
 void BloatedRendererTabHelper::OnRendererIsBloated(
-    content::WebContents* bloated_web_contents) {
+    content::WebContents* bloated_web_contents,
+    const resource_coordinator::PageNavigationIdentity& page_navigation_id) {
   if (web_contents() != bloated_web_contents) {
     // Ignore if the notification is about a different tab.
     return;
   }
+  auto* page_signal_receiver =
+      resource_coordinator::PageSignalReceiver::GetInstance();
+  DCHECK_NE(nullptr, page_signal_receiver);
+  if (page_navigation_id.navigation_id !=
+      page_signal_receiver->GetNavigationIDForWebContents(web_contents())) {
+    // Ignore if the notification is pursuant to an earlier navigation.
+    return;
+  }
+
   if (CanReloadBloatedTab()) {
     const size_t expected_page_count = 1u;
     const bool skip_unload_handlers = true;

chrome/browser/ui/bloated_renderer/bloated_renderer_tab_helper.h

@@ -33,7 +33,9 @@ class BloatedRendererTabHelper
   void WebContentsDestroyed() override;
 
   // resource_coordinator::PageSignalObserver:
-  void OnRendererIsBloated(content::WebContents* web_contents) override;
+  void OnRendererIsBloated(content::WebContents* web_contents,
+                           const resource_coordinator::PageNavigationIdentity&
+                               page_navigation_id) override;
 
   static void ShowInfoBar(InfoBarService* infobar_service);
 

chrome/browser/ui/bloated_renderer/bloated_renderer_tab_helper_browsertest.cc

@@ -37,8 +37,10 @@ IN_PROC_BROWSER_TEST_F(BloatedRendererTabHelperBrowserTest,
   InfoBarService* infobar_service =
       InfoBarService::FromWebContents(web_contents);
   EXPECT_EQ(0u, infobar_service->infobar_count());
+
+  resource_coordinator::PageNavigationIdentity page_id;
   BloatedRendererTabHelper::FromWebContents(web_contents)
-      ->OnRendererIsBloated(web_contents);
+      ->OnRendererIsBloated(web_contents, page_id);
   reload.Wait();
   EXPECT_EQ(1u, infobar_service->infobar_count());
 }

services/resource_coordinator/public/cpp/coordination_unit_mojom_traits.h

@@ -47,7 +47,7 @@ struct COMPONENT_EXPORT(RESOURCE_COORDINATOR_PUBLIC_MOJOM)
       const resource_coordinator::PageNavigationIdentity& id) {
     return id.page_cu_id;
   }
-  static uint64_t navigation_id(
+  static int64_t navigation_id(
       const resource_coordinator::PageNavigationIdentity& id) {
     return id.navigation_id;
   }

services/resource_coordinator/public/cpp/page_navigation_identity.h

@@ -16,7 +16,7 @@ struct PageNavigationIdentity {
   CoordinationUnitID page_cu_id;
   // The unique ID of the NavigationHandle of the page at the time the event
   // relates to.
-  uint64_t navigation_id;
+  int64_t navigation_id;
   // The URL of the last navigation.
   std::string url;
 };