Don't show dialogs from swapped out frames.

content/browser/frame_host/navigation_controller_impl_browsertest.cc

Index: content/browser/frame_host/navigation_controller_impl_browsertest.cc
diff --git a/content/browser/frame_host/navigation_controller_impl_browsertest.cc b/content/browser/frame_host/navigation_controller_impl_browsertest.cc
index 405e65008a2fc1db15c2f90f86afeeb48db0a749..3093cb42699baec76c5a207e0fe9342ecebb14a0 100644
--- a/content/browser/frame_host/navigation_controller_impl_browsertest.cc
+++ b/content/browser/frame_host/navigation_controller_impl_browsertest.cc
@@ -27,6 +27,7 @@
 #include "content/public/browser/resource_dispatcher_host_delegate.h"
 #include "content/public/browser/resource_throttle.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_delegate.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/common/bindings_policy.h"
 #include "content/public/common/browser_side_navigation_policy.h"
@@ -5968,7 +5969,7 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
   EXPECT_EQ(start_url.GetOrigin().spec(), origin + "/");
 }
 
-// A BrowserMessageFilter that delays FrameHostMsg_DidCommitProvisionaLoad IPC
+// A BrowserMessageFilter that delays FrameHostMsg_DidCommitProvisionalLoad IPC
 // message for a specified URL, navigates the WebContents back and then
 // processes the commit message.
 class GoBackAndCommitFilter : public BrowserMessageFilter {
@@ -5996,7 +5997,7 @@ class GoBackAndCommitFilter : public BrowserMessageFilter {
     if (message.type() != FrameHostMsg_DidCommitProvisionalLoad::ID)
       return false;
 
-    // Parse the IPC message so the URL can be checked agains the expected one.
+    // Parse the IPC message so the URL can be checked against the expected one.
     base::PickleIterator iter(message);
     FrameHostMsg_DidCommitProvisionalLoad_Params validated_params;
     if (!IPC::ParamTraits<FrameHostMsg_DidCommitProvisionalLoad_Params>::Read(
@@ -6137,4 +6138,85 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
   EXPECT_FALSE(favicon_status3.valid);
 }
 
+namespace {
+
+// A BrowserMessageFilter that delays the FrameHostMsg_RunJavaScriptMessage IPC
+// message until a commit happens on a given WebContents. This allows testing a
+// race condition.
+class AllowDialogIPCOnCommitFilter : public BrowserMessageFilter,
+                                     public WebContentsObserver,
+                                     public WebContentsDelegate {
+ public:
+  AllowDialogIPCOnCommitFilter(WebContents* web_contents)
+      : BrowserMessageFilter(FrameMsgStart),
+        WebContentsObserver(web_contents),
+        render_frame_host_(web_contents->GetMainFrame()) {}
+
+ protected:
+  ~AllowDialogIPCOnCommitFilter() override {}
+
+ private:
+  // BrowserMessageFilter:
+  bool OnMessageReceived(const IPC::Message& message) override {
+    DCHECK_CURRENTLY_ON(BrowserThread::IO);

[Charlie Reis, 2016/09/27 22:18:17]

Looks like this is failing on the bots?

[nasko, 2016/09/27 22:24:22]

Hmm, this is a bit problematic since this object is both BrowserMessageFilter
and WebContentsObserver, both of which have OnMessageReceived. I'd expect this
will be called on both threads, even though it is even more weird to think which
version of the method that is inheriting : ).

[Avi (use Gerrit), 2016/09/27 23:00:43]

Will fix in the next version.

+    if (message.type() != FrameHostMsg_RunJavaScriptMessage::ID)
+      return false;
+
+    // Suspend the message.
+    message_ = message;
+    return true;
+  }
+
+  // WebContentsObserver:
+  void DidNavigateAnyFrame(RenderFrameHost* render_frame_host,
+                           const LoadCommittedDetails& details,
+                           const FrameNavigateParams& params) override {
+    DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+    // Resume the message.
+    render_frame_host_->OnMessageReceived(message_);
+  }
+
+  // WebContentsDelegate:
+  JavaScriptDialogManager* GetJavaScriptDialogManager(
+      WebContents* source) override {
+    // We can't call FAIL() from here because this isn't a browser test
+    // function, so just crash as loudly as we can.
+    *(volatile int*)(nullptr) = 0xDEAD;

[Charlie Reis, 2016/09/27 22:18:17]

NOTREACHED or CHECK(false)?  Or are those not as effective?

[Avi (use Gerrit), 2016/09/27 23:00:43]

I had DCHECK(false) which didn't do anything. I'll try CHECK(false).

+    return nullptr;
+  }
+
+  RenderFrameHost* render_frame_host_;
+  IPC::Message message_;
+
+  DISALLOW_COPY_AND_ASSIGN(AllowDialogIPCOnCommitFilter);
+};
+
+}  // namespace
+
+// Check that swapped out frames cannot spawn JavaScript dialogs.
+IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
+                       NoDialogsFromSwappedOutFrames) {
+  // Start on a normal page.
+  GURL url1 = embedded_test_server()->GetURL(
+      "/navigation_controller/beforeunload_dialog.html");
+  EXPECT_TRUE(NavigateToURL(shell(), url1));
+
+  // Add a filter to allow us to force an IPC race.
+  WebContents* web_contents = shell()->web_contents();
+  scoped_refptr<AllowDialogIPCOnCommitFilter> filter =
+      new AllowDialogIPCOnCommitFilter(web_contents);
+  web_contents->SetDelegate(filter.get());
+  web_contents->GetMainFrame()->GetProcess()->AddFilter(filter.get());
+
+  // Use a chrome:// url to force the second page to be in a different process.
+  GURL url2(std::string(kChromeUIScheme) + url::kStandardSchemeSeparator +
+            kChromeUIGpuHost);
+  EXPECT_TRUE(NavigateToURL(shell(), url2));
+
+  // The commit in the new renderer will unblock the dialog IPC from the old
+  // renderer. If the dialog IPC is allowed to proceed, the override of
+  // GetJavaScriptDialogManager will crash and the test will fail.
+}
+
 }  // namespace content