Another attempt at fixing dead frames being tracked by webNavigation

chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc

Index: chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc
diff --git a/chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc b/chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc
index 1b4cfb7a4335f728fc6fb79c1dd45cf43c230f60..8e8b321629c390c9b274d2944b2d0ddbe2e6484c 100644
--- a/chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc
+++ b/chrome/browser/extensions/api/web_navigation/web_navigation_apitest.cc
@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/utf_string_conversions.h"
+#include "base/stringprintf.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/extensions/api/web_navigation/web_navigation_api.h"
 #include "chrome/browser/extensions/extension_apitest.h"
@@ -10,6 +12,7 @@
 #include "chrome/browser/tab_contents/render_view_context_menu.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_tabstrip.h"
+#include "chrome/common/chrome_notification_types.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/public/browser/render_view_host.h"
@@ -44,6 +47,69 @@ class TestRenderViewContextMenu : public RenderViewContextMenu {
   DISALLOW_COPY_AND_ASSIGN(TestRenderViewContextMenu);
 };
 
+// Waits for a WC to be created and invokes the given JavaScript |method| on
+// that WC once it starts loading |url|.
+class CallMethodOnLoad : public content::NotificationObserver,
+                         public content::WebContentsObserver {
+ public:
+  CallMethodOnLoad(const std::string& method,

[Charlie Reis, 2012/08/06 20:04:13]

This is really just an arbitrary string to execute, right?  Maybe the name
should be ExecuteJavascriptOnLoad, since it's not limited to methods.

Also, ...OnLoad makes it sound like the JavaScript onload event, which happens
after the load commits.  Perhaps we should call it ExecuteJavascriptOnLoadStart.

[jochen (gone - plz use gerrit), 2012/08/06 21:08:39]

Done.

+                   const GURL& url)
+      : content::WebContentsObserver(),
+        method_(method),
+        url_(url),
+        rvh_(NULL) {
+    registrar_.Add(this,
+                   chrome::NOTIFICATION_TAB_ADDED,
+                   content::NotificationService::AllSources());
+  }
+  virtual ~CallMethodOnLoad() {}
+
+  virtual void Observe(int type,
+                       const content::NotificationSource& source,
+                       const content::NotificationDetails& details) OVERRIDE {
+    if (type != chrome::NOTIFICATION_TAB_ADDED) {
+      NOTREACHED();
+      return;
+    }
+    content::WebContentsObserver::Observe(
+        content::Details<content::WebContents>(details).ptr());
+    registrar_.RemoveAll();
+  }
+
+  virtual void DidStartProvisionalLoadForFrame(
+      int64 frame_id,
+      bool is_main_frame,
+      const GURL& validated_url,
+      bool is_error_page,
+      content::RenderViewHost* render_view_host) OVERRIDE {
+    if (validated_url != url_ || !rvh_)

[Charlie Reis, 2012/08/06 20:04:13]

This is an unexpected condition to me.  It looks like rvh_ isn't set until
commit, so this ExecuteJavascript call doesn't happen until the second page load
starts.  Is that correct?  Maybe that should be made clearer in the class-level
comment.

[jochen (gone - plz use gerrit), 2012/08/06 21:08:39]

Done.

+      return;
+
+    rvh_->ExecuteJavascriptInWebFrame(string16(), UTF8ToUTF16(method_));
+    content::WebContentsObserver::Observe(NULL);
+  }
+
+  virtual void DidCommitProvisionalLoadForFrame(
+      int64 frame_id,
+      bool is_main_frame,
+      const GURL& url,
+      content::PageTransition transition_type,
+      content::RenderViewHost* render_view_host) OVERRIDE {
+    if (!is_main_frame)
+      return;
+    rvh_ = render_view_host;
+  }
+
+ private:
+  content::NotificationRegistrar registrar_;
+
+  std::string method_;
+  GURL url_;
+  content::RenderViewHost* rvh_;
+
+  DISALLOW_COPY_AND_ASSIGN(CallMethodOnLoad);
+};
+
 }  // namespace
 
 IN_PROC_BROWSER_TEST_F(ExtensionApiTest, WebNavigation) {
@@ -320,4 +386,30 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, WebNavigationHistory) {
           << message_;
 }
 
+IN_PROC_BROWSER_TEST_F(ExtensionApiTest, WebNavigationCrossProcess) {
+  FrameNavigationState::set_allow_extension_scheme(true);
+  host_resolver()->AddRule("*", "127.0.0.1");
+  ASSERT_TRUE(StartTestServer());
+
+  CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kAllowLegacyExtensionManifests);
+
+  LoadExtension(test_data_dir_.AppendASCII("webnavigation").AppendASCII("app"));
+
+  // See crossProcess/d.html.
+  CallMethodOnLoad call_method("navigate2()", test_server()->GetURL("slow?30"));

[Charlie Reis, 2012/08/06 20:04:13]

When running this test manually, can you check whether this slow?30 URL actually
runs for 30 seconds, even if the test finishes earlier?  Depending on how the
slow URL works, we could end up blocking for 93 seconds.  (Or the sleeping
process could be alive that long.)

(I've seen this in practice, which is why I wanted to bring it up.  It affected
subsequent tests that tried to use the HTTP server, I believe.)

[jochen (gone - plz use gerrit), 2012/08/06 21:08:39]

Chrome cancels the request immediately, so the test server keeps running and 
throws a "broken pipe" exception, but Chrome immediately stops. I guess that's
ok from a test point of view, as each test starts its own new server?

[Charlie Reis, 2012/08/06 21:30:26]

I'd prefer you check with a Chrome trooper about this before landing.  I've run
into trouble before with having lots of old test server processes waiting for
long delays.  Your idea of using a ResourceThrottle that pauses the response for
tests seems much less flaky and less inclined to cause problems on the bots.

If there's time pressure and if they're ok with a TODO and a bug to later
improve the approach, then I'm ok with it as well.

[jochen (gone - plz use gerrit), 2012/08/06 21:42:32]

I talked with Nicolas and he pointed out that while it shouldn't affect
following tests, it'll make the bots hang if this happens to be the last test. I
guess I'll give the resource throttle a try

+
+  // See crossProcess/e.html.
+  CallMethodOnLoad call_method2(
+      "updateHistory()", test_server()->GetURL("slow?31"));
+
+  // See crossProcess/f.html.
+  CallMethodOnLoad call_method3(
+      "updateFragment()", test_server()->GetURL("slow?32"));
+
+  ASSERT_TRUE(
+      RunExtensionSubtest("webnavigation", "test_crossProcess.html"))
+          << message_;
+}
+
 }  // namespace extensions