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 WebContentsDelegate {
+ public:
+  AllowDialogIPCOnCommitFilter(WebContents* web_contents)
+      : BrowserMessageFilter(FrameMsgStart),
+        render_frame_host_(web_contents->GetMainFrame()) {
+    web_contents_observer_.Observe(web_contents);
+  }
+
+ protected:
+  ~AllowDialogIPCOnCommitFilter() override {}
+
+ private:
+  // BrowserMessageFilter:
+  bool OnMessageReceived(const IPC::Message& message) override {
+    DCHECK_CURRENTLY_ON(BrowserThread::IO);
+    if (message.type() != FrameHostMsg_RunJavaScriptMessage::ID)
+      return false;
+
+    // Suspend the message.
+    web_contents_observer_.SetCallback(
+        base::Bind(&RenderFrameHost::OnMessageReceived,
+                   base::Unretained(render_frame_host_), message));
+    return true;
+  }
+
+  // WebContentsDelegate:
+  JavaScriptDialogManager* GetJavaScriptDialogManager(
+      WebContents* source) override {
+    CHECK(false);
+    return nullptr;  // agh compiler
+  }
+
+  // Separate because WebContentsObserver and BrowserMessageFilter each have an
+  // OnMessageReceived function; this is the simplest way to disambiguate.
+  class : public WebContentsObserver {

[Charlie Reis, 2016/09/28 17:14:16]

Wow, I don't know that I've seen anonymous inner classes in C++ before.  Neat. 
:)

[Avi (use Gerrit), 2016/09/28 17:46:16]

Thanks! I'm not trying to be clever; this seems to be the simplest way of doing
it.

It seems to be hinted at in the C++ style guide, but it certainly not
prohibited.

+   public:
+    using Callback = base::Callback<bool()>;
+
+    using WebContentsObserver::Observe;

[Charlie Reis, 2016/09/28 17:14:16]

Is this needed?  Seems like we should be able to resolve the Observe call above,
but I could be wrong.

[Avi (use Gerrit), 2016/09/28 17:46:16]

No, it's protected, and even though (I think) there's implicit friendship,
friends don't get to call protected functions.

+
+    void SetCallback(Callback callback) { callback_ = callback; }
+
+   private:
+    void DidNavigateAnyFrame(RenderFrameHost* render_frame_host,
+                             const LoadCommittedDetails& details,
+                             const FrameNavigateParams& params) override {
+      DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+      // Resume the message.
+      callback_.Run();
+    }
+
+    Callback callback_;
+  } web_contents_observer_;
+
+  RenderFrameHost* render_frame_host_;
+
+  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));
+
+  // What happens now is that attempting to unload the first page will trigger a
+  // JavaScript alert but allow navigation. The alert IPC will be suspended by
+  // the message filter. The commit of the second page will unblock the IPC. If
+  // the dialog IPC is allowed to spawn a dialog, the call by the WebContents to
+  // its delegate to get the JavaScriptDialogManager will cause a CHECK and the
+  // test will fail.
+}
+
 }  // namespace content