[PM] Fix hanging throttles in TabLoadingFrameNavigation feature.

The bug was as follows:
- mechanism binds to NavigationHandle, storing its NavigationId
- main frame does a subsequent same-document navigation
- PMTabHelper observes this navigation, and updates the internally
  stored NavigationId.
- PM dispatches policy message to mechanism to terminate throttles.
- Policy message looks up WCProxy::LastNavigationId (taken from
  PMTabHelper), and feeds that into the mechanism.
- This doesn't match the original NavId stored in the mechanism, and
  the policy message is dispatched.

Fix:

- Have PMTabHelper/WCProxy independently track same-doc and new-doc
  navigation IDs. Use the last new-doc navigation ID with the policy
  message.

BUG=1084555

Change-Id: I726ace3e06ca83cff99b0dec18372647789910c2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2218228Reviewed-by: Chris Hamilton <chrisha@chromium.org>
Reviewed-by: Sigurður Ásgeirsson <siggi@chromium.org>
Commit-Queue: Chris Hamilton <chrisha@chromium.org>
Cr-Commit-Position: refs/heads/master@{#772318}

components/performance_manager/graph/policies/tab_loading_frame_navigation_policy.cc

@@ -345,7 +345,8 @@ void TabLoadingFrameNavigationPolicy::StopThrottlingExpiredPages() {
                                     auto* contents = proxy.Get();
                                     if (contents)
                                       mechanism->StopThrottling(
-                                          contents, proxy.LastNavigationId());
+                                          contents,
+                                          proxy.LastNewDocNavigationId());
                                   },
                                   base::Unretained(mechanism_),
                                   page_node->GetContentsProxy()));

components/performance_manager/graph/policies/tab_loading_frame_navigation_policy_unittest.cc

@@ -9,11 +9,13 @@
 #include "base/test/bind_test_util.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/test/task_environment.h"
+#include "components/performance_manager/performance_manager_tab_helper.h"
 #include "components/performance_manager/public/features.h"
 #include "components/performance_manager/public/performance_manager.h"
 #include "components/performance_manager/test_support/performance_manager_test_harness.h"
 #include "content/public/browser/browser_task_traits.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/mock_navigation_handle.h"
 #include "content/public/test/navigation_simulator.h"
@@ -152,7 +154,8 @@ class TabLoadingFrameNavigationPolicyTest
 
   // The MechanismDelegate calls redirect here.
   MOCK_METHOD1(OnSetThrottlingEnabled, void(bool));
-  MOCK_METHOD1(OnStopThrottling, void(content::WebContents*));
+  MOCK_METHOD2(OnStopThrottling,
+               void(content::WebContents*, int64_t last_navigation_id));
 
   // Accessors.
   TabLoadingFrameNavigationPolicy* policy() const { return policy_; }
@@ -175,8 +178,8 @@ class TabLoadingFrameNavigationPolicyTest
     quit_closure_.Run();
   }
   void StopThrottling(content::WebContents* contents,
-                      int64_t last_navigation_id_unused) override {
-    OnStopThrottling(contents);
+                      int64_t last_navigation_id) override {
+    OnStopThrottling(contents, last_navigation_id);
 
     // Time can be manually advanced as well, so we're not always in a RunLoop.
     // Only try to invoke the quit closure if it has been set.
@@ -190,6 +193,35 @@ class TabLoadingFrameNavigationPolicyTest
   base::TimeTicks start_;
 };
 
+// Navigates and commits from the browser, returning the navigation id.
+int64_t NavigateAndCommitFromBrowser(content::WebContents* contents,
+                                     GURL gurl) {
+  std::unique_ptr<content::NavigationSimulator> simulator(
+      content::NavigationSimulator::CreateBrowserInitiated(gurl, contents));
+  simulator->Start();
+  int64_t navigation_id = simulator->GetNavigationHandle()->GetNavigationId();
+  simulator->Commit();
+  auto* pmth = PerformanceManagerTabHelper::FromWebContents(contents);
+  CHECK(pmth);
+  EXPECT_EQ(pmth->LastNavigationId(), navigation_id);
+  EXPECT_EQ(pmth->LastNewDocNavigationId(), navigation_id);
+  return navigation_id;
+}
+
+// Navigates and commits a same document navigation from the renderer.
+// Returns the navigation id.
+int64_t NavigateAndCommitSameDocFromRenderer(content::WebContents* contents,
+                                             GURL gurl) {
+  std::unique_ptr<content::NavigationSimulator> simulator(
+      content::NavigationSimulator::CreateRendererInitiated(
+          gurl, contents->GetMainFrame()));
+  simulator->CommitSameDocument();
+  auto* pmth = PerformanceManagerTabHelper::FromWebContents(contents);
+  CHECK(pmth);
+  EXPECT_NE(pmth->LastNavigationId(), pmth->LastNewDocNavigationId());
+  return pmth->LastNavigationId();
+}
+
 }  // namespace
 
 TEST_F(TabLoadingFrameNavigationPolicyTest, OnlyHttpContentsThrottled) {
@@ -297,7 +329,7 @@ TEST_F(TabLoadingFrameNavigationPolicyTest, TimeoutWorks) {
 
   // Navigate and throttle a first contents. It will expire at time T.
   std::unique_ptr<content::WebContents> contents1 = CreateTestWebContents();
-  content::NavigationSimulator::NavigateAndCommitFromBrowser(
+  int64_t navigation_id = NavigateAndCommitFromBrowser(
       contents1.get(), GURL("http://www.foo1.com/"));
   EXPECT_TRUE(policy()->ShouldThrottleWebContents(contents1.get()));
   ExpectThrottledPageCount(1);
@@ -314,13 +346,12 @@ TEST_F(TabLoadingFrameNavigationPolicyTest, TimeoutWorks) {
 
   // Navigate and throttle a second contents. It will expire at 1.5 T.
   std::unique_ptr<content::WebContents> contents2 = CreateTestWebContents();
-  content::NavigationSimulator::NavigateAndCommitFromBrowser(
-      contents2.get(), GURL("https://www.foo2.com/"));
+  NavigateAndCommitFromBrowser(contents2.get(), GURL("https://www.foo2.com/"));
   EXPECT_TRUE(policy()->ShouldThrottleWebContents(contents2.get()));
   ExpectThrottledPageCount(2);
 
   // Run until the first contents times out.
-  EXPECT_CALL(*this, OnStopThrottling(contents1.get()));
+  EXPECT_CALL(*this, OnStopThrottling(contents1.get(), navigation_id));
   RunUntilStopThrottling();
   ExpectThrottledPageCount(1);
   base::TimeTicks stop1 = task_environment()->GetMockTickClock()->NowTicks();
@@ -331,8 +362,8 @@ TEST_F(TabLoadingFrameNavigationPolicyTest, TimeoutWorks) {
 
   // Navigate and throttle a third contents. It will expire at time 2 T.
   std::unique_ptr<content::WebContents> contents3 = CreateTestWebContents();
-  content::NavigationSimulator::NavigateAndCommitFromBrowser(
-      contents3.get(), GURL("https://www.foo3.com/"));
+  navigation_id = NavigateAndCommitFromBrowser(contents3.get(),
+                                               GURL("https://www.foo3.com/"));
   EXPECT_TRUE(policy()->ShouldThrottleWebContents(contents3.get()));
   ExpectThrottledPageCount(2);
 
@@ -346,6 +377,12 @@ TEST_F(TabLoadingFrameNavigationPolicyTest, TimeoutWorks) {
   // We are now at time 1.25 T.
   ASSERT_EQ(1.25, GetRelativeTime());
 
+  // Do a same document navigation. This will cause another navigation commit,
+  // but the policy should remain bound to the previous navigation id.
+  int64_t same_doc_navigation_id = NavigateAndCommitSameDocFromRenderer(
+      contents3.get(), GURL("https://www.foo3.com/#somehash"));
+  ASSERT_NE(navigation_id, same_doc_navigation_id);
+
   // Close the 2nd contents before the timeout expires, and expect the throttled
   // count to drop. Now the timer should be running for the 3rd contents.
   contents2.reset();
@@ -363,7 +400,7 @@ TEST_F(TabLoadingFrameNavigationPolicyTest, TimeoutWorks) {
   ASSERT_EQ(1.6, GetRelativeTime());
 
   // Finally, run until the third contents times out.
-  EXPECT_CALL(*this, OnStopThrottling(contents3.get()));
+  EXPECT_CALL(*this, OnStopThrottling(contents3.get(), navigation_id));
   RunUntilStopThrottling();
   ExpectThrottledPageCount(0);
   base::TimeTicks stop3 = task_environment()->GetMockTickClock()->NowTicks();

components/performance_manager/performance_manager_tab_helper.cc

@@ -280,7 +280,8 @@ void PerformanceManagerTabHelper::DidFinishNavigation(
 
   // Make sure the hierarchical structure is constructed before sending signal
   // to the performance manager.
-  OnMainFrameNavigation(navigation_handle->GetNavigationId());
+  OnMainFrameNavigation(navigation_handle->GetNavigationId(),
+                        navigation_handle->IsSameDocument());
   PerformanceManagerImpl::CallOnGraphImpl(
       FROM_HERE,
       base::BindOnce(
@@ -354,6 +355,10 @@ int64_t PerformanceManagerTabHelper::LastNavigationId() const {
   return last_navigation_id_;
 }
 
+int64_t PerformanceManagerTabHelper::LastNewDocNavigationId() const {
+  return last_new_doc_navigation_id_;
+}
+
 FrameNodeImpl* PerformanceManagerTabHelper::GetFrameNode(
     content::RenderFrameHost* render_frame_host) {
   auto it = frames_.find(render_frame_host);
@@ -368,8 +373,11 @@ void PerformanceManagerTabHelper::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
-void PerformanceManagerTabHelper::OnMainFrameNavigation(int64_t navigation_id) {
+void PerformanceManagerTabHelper::OnMainFrameNavigation(int64_t navigation_id,
+                                                        bool same_doc) {
   last_navigation_id_ = navigation_id;
+  if (!same_doc)
+    last_new_doc_navigation_id_ = navigation_id;
   ukm_source_id_ =
       ukm::ConvertToSourceId(navigation_id, ukm::SourceIdType::NAVIGATION_ID);
   PerformanceManagerImpl::CallOnGraphImpl(

components/performance_manager/performance_manager_tab_helper.h

@@ -76,6 +76,7 @@ class PerformanceManagerTabHelper
   // WebContentsProxyImpl overrides.
   content::WebContents* GetWebContents() const override;
   int64_t LastNavigationId() const override;
+  int64_t LastNewDocNavigationId() const override;
 
   void BindDocumentCoordinationUnit(
       content::RenderFrameHost* render_frame_host,
@@ -110,7 +111,7 @@ class PerformanceManagerTabHelper
   // PerformanceManagerRegistry.
   using WebContentsUserData<PerformanceManagerTabHelper>::CreateForWebContents;
 
-  void OnMainFrameNavigation(int64_t navigation_id);
+  void OnMainFrameNavigation(int64_t navigation_id, bool same_doc);
 
   std::unique_ptr<PageNodeImpl> page_node_;
   ukm::SourceId ukm_source_id_ = ukm::kInvalidSourceId;
@@ -125,6 +126,10 @@ class PerformanceManagerTabHelper
   // The last navigation ID that was committed to a main frame in this web
   // contents.
   int64_t last_navigation_id_ = 0;
+  // Similar to the above, but for the last non same-document navigation
+  // associated with this WebContents. This is always for a navigation that is
+  // older or equal to |last_navigation_id_|.
+  int64_t last_new_doc_navigation_id_ = 0;
 
   // Maps from RenderFrameHost to the associated PM node.
   std::map<content::RenderFrameHost*, std::unique_ptr<FrameNodeImpl>> frames_;

components/performance_manager/public/web_contents_proxy.h

@@ -42,6 +42,12 @@ class WebContentsProxy {
   // on the UI thread.
   int64_t LastNavigationId() const;
 
+  // Similar to the above, but for the last non same-document navigation
+  // associated with this WebContents. This is always for a navigation that is
+  // older or equal to "LastNavigationId". This must only be called on the UI
+  // thread.
+  int64_t LastNewDocNavigationId() const;
+
  protected:
   friend class PerformanceManagerTabHelper;
 

components/performance_manager/web_contents_proxy.cc

@@ -42,6 +42,13 @@ int64_t WebContentsProxy::LastNavigationId() const {
   return proxy->LastNavigationId();
 }
 
+int64_t WebContentsProxy::LastNewDocNavigationId() const {
+  auto* proxy = impl_.get();
+  if (!proxy)
+    return 0;
+  return proxy->LastNewDocNavigationId();
+}
+
 WebContentsProxy::WebContentsProxy(
     const base::WeakPtr<WebContentsProxyImpl>& impl)
     : impl_(impl) {}

components/performance_manager/web_contents_proxy_impl.h

@@ -35,6 +35,12 @@ class WebContentsProxyImpl {
   // web contents. This must only be called on the UI thread.
   virtual int64_t LastNavigationId() const = 0;
 
+  // Similar to the above, but for the last non same-document navigation
+  // associated with this WebContents. This is always for a navigation that is
+  // older or equal to "LastNavigationId". This must only be called on the UI
+  // thread.
+  virtual int64_t LastNewDocNavigationId() const = 0;
+
  private:
   DISALLOW_COPY_AND_ASSIGN(WebContentsProxyImpl);
 };