[Extension] Create lazy background host if there are pending requests.

A pending request could get added to LazyBackgroundTaskQueue while the
extension is not enabled (e.g. if it crashed and is waiting to be
reloaded). This appears to be by design. In such a case we do not
attempt to create the background host. The request (and subsequent
requests) could become stuck if the background host does not get
created.

This patch changes LazyBackgroundTaskQueue to listen for extension
loaded, and creates a background host if there are pending requests.

This patch also makes sure we only create a PendingTasksList entry if
we are enqueueing a request to it.

Bug: 835017
Change-Id: Ie2aff8cfb620a1867b033c3474a3277a283fb258
Reviewed-on: https://chromium-review.googlesource.com/1058083Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: Derek Cheng <imcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560303}

chrome/browser/media/router/event_page_request_manager.cc

@@ -141,6 +141,11 @@ void EventPageRequestManager::AttemptWakeEventPage() {
 }
 
 void EventPageRequestManager::OnWakeComplete(bool success) {
+  // If there are multiple overlapping WakeEventPage requests, ensure the
+  // metrics are only recorded once.
+  if (current_wake_reason_ == MediaRouteProviderWakeReason::TOTAL_COUNT)
+    return;
+
   if (success) {
     MediaRouterMojoMetrics::RecordMediaRouteProviderWakeReason(
         current_wake_reason_);

extensions/browser/lazy_background_task_queue.cc

@@ -42,6 +42,16 @@ void PendingTaskAdapter(LazyContextTaskQueue::PendingTask original_task,
   }
 }
 
+// Attempts to create a background host for a lazy background page. Returns true
+// if the background host is created.
+bool CreateLazyBackgroundHost(ProcessManager* pm, const Extension* extension) {
+  pm->IncrementLazyKeepaliveCount(extension);
+  // Creating the background host may fail, e.g. if the extension isn't enabled
+  // in incognito mode.
+  return pm->CreateBackgroundHost(extension,
+                                  BackgroundInfo::GetBackgroundURL(extension));
+}
+
 }  // namespace
 
 LazyBackgroundTaskQueue::LazyBackgroundTaskQueue(
@@ -104,26 +114,21 @@ void LazyBackgroundTaskQueue::AddPendingTask(
   PendingTasksKey key(browser_context, extension_id);
   PendingTasksMap::iterator it = pending_tasks_.find(key);
   if (it == pending_tasks_.end()) {
-    auto tasks_list_tmp = std::make_unique<PendingTasksList>();
-    tasks_list = tasks_list_tmp.get();
-    pending_tasks_[key] = std::move(tasks_list_tmp);
-
-    const Extension* extension =
-        ExtensionRegistry::Get(browser_context)->enabled_extensions().GetByID(
-            extension_id);
+    const Extension* extension = ExtensionRegistry::Get(browser_context)
+                                     ->enabled_extensions()
+                                     .GetByID(extension_id);
     if (extension && BackgroundInfo::HasLazyBackgroundPage(extension)) {
       // If this is the first enqueued task, and we're not waiting for the
       // background page to unload, ensure the background page is loaded.
-      ProcessManager* pm = ProcessManager::Get(browser_context);
-      pm->IncrementLazyKeepaliveCount(extension);
-      // Creating the background host may fail, e.g. if |profile| is incognito
-      // but the extension isn't enabled in incognito mode.
-      if (!pm->CreateBackgroundHost(
-            extension, BackgroundInfo::GetBackgroundURL(extension))) {
+      if (!CreateLazyBackgroundHost(ProcessManager::Get(browser_context),
+                                    extension)) {
         std::move(task).Run(nullptr);
         return;
       }
     }
+    auto tasks_list_tmp = std::make_unique<PendingTasksList>();
+    tasks_list = tasks_list_tmp.get();
+    pending_tasks_[key] = std::move(tasks_list_tmp);
   } else {
     tasks_list = it->second.get();
   }
@@ -158,14 +163,28 @@ void LazyBackgroundTaskQueue::ProcessPendingTasks(
 
   pending_tasks_.erase(key);
 
-  // Balance the keepalive in AddPendingTask. Note we don't do this on a
-  // failure to load, because the keepalive count is reset in that case.
+  // Balance the keepalive in CreateLazyBackgroundHost. Note we don't do this on
+  // a failure to load, because the keepalive count is reset in that case.
   if (host && BackgroundInfo::HasLazyBackgroundPage(extension)) {
     ProcessManager::Get(browser_context)
         ->DecrementLazyKeepaliveCount(extension);
   }
 }
 
+void LazyBackgroundTaskQueue::NotifyTasksExtensionFailedToLoad(
+    content::BrowserContext* browser_context,
+    const Extension* extension) {
+  ProcessPendingTasks(nullptr, browser_context, extension);
+  // If this extension is also running in an off-the-record context, notify that
+  // task queue as well.
+  ExtensionsBrowserClient* browser_client = ExtensionsBrowserClient::Get();
+  if (browser_client->HasOffTheRecordContext(browser_context)) {
+    ProcessPendingTasks(nullptr,
+                        browser_client->GetOffTheRecordContext(browser_context),
+                        extension);
+  }
+}
+
 void LazyBackgroundTaskQueue::Observe(
     int type,
     const content::NotificationSource& source,
@@ -203,20 +222,48 @@ void LazyBackgroundTaskQueue::Observe(
   }
 }
 
-void LazyBackgroundTaskQueue::OnExtensionUnloaded(
+void LazyBackgroundTaskQueue::OnExtensionLoaded(
     content::BrowserContext* browser_context,
-    const Extension* extension,
-    UnloadedExtensionReason reason) {
-  // Notify consumers that the page failed to load.
-  ProcessPendingTasks(NULL, browser_context, extension);
-  // If this extension is also running in an off-the-record context, notify that
-  // task queue as well.
+    const Extension* extension) {
+  // If there are pending tasks for a lazy background page, and its background
+  // host has not been created yet, then create it. This can happen if a pending
+  // task was added while the extension is not yet enabled (e.g., component
+  // extension crashed and waiting to reload, https://crbug.com/835017).
+  if (!BackgroundInfo::HasLazyBackgroundPage(extension))
+    return;
+
+  CreateLazyBackgroundHostOnExtensionLoaded(browser_context, extension);
+
+  // Also try to create the background host for the off-the-record context.
   ExtensionsBrowserClient* browser_client = ExtensionsBrowserClient::Get();
   if (browser_client->HasOffTheRecordContext(browser_context)) {
-    ProcessPendingTasks(NULL,
-                        browser_client->GetOffTheRecordContext(browser_context),
-                        extension);
+    CreateLazyBackgroundHostOnExtensionLoaded(
+        browser_client->GetOffTheRecordContext(browser_context), extension);
   }
 }
 
+void LazyBackgroundTaskQueue::OnExtensionUnloaded(
+    content::BrowserContext* browser_context,
+    const Extension* extension,
+    UnloadedExtensionReason reason) {
+  NotifyTasksExtensionFailedToLoad(browser_context, extension);
+}
+
+void LazyBackgroundTaskQueue::CreateLazyBackgroundHostOnExtensionLoaded(
+    content::BrowserContext* browser_context,
+    const Extension* extension) {
+  PendingTasksKey key(browser_context, extension->id());
+  if (!base::ContainsKey(pending_tasks_, key))
+    return;
+
+  ProcessManager* pm = ProcessManager::Get(browser_context);
+
+  // Background host already created, just wait for it to finish loading.
+  if (pm->GetBackgroundHostForExtension(extension->id()))
+    return;
+
+  if (!CreateLazyBackgroundHost(pm, extension))
+    ProcessPendingTasks(nullptr, browser_context, extension);
+}
+
 }  // namespace extensions

extensions/browser/lazy_background_task_queue.h

@@ -76,7 +76,8 @@ class LazyBackgroundTaskQueue : public KeyedService,
  private:
   FRIEND_TEST_ALL_PREFIXES(LazyBackgroundTaskQueueTest, AddPendingTask);
   FRIEND_TEST_ALL_PREFIXES(LazyBackgroundTaskQueueTest, ProcessPendingTasks);
-
+  FRIEND_TEST_ALL_PREFIXES(LazyBackgroundTaskQueueTest,
+                           CreateLazyBackgroundPageOnExtensionLoaded);
   // A map between a BrowserContext/extension_id pair and the queue of tasks
   // pending the load of its background page.
   using PendingTasksKey = std::pair<content::BrowserContext*, ExtensionId>;
@@ -90,17 +91,31 @@ class LazyBackgroundTaskQueue : public KeyedService,
                const content::NotificationDetails& details) override;
 
   // ExtensionRegistryObserver interface.
+  void OnExtensionLoaded(content::BrowserContext* browser_context,
+                         const Extension* extension) override;
   void OnExtensionUnloaded(content::BrowserContext* browser_context,
                            const Extension* extension,
                            UnloadedExtensionReason reason) override;
 
+  // If there are pending tasks for |extension| in |browser_context|, try to
+  // create the background host. If the background host cannot be created, the
+  // pending tasks are invoked with nullptr.
+  void CreateLazyBackgroundHostOnExtensionLoaded(
+      content::BrowserContext* browser_context,
+      const Extension* extension);
+
   // Called when a lazy background page has finished loading, or has failed to
-  // load (host is NULL in that case). All enqueued tasks are run in order.
+  // load (host is nullptr in that case). All enqueued tasks are run in order.
   void ProcessPendingTasks(
       ExtensionHost* host,
       content::BrowserContext* context,
       const Extension* extension);
 
+  // Notifies queued tasks that a lazy background page has failed to load.
+  void NotifyTasksExtensionFailedToLoad(
+      content::BrowserContext* browser_context,
+      const Extension* extension);
+
   content::BrowserContext* browser_context_;
   content::NotificationRegistrar registrar_;
   PendingTasksMap pending_tasks_;

extensions/browser/lazy_background_task_queue_unittest.cc

@@ -70,6 +70,7 @@ class LazyBackgroundTaskQueueTest : public ExtensionsTest {
   ~LazyBackgroundTaskQueueTest() override {}
 
   int task_run_count() { return task_run_count_; }
+  TestProcessManager* process_manager() { return process_manager_; }
 
   // A simple callback for AddPendingTask.
   void RunPendingTask(ExtensionHost* host) {
@@ -101,6 +102,15 @@ class LazyBackgroundTaskQueueTest : public ExtensionsTest {
   void SetUp() override {
     ExtensionsTest::SetUp();
     user_prefs::UserPrefs::Set(browser_context(), &testing_pref_service_);
+
+    process_manager_ = static_cast<TestProcessManager*>(
+        ProcessManagerFactory::GetInstance()->SetTestingFactoryAndUse(
+            browser_context(), CreateTestProcessManager));
+  }
+
+  void TearDown() override {
+    process_manager_ = nullptr;
+    ExtensionsTest::TearDown();
   }
 
  private:
@@ -108,6 +118,7 @@ class LazyBackgroundTaskQueueTest : public ExtensionsTest {
 
   // The total number of pending tasks that have been executed.
   int task_run_count_;
+  TestProcessManager* process_manager_ = nullptr;
 
   DISALLOW_COPY_AND_ASSIGN(LazyBackgroundTaskQueueTest);
 };
@@ -129,11 +140,6 @@ TEST_F(LazyBackgroundTaskQueueTest, ShouldEnqueueTask) {
 // Tests that adding tasks actually increases the pending task count, and that
 // multiple extensions can have pending tasks.
 TEST_F(LazyBackgroundTaskQueueTest, AddPendingTask) {
-  // Get our TestProcessManager.
-  TestProcessManager* process_manager = static_cast<TestProcessManager*>(
-      ProcessManagerFactory::GetInstance()->SetTestingFactoryAndUse(
-          browser_context(), CreateTestProcessManager));
-
   LazyBackgroundTaskQueue queue(browser_context());
 
   // Build a simple extension with no background page.
@@ -164,9 +170,9 @@ TEST_F(LazyBackgroundTaskQueueTest, AddPendingTask) {
                        lazy_background->id(),
                        base::Bind(&LazyBackgroundTaskQueueTest::RunPendingTask,
                                   base::Unretained(this)));
-  EXPECT_EQ(2u, queue.pending_tasks_.size());
+  EXPECT_EQ(1u, queue.pending_tasks_.size());
   // The process manager tried to create a background host.
-  EXPECT_EQ(1, process_manager->create_count());
+  EXPECT_EQ(1, process_manager()->create_count());
   // The task ran immediately because the creation failed.
   EXPECT_EQ(1, task_run_count());
 }
@@ -201,4 +207,41 @@ TEST_F(LazyBackgroundTaskQueueTest, ProcessPendingTasks) {
   EXPECT_EQ(0u, queue.pending_tasks_.size());
 }
 
+// Tests that if a pending task was added before the extension with a lazy
+// background page is loaded, then we will create the lazy background page when
+// the extension is loaded.
+TEST_F(LazyBackgroundTaskQueueTest, CreateLazyBackgroundPageOnExtensionLoaded) {
+  LazyBackgroundTaskQueue queue(browser_context());
+
+  scoped_refptr<Extension> lazy_background =
+      ExtensionBuilder("Lazy background")
+          .SetBackgroundPage(ExtensionBuilder::BackgroundPage::EVENT)
+          .SetID("bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb")
+          .Build();
+
+  queue.OnExtensionLoaded(browser_context(), lazy_background.get());
+  // Did not try to create a background host because there are no queued tasks.
+  EXPECT_EQ(0, process_manager()->create_count());
+
+  queue.AddPendingTask(browser_context(), lazy_background->id(),
+                       base::Bind(&LazyBackgroundTaskQueueTest::RunPendingTask,
+                                  base::Unretained(this)));
+  EXPECT_EQ(1u, queue.pending_tasks_.size());
+  // Did not try to create a background host because extension is not yet
+  // loaded.
+  EXPECT_EQ(0, process_manager()->create_count());
+  // The task is queued.
+  EXPECT_EQ(0, task_run_count());
+
+  ExtensionRegistry::Get(browser_context())->AddEnabled(lazy_background);
+  queue.OnExtensionLoaded(browser_context(), lazy_background.get());
+
+  // The process manager tried to create a background host because there is a
+  // queued task.
+  EXPECT_EQ(1, process_manager()->create_count());
+  // The queued task ran because the creation failed.
+  EXPECT_EQ(1, task_run_count());
+  EXPECT_EQ(0u, queue.pending_tasks_.size());
+}
+
 }  // namespace extensions