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

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

Index: chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
diff --git a/chrome/browser/extensions/api/extension_action/browser_action_apitest.cc b/chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
index 2d57dda866e562ffd42d7201213406c8be3b8c72..95d95e3f2e69bc30bddd8dffb60870858af64967 100644
--- a/chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
+++ b/chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
@@ -907,6 +907,10 @@ class NavigatingExtensionPopupBrowserTest : public BrowserActionApiTest {
       // 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