Reland: DNR tests: Use ExtensionTestMessageListener to detect background page load.

This CL relands r555005 which was reverted since it introduced flakiness. The
flakiness was most probably since we ended up creating the
ExtensionTestMessageListener too late for the
DeclarativeNetRequestBrowserTest_Packed.PageWhitelistingAPI test. The listener
was created in SetUpOnMainThread override while the InstalledLoader ends up
loading all extensions before this (during
BrowserMainLoop::CreateStartupTasks()). Hence a subsequent
ExtensionTestMessageListener::WaitUntilSatisfied() call would end up running
indefinitely. To get around this, only wait for the background page to load if
the background page isn't already ready.

BUG= 696822, 838536

Change-Id: I2b288c6e6703e5be588d480403b362bf7c30db4e
Reviewed-on: https://chromium-review.googlesource.com/1043271
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556319}

chrome/browser/extensions/api/declarative_net_request/declarative_net_request_browsertest.cc

@@ -31,7 +31,6 @@
 #include "components/proxy_config/proxy_config_pref_names.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_entry.h"
-#include "content/public/browser/notification_source.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/test_utils.h"
@@ -44,13 +43,13 @@
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
 #include "extensions/browser/extension_util.h"
-#include "extensions/browser/notification_types.h"
 #include "extensions/browser/runtime_data.h"
 #include "extensions/common/api/declarative_net_request/constants.h"
 #include "extensions/common/api/declarative_net_request/test_utils.h"
 #include "extensions/common/constants.h"
 #include "extensions/common/extension_id.h"
 #include "extensions/common/url_pattern.h"
+#include "extensions/test/extension_test_message_listener.h"
 #include "net/dns/mock_host_resolver.h"
 #include "net/test/spawned_test_server/spawned_test_server.h"
 #include "net/test/test_data_directory.h"
@@ -184,6 +183,7 @@ class DeclarativeNetRequestBrowserTest
                             kJSONRulesFilename, rules, hosts,
                             has_background_script_);
 
+    ExtensionTestMessageListener listener("ready", false /*will_reply*/);
     const Extension* extension = nullptr;
     switch (GetParam()) {
       case ExtensionLoadType::PACKED:
@@ -201,6 +201,10 @@ class DeclarativeNetRequestBrowserTest
     // Ensure the ruleset is also loaded on the IO thread.
     content::RunAllTasksUntilIdle();
 
+    // Wait for the background page to load if needed.
+    if (has_background_script_)
+      WaitForBackgroundScriptToLoad(&listener, extension->id());
+
     // Ensure no load errors were reported.
     EXPECT_TRUE(LoadErrorReporter::GetInstance()->GetErrors()->empty());
 
@@ -220,6 +224,12 @@ class DeclarativeNetRequestBrowserTest
                            {URLPattern::kAllUrlsPattern});
   }
 
+  void WaitForBackgroundScriptToLoad(ExtensionTestMessageListener* listener,
+                                     const ExtensionId& extension_id) {
+    ASSERT_TRUE(listener->WaitUntilSatisfied());
+    ASSERT_EQ(extension_id, listener->extension_id_for_message());
+  }
+
  private:
   base::ScopedTempDir temp_dir_;
   bool has_background_script_ = false;
@@ -1137,15 +1147,6 @@ IN_PROC_BROWSER_TEST_P(DeclarativeNetRequestBrowserTest_Packed,
   ASSERT_TRUE(dnr_extension);
   EXPECT_EQ("Test extension", dnr_extension->name());
 
-  // Ensure the background page is ready before dispatching the script to it.
-  if (!ExtensionSystem::Get(profile())->runtime_data()->IsBackgroundPageReady(
-          dnr_extension)) {
-    content::WindowedNotificationObserver(
-        NOTIFICATION_EXTENSION_BACKGROUND_PAGE_READY,
-        content::Source<Extension>(dnr_extension))
-        .Wait();
-  }
-
   // Whitelist "https://www.google.com/".
   const char* script1 = R"(
     chrome.declarativeNetRequest.addWhitelistedPages(
@@ -1191,12 +1192,14 @@ IN_PROC_BROWSER_TEST_P(DeclarativeNetRequestBrowserTest_Packed,
   ASSERT_TRUE(dnr_extension);
 
   // Ensure the background page is ready before dispatching the script to it.
+  // TODO(karandeepb): Remove the need to check IsBackgroundPageReady by
+  // creating the ExtensionTestMessageListener early. This should also ensure
+  // that the we start listening for messages from extension early enough to
+  // avoid any races. See crbug.com/838536.
   if (!ExtensionSystem::Get(profile())->runtime_data()->IsBackgroundPageReady(
           dnr_extension)) {
-    content::WindowedNotificationObserver(
-        NOTIFICATION_EXTENSION_BACKGROUND_PAGE_READY,
-        content::Source<Extension>(dnr_extension))
-        .Wait();
+    ExtensionTestMessageListener listener("ready", false /*will_reply*/);
+    WaitForBackgroundScriptToLoad(&listener, dnr_extension->id());
   }
 
   const char* script1 = R"(

extensions/common/api/declarative_net_request/test_utils.cc

@@ -156,9 +156,9 @@ void WriteManifestAndRuleset(
   JSONFileValueSerializer(extension_dir.Append(json_rules_filepath))
       .Serialize(rules);
 
-  // Persists an empty background script if needed.
+  // Persists a background script if needed.
   if (has_background_script) {
-    std::string content;
+    std::string content = "chrome.test.sendMessage('ready');";
     CHECK_EQ(static_cast<int>(content.length()),
              base::WriteFile(extension_dir.Append(kBackgroundScriptFilepath),
                              content.c_str(), content.length()));

extensions/common/api/declarative_net_request/test_utils.h

@@ -84,8 +84,10 @@ std::unique_ptr<base::ListValue> ToListValue(
 
 // Writes the declarative |rules| in the given |extension_dir| together with the
 // manifest file. |hosts| specifies the host permissions, the extensions should
-// have. If |has_background_script| is true, an empty background script
-// ("background.js") will also be persisted for the extension.
+// have. If |has_background_script| is true, a background script
+// ("background.js") will also be persisted for the extension. Clients can
+// listen in to the "ready" message from the background page to detect its
+// loading.
 void WriteManifestAndRuleset(
     const base::FilePath& extension_dir,
     const base::FilePath::CharType* json_rules_filepath,

extensions/test/extension_test_message_listener.cc

@@ -76,6 +76,7 @@ void ExtensionTestMessageListener::Reset() {
   satisfied_ = false;
   failed_ = false;
   message_.clear();
+  extension_id_for_message_.clear();
   replied_ = false;
 }
 
@@ -89,13 +90,19 @@ void ExtensionTestMessageListener::Observe(
   // extension.
   extensions::TestSendMessageFunction* function =
       content::Source<extensions::TestSendMessageFunction>(source).ptr();
+
+  std::string sender_extension_id;
+  if (function->extension())
+    sender_extension_id = function->extension_id();
+
   if (satisfied_ ||
-      (!extension_id_.empty() && function->extension_id() != extension_id_)) {
+      (!extension_id_.empty() && sender_extension_id != extension_id_)) {
     return;
   }
 
   // We should have an empty message if we're not already satisfied.
   CHECK(message_.empty());
+  CHECK(extension_id_for_message_.empty());
 
   std::pair<std::string, bool*>* message_details =
       content::Details<std::pair<std::string, bool*>>(details).ptr();
@@ -106,6 +113,7 @@ void ExtensionTestMessageListener::Observe(
     // empty string.
     *message_details->second = true;
     message_ = message;
+    extension_id_for_message_ = sender_extension_id;
     satisfied_ = true;
     failed_ = (message_ == failure_message_);
 

extensions/test/extension_test_message_listener.h

@@ -11,6 +11,7 @@
 #include "base/memory/ref_counted.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
+#include "extensions/common/extension_id.h"
 
 namespace extensions {
 class TestSendMessageFunction;
@@ -94,7 +95,8 @@ class ExtensionTestMessageListener : public content::NotificationObserver {
   ~ExtensionTestMessageListener() override;
 
   // This returns true immediately if we've already gotten the expected
-  // message, or waits until it arrives.
+  // message, or waits until it arrives. Once this returns true, message() and
+  // extension_id_for_message() accessors can be used.
   // Returns false if the wait is interrupted and we still haven't gotten the
   // message, or if the message was equal to |failure_message_|.
   bool WaitUntilSatisfied() WARN_UNUSED_RESULT;
@@ -127,6 +129,10 @@ class ExtensionTestMessageListener : public content::NotificationObserver {
 
   const std::string& message() const { return message_; }
 
+  const extensions::ExtensionId& extension_id_for_message() const {
+    return extension_id_for_message_;
+  }
+
  private:
   // Implements the content::NotificationObserver interface.
   void Observe(int type,
@@ -167,6 +173,9 @@ class ExtensionTestMessageListener : public content::NotificationObserver {
   // If we received a message that was the failure message.
   bool failed_;
 
+  // The extension id from which |message_| was received.
+  extensions::ExtensionId extension_id_for_message_;
+
   // The function we need to reply to.
   scoped_refptr<extensions::TestSendMessageFunction> function_;
 };