Restrict CM API interface request and message dispatch.

chrome/browser/password_manager/credential_manager_browsertest.cc

Index: chrome/browser/password_manager/credential_manager_browsertest.cc
diff --git a/chrome/browser/password_manager/credential_manager_browsertest.cc b/chrome/browser/password_manager/credential_manager_browsertest.cc
index f33470ed1a9c5dbc5b503a20f16e33f01cda07bc..40cd723ef7783088b10279f86d8d3200358ee642 100644
--- a/chrome/browser/password_manager/credential_manager_browsertest.cc
+++ b/chrome/browser/password_manager/credential_manager_browsertest.cc
@@ -3,23 +3,63 @@
 // found in the LICENSE file.
 
 #include "base/macros.h"
+#include "base/run_loop.h"
 #include "base/stl_util.h"
+#include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/password_manager/chrome_password_manager_client.h"
 #include "chrome/browser/password_manager/password_manager_test_base.h"
 #include "chrome/browser/password_manager/password_store_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_io_data.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/passwords/passwords_model_delegate.h"
+#include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/password_manager/core/browser/password_bubble_experiment.h"
 #include "components/password_manager/core/browser/test_password_store.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_contents_observer.h"
 #include "content/public/test/browser_test.h"
 #include "content/public/test/browser_test_utils.h"
 #include "net/dns/mock_host_resolver.h"
 
 namespace {
 
+// This class implements waiting for RenderFrameHost destruction. It relies on
+// the fact that RenderFrameDeleted event is fired when RenderFrameHost is
+// destroyed.

[vasilii, 2017/06/29 11:59:59]

Is the second sentence trying to say more than "RenderFrameDeleted means that
RenderFrameHost is being deleted". If not then I'd just drop it as confusing.

[engedy, 2017/06/30 19:41:55]

Yes, there is a subtle difference between the `Deleted` event and the RFH::~dtor
being called. Luckily, the two coincides.

But no longer applicable, as I found this exact class among test utils.

+class RenderFrameHostDestructionObserver : public content::WebContentsObserver {
+ public:
+  explicit RenderFrameHostDestructionObserver(content::RenderFrameHost* rfh)
+      : WebContentsObserver(content::WebContents::FromRenderFrameHost(rfh)),
+        render_frame_host_(rfh) {}
+  ~RenderFrameHostDestructionObserver() override {}
+
+  void WaitForDestruction() {
+    if (destroyed_)
+      return;

[vasilii, 2017/06/29 11:59:59]

Note that if destroyed_ is true then run_loop_.Run() exits immediately.
You still can have this condition, of course.

[engedy, 2017/06/30 19:41:55]

N/A, removed this class.

+    run_loop_.Run();
+  }
+
+  // WebContentsObserver:
+  void RenderFrameDeleted(content::RenderFrameHost* rfh) override {
+    if (rfh == render_frame_host_) {
+      ASSERT_FALSE(destroyed_);
+      destroyed_ = true;
+      run_loop_.Quit();
+    }
+  }
+
+ private:
+  base::RunLoop run_loop_;
+  content::RenderFrameHost* render_frame_host_;
+
+  bool destroyed_ = false;
+
+  DISALLOW_COPY_AND_ASSIGN(RenderFrameHostDestructionObserver);
+};
+
 class CredentialManagerBrowserTest : public PasswordManagerBrowserTestBase {
  public:
   CredentialManagerBrowserTest() = default;
@@ -48,6 +88,41 @@ class CredentialManagerBrowserTest : public PasswordManagerBrowserTestBase {
     observer.Wait();
   }
 
+  // Triggers a call to `navigator.credentials.get` to retrieve passwords, waits
+  // for success, and ASSERTs that |expect_has_results| is satisfied.
+  void TriggerNavigatorGetPasswordCredentialsAndExpectHasResult(
+      content::WebContents* web_contents,
+      bool expect_has_results) {
+    bool result = false;
+    ASSERT_TRUE(content::ExecuteScriptAndExtractBool(
+        web_contents,
+        "navigator.credentials.get({password: true}).then(c => {"
+        "  window.domAutomationController.send(!!c);"
+        "});",
+        &result));
+    ASSERT_EQ(expect_has_results, result);
+  }
+
+  // Schedules a call to be made to navigator.credentials.store() in the
+  // `unload` handler to save a credential with |username| and |password|.
+  void ScheduleNavigatorStoreCredentialAtUnload(
+      content::WebContents* web_contents,
+      const char* username,
+      const char* password) {
+    ASSERT_TRUE(content::ExecuteScript(
+        web_contents,
+        base::StringPrintf(
+            "window.addEventListener(\"unload\", () => {"
+            "  var c = new PasswordCredential({ id: '%s', password: '%s' });"
+            "  navigator.credentials.store(c);"
+            "});",
+            username, password)));
+  }
+
+  content::WebContents* active_web_contents() {
+    return browser()->tab_strip_model()->GetActiveWebContents();
+  }
+
  private:
   DISALLOW_COPY_AND_ASSIGN(CredentialManagerBrowserTest);
 };
@@ -245,6 +320,224 @@ IN_PROC_BROWSER_TEST_F(CredentialManagerBrowserTest,
   EXPECT_FALSE(prompt_observer.IsShowingSavePrompt());
 }
 
+// Regression test for https://crbug.com/736357.
+IN_PROC_BROWSER_TEST_F(
+    CredentialManagerBrowserTest,
+    StoreInUnloadHandler_ServicedUsingPriorURLForSameSiteNavigation) {
+  // Use URLs that differ on subdomains so we can tell which one was used for
+  // saving, but they still belong to the same SiteInstance, so they will be
+  // renderered in the same process.
+  const GURL a_url1 = https_test_server().GetURL("foo.a.com", "/title1.html");
+  const GURL a_url2 = https_test_server().GetURL("bar.a.com", "/title2.html");
+
+  for (const auto preestablish_mojo_pipe : {false, true}) {

[vasilii, 2017/06/29 11:59:59]

I'd prefer to split this test into two. The body can go to a method taking
preestablish_mojo_pipe as a parameter.
My concern is that the loop iteration may produce unwanted side effects.

[vasilii, 2017/06/29 12:00:00]

const bool

[engedy, 2017/06/30 19:41:55]

Done.

[engedy, 2017/06/30 19:41:55]

Ack. Done this one, and also the one below.

+    SCOPED_TRACE(::testing::Message()
+                 << "Pre-establish Mojo pipe ahead of time: "
+                 << preestablish_mojo_pipe);
+
+    // Open a new WebContents, navigate to a mostly empty page.
+    NavigateToURLWithDisposition(
+        browser(), a_url1, WindowOpenDisposition::NEW_FOREGROUND_TAB,
+        ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
+
+    ChromePasswordManagerClient* client =
+        ChromePasswordManagerClient::FromWebContents(active_web_contents());
+
+    // Optionally set up a Mojo connection to the CredentialManagerImpl ahead of
+    // time, instead of letting it be established on-demand when the call to
+    // store() triggered from the unload handler.
+    if (preestablish_mojo_pipe) {
+      EXPECT_FALSE(client->has_binding_for_credential_manager());
+      ASSERT_NO_FATAL_FAILURE(
+          TriggerNavigatorGetPasswordCredentialsAndExpectHasResult(
+              active_web_contents(), false));
+      EXPECT_TRUE(client->has_binding_for_credential_manager());
+    }
+
+    // Schedule storing a credential on the `unload` event.
+    ASSERT_NO_FATAL_FAILURE(ScheduleNavigatorStoreCredentialAtUnload(
+        active_web_contents(), "user", "hunter2"));
+
+    // Trigger a same-site navigation that is carried out in the same renderer.
+    content::RenderFrameHost* old_rfh = active_web_contents()->GetMainFrame();
+    ui_test_utils::NavigateToURL(browser(), a_url2);
+    base::RunLoop().RunUntilIdle();

[vasilii, 2017/06/29 11:59:59]

I'm not a big fan of RunUntilIdle() in the browser tests. The test may time out.
I think NavigateToURL already waits for everything you need.

[engedy, 2017/06/30 19:41:55]

Done.

+    ASSERT_EQ(old_rfh, active_web_contents()->GetMainFrame());
+
+    // Ensure that the old document no longer has a Mojo connection to the
+    // CredentialManagerImpl, nor can it get one later.
+    //
+    // Because the InterfaceRegistry through which the interface to the
+    // CredentialManagerImpl is retrieved is synchronized to FrameHost

[vasilii, 2017/06/29 11:59:59]

Is this request itself coming from IPC?

[engedy, 2017/06/30 19:41:54]

As discussed offline, made this an associated interface and updated comment.

+    // messages, there cannot be InterfaceRequest messages in flight.
+    //
+    // Furthermore, FrameLoader::PrepareForCommit had already shut down the old
+    // Document before the provisional load could be commited in the renderer,
+    // so no new InterfaceRequests can be issued either.
+    //
+    // Hence it is sufficient to check that the Mojo connection is closed now.
+    EXPECT_FALSE(client->has_binding_for_credential_manager());

[vasilii, 2017/06/29 11:59:59]

Is it the check that would fail before the CL?

[engedy, 2017/06/30 19:41:55]

Among others, yes.

+
+    // Ensure that the navigator.credentials.store() call was either never
+    // serviced, or was serviced in the context of the old URL, |a_url|.
+    //
+    // Both are possible, as CredentialManager Mojo messages on an pre-existing
+    // connection are no longer synchronized to FrameHost messages. Therefore,
+    // servicing the store() called from the `unload` handler, triggered from
+    // FrameLoader::PrepareForCommit, is at race with DidFinishNavigation, which
+    // is triggered by FrameHostMsg DidCommitProvisionalLoad.
+    if (client->was_store_ever_called()) {
+      BubbleObserver prompt_observer(active_web_contents());
+      prompt_observer.WaitForSavePrompt();

[vasilii, 2017/06/29 11:59:59]

You may really crash here. 
WaitForSavePrompt() can be used only on WebContents(). That is, a specially
created tab inside PasswordManagerBrowserTestBase with a fake UI controller.

Instead you can wait for password store and check
prompt_observer.IsShowingSavePrompt()

[engedy, 2017/06/30 19:41:54]

Done. (Using WebContents() now.)

+      ASSERT_TRUE(prompt_observer.IsShowingSavePrompt());
+      prompt_observer.AcceptSavePrompt();
+
+      WaitForPasswordStore();
+
+      password_manager::TestPasswordStore* test_password_store =
+          static_cast<password_manager::TestPasswordStore*>(
+              PasswordStoreFactory::GetForProfile(
+                  browser()->profile(), ServiceAccessType::IMPLICIT_ACCESS)
+                  .get());
+
+      ASSERT_EQ(1u, test_password_store->stored_passwords().size());
+      autofill::PasswordForm signin_form =
+          test_password_store->stored_passwords().begin()->second[0];
+      EXPECT_EQ(base::ASCIIToUTF16("user"), signin_form.username_value);
+      EXPECT_EQ(base::ASCIIToUTF16("hunter2"), signin_form.password_value);
+      EXPECT_EQ(a_url1.GetOrigin(), signin_form.origin);
+      test_password_store->Clear();
+    }
+  }
+}
+
+// Regression test for https://crbug.com/736357.
+IN_PROC_BROWSER_TEST_F(CredentialManagerBrowserTest,
+                       StoreInUnloadHandler_IgnoredOnCrossSiteNavigation) {
+  const GURL a_url = https_test_server().GetURL("a.com", "/title1.html");
+  const GURL b_url = https_test_server().GetURL("b.com", "/title2.html");
+
+  for (const auto preestablish_mojo_pipe : {false, true}) {
+    SCOPED_TRACE(::testing::Message()
+                 << "Pre-establish Mojo pipe ahead of time: "
+                 << preestablish_mojo_pipe);
+
+    // Open a new WebContents, navigate to a mostly empty page.
+    NavigateToURLWithDisposition(
+        browser(), a_url, WindowOpenDisposition::NEW_FOREGROUND_TAB,
+        ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
+
+    ChromePasswordManagerClient* client =
+        ChromePasswordManagerClient::FromWebContents(active_web_contents());
+
+    // Optionally set up a Mojo connection to the CredentialManagerImpl ahead of
+    // time, instead of letting it be established on-demand when the call to
+    // store() triggered from the unload handler.
+    if (preestablish_mojo_pipe) {
+      EXPECT_FALSE(client->has_binding_for_credential_manager());
+      ASSERT_NO_FATAL_FAILURE(
+          TriggerNavigatorGetPasswordCredentialsAndExpectHasResult(
+              active_web_contents(), false));
+      EXPECT_TRUE(client->has_binding_for_credential_manager());
+    }
+
+    // Schedule storing a credential on the `unload` event.
+    ASSERT_NO_FATAL_FAILURE(ScheduleNavigatorStoreCredentialAtUnload(
+        active_web_contents(), "user", "hunter2"));
+
+    // Trigger a cross-site navigation that is carried out in a new renderer,
+    // and which will swap out the old RenderFrameHost.
+    RenderFrameHostDestructionObserver rfh_destruction_observer(
+        active_web_contents()->GetMainFrame());
+    ui_test_utils::NavigateToURL(browser(), b_url);
+
+    // Ensure that the navigator.credentials.store() call is never serviced.

[vasilii, 2017/06/29 11:59:59]

Why can't the same scenario as in the previous test happen? (RenderFrameHost
gets a store() message right before deletion)

[engedy, 2017/06/30 19:41:55]

It's channel is disconnected in DidFinishNavigation, and it cannot establish a
new one due to the other check in
ChromePasswordManagerClient::BindCredentialManager. Does this make sense?

+    // The sufficient conditions for this are:
+    //  -- The swapped out RFH is destroyed, so the RenderFrame cannot establish
+    //     a new Mojo connection to CredentialManagerImpl anymore.
+    //  -- There is no already existing Mojo connection to CredentialManagerImpl
+    //     either, which could be used to call store() in the future.
+    //  -- There have not been any calls to store() in the past.
+    rfh_destruction_observer.WaitForDestruction();
+    base::RunLoop().RunUntilIdle();

[vasilii, 2017/06/29 11:59:59]

Unnecessary I think.

[engedy, 2017/06/30 19:41:55]

Done.

+    EXPECT_FALSE(client->has_binding_for_credential_manager());
+    EXPECT_FALSE(client->was_store_ever_called());
+  }
+}
+
+// Regression test for https://crbug.com/736357.
+IN_PROC_BROWSER_TEST_F(CredentialManagerBrowserTest,
+                       MojoConnectionRecreatedAfterNavigation) {
+  const GURL a_url1 = https_test_server().GetURL("a.com", "/title1.html");
+  const GURL a_url2 = https_test_server().GetURL("a.com", "/title2.html");
+  const GURL a_url2_ref = https_test_server().GetURL("a.com", "/title2.html#r");
+  const GURL b_url = https_test_server().GetURL("b.com", "/title2.html#ref");
+
+  // Enable 'auto signin' for the profile.
+  password_bubble_experiment::RecordAutoSignInPromptFirstRunExperienceWasShown(
+      browser()->profile()->GetPrefs());
+
+  // Navigate to a mostly empty page.
+  ui_test_utils::NavigateToURL(browser(), a_url1);
+
+  ChromePasswordManagerClient* client =
+      ChromePasswordManagerClient::FromWebContents(active_web_contents());
+
+  // Store a credential, and expect it to establish the Mojo connection.
+  EXPECT_FALSE(client->has_binding_for_credential_manager());
+  EXPECT_FALSE(client->was_store_ever_called());
+
+  ASSERT_TRUE(content::ExecuteScript(
+      active_web_contents(),
+      "var c = new PasswordCredential({ id: 'user', password: 'hunter2' });"
+      "navigator.credentials.store(c);"));
+
+  BubbleObserver prompt_observer(active_web_contents());

[vasilii, 2017/06/29 11:59:59]

Please use WebContents() for the reasons stated above.

[engedy, 2017/06/30 19:41:55]

Done.

+  prompt_observer.WaitForSavePrompt();
+  ASSERT_TRUE(prompt_observer.IsShowingSavePrompt());
+  prompt_observer.AcceptSavePrompt();
+  WaitForPasswordStore();
+
+  EXPECT_TRUE(client->has_binding_for_credential_manager());
+  EXPECT_TRUE(client->was_store_ever_called());
+
+  // Trigger a same-site navigation.
+  content::RenderFrameHost* old_rfh = active_web_contents()->GetMainFrame();
+  ui_test_utils::NavigateToURL(browser(), a_url2);
+  base::RunLoop().RunUntilIdle();

[vasilii, 2017/06/29 12:00:00]

ditto

[engedy, 2017/06/30 19:41:55]

Done.

+  ASSERT_EQ(old_rfh, active_web_contents()->GetMainFrame());
+
+  // Expect the Mojo connection closed.
+  EXPECT_FALSE(client->has_binding_for_credential_manager());
+
+  // Calling navigator.credentials.get() again should re-establish the Mojo
+  // connection and succeed.
+  ASSERT_NO_FATAL_FAILURE(
+      TriggerNavigatorGetPasswordCredentialsAndExpectHasResult(
+          active_web_contents(), true));
+  EXPECT_TRUE(client->has_binding_for_credential_manager());
+
+  // Same-document navigation. Call to get() succeeds.
+  ui_test_utils::NavigateToURL(browser(), a_url2_ref);
+  ASSERT_EQ(old_rfh, active_web_contents()->GetMainFrame());

[vasilii, 2017/06/29 12:00:00]

Do you want to check the connection?

[engedy, 2017/06/30 19:41:55]

Done.

+  ASSERT_NO_FATAL_FAILURE(
+      TriggerNavigatorGetPasswordCredentialsAndExpectHasResult(
+          active_web_contents(), true));
+
+  // Cross-site navigation. Call to get() succeeds without results.
+  ui_test_utils::NavigateToURL(browser(), b_url);
+  ASSERT_NO_FATAL_FAILURE(
+      TriggerNavigatorGetPasswordCredentialsAndExpectHasResult(
+          active_web_contents(), false));
+
+  // Trigger a cross-site navigation back. Call to get() should still succeed,
+  // and once again with results.
+  ui_test_utils::NavigateToURL(browser(), a_url1);
+  ASSERT_NO_FATAL_FAILURE(
+      TriggerNavigatorGetPasswordCredentialsAndExpectHasResult(
+          active_web_contents(), true));
+}
+
 IN_PROC_BROWSER_TEST_F(CredentialManagerBrowserTest, SaveViaAPIAndAutofill) {
   NavigateToFile("/password/password_form.html");