Don't show dialogs from swapped out frames.

content/browser/frame_host/navigation_controller_impl_browsertest.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/frame_host/navigation_controller_impl.h"
 
 #include <stdint.h>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/macros.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "content/browser/frame_host/frame_navigation_entry.h"
 #include "content/browser/frame_host/frame_tree.h"
 #include "content/browser/frame_host/navigation_entry_impl.h"
 #include "content/browser/loader/resource_dispatcher_host_impl.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/common/frame_messages.h"
 #include "content/common/page_state_serialization.h"
 #include "content/common/site_isolation_policy.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/resource_controller.h"
 #include "content/public/browser/resource_dispatcher_host.h"
 #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"
 #include "content/public/common/renderer_preferences.h"
 #include "content/public/common/url_constants.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/content_browser_test.h"
 #include "content/public/test/content_browser_test_utils.h"
 #include "content/public/test/test_navigation_observer.h"
 #include "content/public/test/test_utils.h"
@@ skipping 5921 matching lines @@
   }
 
   // Verify the expected origin through JavaScript. It also has the additional
   // verification of the process also being still alive.
   std::string origin;
   EXPECT_TRUE(ExecuteScriptAndExtractString(
       web_contents, "domAutomationController.send(document.origin)", &origin));
   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 {
  public:
   GoBackAndCommitFilter(const GURL& url, WebContentsImpl* web_contents)
       : BrowserMessageFilter(FrameMsgStart),
         url_(url),
         web_contents_(web_contents) {}
 
  protected:
   ~GoBackAndCommitFilter() override {}
 
  private:
   static void NavigateBackAndCommit(const IPC::Message& message,
                                     WebContentsImpl* web_contents) {
     web_contents->GetController().GoBack();
 
     RenderFrameHostImpl* rfh = web_contents->GetMainFrame();
     DCHECK_EQ(rfh->routing_id(), message.routing_id());
     rfh->OnMessageReceived(message);
   }
 
   // BrowserMessageFilter:
   bool OnMessageReceived(const IPC::Message& message) override {
     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(
             &message, &iter, &validated_params)) {
       return false;
     }
 
     // Only handle the message if the URLs are matching.
     if (validated_params.url != url_)
       return false;
@@ skipping 120 matching lines @@
   entry = controller.GetLastCommittedEntry();
   content::FaviconStatus& favicon_status2 = entry->GetFavicon();
   EXPECT_TRUE(favicon_status2.valid);
 
   ASSERT_TRUE(RendererLocationReplace(shell(), GURL("data:text/html,page2")));
   entry = controller.GetLastCommittedEntry();
   content::FaviconStatus& favicon_status3 = entry->GetFavicon();
   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