Don't defer quitting in ExecuteScript and other helpers that use DOMMessageQueue.

chrome/browser/plugins/flash_permission_browsertest.cc

 // Copyright 2016 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 "base/base_switches.h"
 #include "base/command_line.h"
 #include "base/path_service.h"
 #include "chrome/browser/permissions/permissions_browsertest.h"
 #include "chrome/browser/ui/website_settings/mock_permission_prompt_factory.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/variations/variations_switches.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/ppapi_test_utils.h"
 #include "third_party/WebKit/public/platform/WebInputEvent.h"
 #include "url/gurl.h"
 
+namespace {
+
+class PageReloadWaiter {

[nasko, 2016/11/30 17:38:56]

Why do we need this wrapper for TestNavigationManager? The goal of TNM is to be
as simple to use, so I see this wrapper as unnecessary complexity.
If a wait for full load completion is needed, there is TestNavigationObserver
class that accomplishes exactly that.

[Alexander Semashko, 2016/11/30 22:23:08]

Yes, waiting only for commit here seems wrong.
TestNavigationObserver does not fit well, because in some cases there are
intermediate navigations to urls like
https://get.adobe.com/flashplayer/, which are then dropped. TNO stops waiting at
this moment, but the reload that we're waiting for starts later, which brings us
to an awkward construction with two observers. The current way filters
navigations by URL, which looks a bit clearer to me.

By the way, don't you think that the name TestNavigationObserver is somewhat
misleading? It sort of implies that this class waits for DidFinishNavigation at
most, not the whole page load.

[nasko, 2016/12/01 01:17:28]

Maybe UrlLoadObserver then fits the picture better?
In general, we should reuse code as much as possible and avoiding one-off
implementations whenever we can. It makes fixing issues much easier later on.
 
Welcome to reality and history :). TestNavigationObserver existed long before
DidFinishNavigation and our effort in being more precise/explicit about what is
navigation and what is a document load.

[Alexander Semashko, 2016/12/01 12:24:59]

Unfortunately, no. When an intermediate navigation gets cancelled,
UrlLoadObserver sees that navigation stopped, and that the last committed url is
the "right" one, and quits.
I'm not happy to add this class too. It even makes me think that probably we
need a generic solution with something like "navigation matchers" that can
encode and check various conditions for different navigation steps and can be
combined to handle complex cases. If only this could be lightweight and easy to
use...

[nasko, 2016/12/01 17:23:09]

: ( Is the intermediate navigation in the same frame as the last committed one?
Do you know what cancels it?
nick@ is working on a more generic way to have control over navigations in
tests, maybe this scenario can be factored in that.

I do think that it is best to fix the loop quit issue, so let's proceed with
this as is and come back in the future to see if it can be generalized. Also, if
you end up hitting the same requirements in other tests, we can do more general
work at that time.
 

+ public:
+  explicit PageReloadWaiter(content::WebContents* web_contents)
+      : web_contents_(web_contents),
+        navigation_observer_(web_contents,
+                             web_contents->GetLastCommittedURL()) {}
+
+  void Wait() {
+    navigation_observer_.WaitForNavigationFinished();
+    EXPECT_TRUE(content::WaitForLoadStop(web_contents_));
+  }
+
+ private:
+  content::WebContents* web_contents_;
+  content::TestNavigationManager navigation_observer_;
+};
+
+}  // namespace
+
 class FlashPermissionBrowserTest : public PermissionsBrowserTest {
  public:
   FlashPermissionBrowserTest()
       : PermissionsBrowserTest("/permissions/flash.html") {}
   ~FlashPermissionBrowserTest() override {}
 
   // PermissionsBrowserTest
   void SetUpCommandLine(base::CommandLine* command_line) override {
     PermissionsBrowserTest::SetUpCommandLine(command_line);
 
     ASSERT_TRUE(ppapi::RegisterFlashTestPlugin(command_line));
 
     // Set a high engagement threshhold so it doesn't interfere with testing the
     // permission.
     command_line->AppendSwitchASCII(switches::kEnableFeatures,
                                     "PreferHtmlOverPlugins<Study1");
     command_line->AppendSwitchASCII(switches::kForceFieldTrials,
                                     "Study1/Enabled/");
     command_line->AppendSwitchASCII(
         variations::switches::kForceFieldTrialParams,
         "Study1.Enabled:engagement_threshold_for_flash/100");
   }
 
   void TriggerPrompt() override {
     if (prompt_factory()->response_type() ==
         PermissionRequestManager::ACCEPT_ALL) {
       // If the prompt will be allowed, we need to wait for the page to refresh.
-      content::TestNavigationManager observer(
-          GetWebContents(), GetWebContents()->GetLastCommittedURL());
+      PageReloadWaiter reload_waiter(GetWebContents());
       EXPECT_TRUE(RunScriptReturnBool("triggerPrompt();"));
-      observer.WaitForNavigationFinished();
+      reload_waiter.Wait();
     } else {
       EXPECT_TRUE(RunScriptReturnBool("triggerPrompt();"));
     }
   }
 
   bool FeatureUsageSucceeds() override {
-    // Wait until the page is refreshed before testing whether flash is enabled
-    // or disabled.
-    ui_test_utils::NavigateToURL(browser(),

[Alexander Semashko, 2016/11/30 22:23:08]

Just for the information.
This was an "extra" navigation, seemingly not needed in any of the tests.
And in TriggerPromptViaNewWindow auto-reload could happen after entering
NavigateToURL, which produced the most weird results (either interrupting the
navigation at a random point, or happening while we wait for script result).

-                                 GetWebContents()->GetLastCommittedURL());
     // If either flash with or without fallback content runs successfully it
     // indicates the feature is at least partly working, which could imply a
     // faulty permission.
     return RunScriptReturnBool("flashIsEnabled();") ||
            RunScriptReturnBool("flashIsEnabledForPluginWithoutFallback();");
   }
 };
 
 IN_PROC_BROWSER_TEST_F(FlashPermissionBrowserTest,
                        CommonFailsBeforeRequesting) {
   CommonFailsBeforeRequesting();
 }
 
 IN_PROC_BROWSER_TEST_F(FlashPermissionBrowserTest, CommonFailsIfDismissed) {
   CommonFailsIfDismissed();
 }
 
 IN_PROC_BROWSER_TEST_F(FlashPermissionBrowserTest, CommonFailsIfBlocked) {
   CommonFailsIfBlocked();
 }
 
 IN_PROC_BROWSER_TEST_F(FlashPermissionBrowserTest, CommonSucceedsIfAllowed) {
   CommonSucceedsIfAllowed();
 }
 
 IN_PROC_BROWSER_TEST_F(FlashPermissionBrowserTest, TriggerPromptViaNewWindow) {
   EXPECT_EQ(0, prompt_factory()->total_request_count());
   prompt_factory()->set_response_type(PermissionRequestManager::ACCEPT_ALL);
+  // FlashPermissionContext::UpdateTabContext will reload the page, we'll have
+  // to wait until it is ready.
+  PageReloadWaiter reload_waiter(GetWebContents());
   EXPECT_TRUE(RunScriptReturnBool("triggerPromptViaNewWindow();"));
+  reload_waiter.Wait();
 
   EXPECT_TRUE(FeatureUsageSucceeds());
   EXPECT_EQ(1, prompt_factory()->total_request_count());
 }
 
 IN_PROC_BROWSER_TEST_F(FlashPermissionBrowserTest,
                        TriggerPromptViaPluginPlaceholder) {
   EXPECT_EQ(0, prompt_factory()->total_request_count());
   EXPECT_FALSE(FeatureUsageSucceeds());
   prompt_factory()->set_response_type(PermissionRequestManager::ACCEPT_ALL);
   // We need to simulate a mouse click to trigger the placeholder to prompt.
-  content::TestNavigationManager observer(
-      GetWebContents(), GetWebContents()->GetLastCommittedURL());
+  // When the prompt is auto-accepted, the page will be reloaded.
+  PageReloadWaiter reload_waiter(GetWebContents());
   content::SimulateMouseClickAt(GetWebContents(), 0 /* modifiers */,
                                 blink::WebMouseEvent::Button::Left,
                                 gfx::Point(50, 50));
-  observer.WaitForNavigationFinished();
+  reload_waiter.Wait();
 
   EXPECT_TRUE(FeatureUsageSucceeds());
   EXPECT_EQ(1, prompt_factory()->total_request_count());
 }
 
 IN_PROC_BROWSER_TEST_F(FlashPermissionBrowserTest, AllowFileURL) {
   base::FilePath test_path;
   PathService::Get(chrome::DIR_TEST_DATA, &test_path);
   ui_test_utils::NavigateToURL(
       browser(), GURL("file://" + test_path.AsUTF8Unsafe() + test_url()));
@@ skipping 14 matching lines @@
       browser(), GURL("file://" + test_path.AsUTF8Unsafe() + test_url()));
   CommonFailsIfBlocked();
   EXPECT_EQ(1, prompt_factory()->total_request_count());
 
   // Navigate to a second URL to verify it's blocked on all file: URLs.
   ui_test_utils::NavigateToURL(
       browser(),
       GURL("file://" + test_path.AsUTF8Unsafe() + "/permissions/flash2.html"));
   EXPECT_FALSE(FeatureUsageSucceeds());
 }