Revert "Revert of Reland: Switch WindowedNotificationObserver to use base::RunLoop. (patchset #3 id…

chrome/browser/extensions/api/extension_action/browser_action_apitest.cc

 // Copyright (c) 2012 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 <stdint.h>
 
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/macros.h"
@@ skipping 889 matching lines @@
     // Verify if the popup navigation succeeded or failed as expected.
     if (!popup) {
       // If navigation ends up in a tab, then the tab will be focused and
       // therefore the popup will be closed, destroying associated WebContents -
       // don't do any verification in this case.
       ADD_FAILURE() << "Navigation should not close extension pop-up";
     } else {
       // If the extension popup is still opened, then wait until there is no
       // load in progress, and verify whether the navigation succeeded or not.
       WaitForLoadStop(popup);
+
+      // TODO(https://crbug.com/695073): Find out why tests fail without it.

[jam, 2017/03/09 16:52:57]

no one is assigned that bug, so I worry that we'll never get to it. 

Since the test fails consistently without this with PlzNavigate, seems like a
repro scenario that can help track this down?

otherwise the advantage of using RunLoop seems to be lost if we have to sprinkle
this.

[Alexander Semashko, 2017/03/09 19:47:48]

I was hoping that an extensions/ owner would pick it, and currently
planning to address it later, if nobody chimes in. Because I'm quite
overloaded now, and the investigation can be very time consuming (I
did a quick attempt earlier).

I'm afraid that if we postpone this CL, there might appear new tests
and/or helpers that implicitly depend on deferred quit, and it will
cause another sad series of commits and reverts. OTOH, the repro
scenario will likely remain.

[jam, 2017/03/09 20:48:46]

While I do appreciate this change in making things more deterministic, this has
been reverted a few times already for making some tests flaky. Given we don't
have visibility in how many other tests this increased flakiness for, I think
landing this again before understanding the previous flakiness is premature. Yes
there's a small chance that new tests can depend on this behavior, but that
seems unlikely since we only have a small number of known failures out of many
thousands of browser tests.

The solution here to clear the pending tasks in some select areas is worrisome
to me: how much more flakiness is this adding in other areas that we don't know
about?

+      content::RunAllPendingInMessageLoop();
+
       if (expected_navigation_status == EXPECTING_NAVIGATION_SUCCESS) {
         EXPECT_EQ(target_url, popup->GetLastCommittedURL())
             << "Navigation to " << target_url
             << " should succeed in an extension pop-up";
       } else {
         EXPECT_NE(target_url, popup->GetLastCommittedURL())
             << "Navigation to " << target_url
             << " should fail in an extension pop-up";
         EXPECT_THAT(
             popup->GetLastCommittedURL(),
@@ skipping 120 matching lines @@
   // The test verification below is applicable only to scenarios where the
   // download shelf is supported - on ChromeOS, instead of the download shelf,
   // there is a download notification in the right-bottom corner of the screen.
 #if !defined(OS_CHROMEOS)
   EXPECT_TRUE(browser()->window()->IsDownloadShelfVisible());
 #endif
 }
 
 }  // namespace
 }  // namespace extensions