Finish the implementation of chrome.runtime.getBackgroundClient() for extension service workers.

Until now it assumed that the extension's background page was open. Now it uses
WakeEventPage IPC call to wake the event page if it's asleep. This required
making WakeEventPage thread safe, to be able to be called on a service worker
thread, then when the response comes from the browser, call back on that same
thread.

It was also incorrectly called chrome.getBackgroundClient() rather than
chrome.runtime.getBackgroundClient().

I also did a rewrite of the extension service worker tests to be better
reusable, and easier to add more tests. It was hard to test a closed event page
in the current framework. Then I added more tests.

BUG=501569, 532720
R=rdevlin.cronin@chromium.org

Review URL: https://codereview.chromium.org/1344243003

Cr-Commit-Position: refs/heads/master@{#349700}

4 files changed, 121 insertions, 33 deletions

diff --git a/extensions/renderer/dispatcher.cc b/extensions/renderer/dispatcher.cc
index c9dfb9c..d69eb59 100644
--- a/extensions/renderer/dispatcher.cc
+++ b/extensions/renderer/dispatcher.cc
@@ -5,6 +5,7 @@
 #include "extensions/renderer/dispatcher.h"
 
 #include "base/bind.h"
+#include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/command_line.h"
 #include "base/containers/scoped_ptr_map.h"
@@ -384,17 +385,34 @@ void Dispatcher::DidInitializeServiceWorkerContextOnWorkerThread(
   v8::Local<v8::String> script = v8::String::NewExternal(
       isolate, new StaticV8ExternalOneByteStringResource(script_resource));
 
-  // Run the script to get the main function, then run the main function to
-  // inject service worker bindings.
-  v8::Local<v8::Value> result = context->RunScript(
-      v8_helpers::ToV8StringUnsafe(isolate, "service_worker"), script,
-      base::Bind(&CrashOnException));
-  CHECK(result->IsFunction());
+  // Run service_worker.js to get the main function.
+  v8::Local<v8::Function> main_function;
+  {
+    v8::Local<v8::Value> result = context->RunScript(
+        v8_helpers::ToV8StringUnsafe(isolate, "service_worker"), script,
+        base::Bind(&CrashOnException));
+    CHECK(result->IsFunction());
+    main_function = result.As<v8::Function>();
+  }
+
+  // Expose CHECK/DCHECK/NOTREACHED to the main function with a
+  // LoggingNativeHandler. Admire the neat base::Bind trick to both Invalidate
+  // and delete the native handler.
+  LoggingNativeHandler* logging = new LoggingNativeHandler(context);
+  context->AddInvalidationObserver(
+      base::Bind(&NativeHandler::Invalidate, base::Owned(logging)));
+
+  // Execute the main function with its dependencies passed in as arguments.
   v8::Local<v8::Value> args[] = {
+      // The extension's background URL.
       v8_helpers::ToV8StringUnsafe(
           isolate, BackgroundInfo::GetBackgroundURL(extension).spec()),
+      // The wake-event-page native function.
+      WakeEventPage::Get()->GetForContext(context),
+      // The logging module.
+      logging->NewInstance(),
   };
-  context->CallFunction(result.As<v8::Function>(), arraysize(args), args);
+  context->CallFunction(main_function, arraysize(args), args);
 
   const base::TimeDelta elapsed = base::TimeTicks::Now() - start_time;
   UMA_HISTOGRAM_TIMES(
diff --git a/extensions/renderer/resources/service_worker_bindings.js b/extensions/renderer/resources/service_worker_bindings.js
index bc8cfd6b..532f51d 100644
--- a/extensions/renderer/resources/service_worker_bindings.js
+++ b/extensions/renderer/resources/service_worker_bindings.js
@@ -2,25 +2,66 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-(function(backgroundUrl) {
+// This function is returned to DidInitializeServiceWorkerContextOnWorkerThread
+// then executed, passing in dependencies as function arguments.
+//
+// |backgroundUrl| is the URL of the extension's background page.
+// |wakeEventPage| is a function that wakes up the current extension's event
+// page, then runs its callback on completion or failure.
+// |logging| is an object equivalent to a subset of base/debug/logging.h, with
+// CHECK/DCHECK/etc.
+(function(backgroundUrl, wakeEventPage, logging) {
   'use strict';
-
   self.chrome = self.chrome || {};
+  self.chrome.runtime = self.chrome.runtime || {};
 
-  self.chrome.getBackgroundClient = function() { return new Promise(
-   function(resolve, reject) {
-     self.clients.matchAll({
+  // Returns a Promise that resolves to the background page's client, or null
+  // if there is no background client.
+  function findBackgroundClient() {
+    return self.clients.matchAll({
       includeUncontrolled: true,
       type: 'window'
-     }).then(function(clients) {
-      for (let client of clients) {
-         if (client.url == backgroundUrl) {
-           resolve(client);
-           return;
-         }
-       }
-       reject("BackgroundClient ('" + backgroundUrl + "') does not exist.")
-     })
+    }).then(function(clients) {
+      return clients.find(function(client) {
+        return client.url == backgroundUrl;
+      });
+    });
+  }
+
+  // Returns a Promise wrapper around wakeEventPage, that resolves on success,
+  // or rejects on failure.
+  function makeWakeEventPagePromise() {
+    return new Promise(function(resolve, reject) {
+      wakeEventPage(function(success) {
+        if (success)
+          resolve();
+        else
+          reject('Failed to start background client "' + backgroundUrl + '"');
+      });
     });
   }
+
+  // The chrome.runtime.getBackgroundClient function is documented in
+  // runtime.json. It returns a Promise that resolves to the background page's
+  // client, or is rejected if there is no background client or if the
+  // background client failed to wake.
+  self.chrome.runtime.getBackgroundClient = function() {
+    return findBackgroundClient().then(function(client) {
+      if (client) {
+        // Background client is already awake, or it was persistent.
+        return client;
+      }
+
+      // Event page needs to be woken.
+      return makeWakeEventPagePromise().then(function() {
+        return findBackgroundClient();
+      }).then(function(client) {
+        if (!client) {
+          return Promise.reject(
+            'Background client "' + backgroundUrl + '" not found');
+        }
+        return client;
+      });
+    });
+  };
 });
diff --git a/extensions/renderer/wake_event_page.cc b/extensions/renderer/wake_event_page.cc
index 1bc4304..7a64d1a 100644
--- a/extensions/renderer/wake_event_page.cc
+++ b/extensions/renderer/wake_event_page.cc
@@ -7,8 +7,10 @@
 #include "base/atomic_sequence_num.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
+#include "base/lazy_instance.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
+#include "content/public/child/worker_thread.h"
 #include "content/public/renderer/render_thread.h"
 #include "extensions/common/extension_messages.h"
 #include "extensions/renderer/object_backed_native_handler.h"
@@ -124,7 +126,8 @@ v8::Local<v8::Function> WakeEventPage::GetForContext(ScriptContext* context) {
     const char* kFunctionName = "WakeEventPage";
     WakeEventPageNativeHandler* native_handler = new WakeEventPageNativeHandler(
         context, kFunctionName, base::Bind(&WakeEventPage::MakeRequest,
-                                           weak_ptr_factory_.GetWeakPtr()));
+                                           // Safe, owned by a LazyInstance.
+                                           base::Unretained(this)));
 
     // Extract and cache the wake-event-page function from the native handler.
     wake_event_page = GetPropertyUnsafe(
@@ -136,12 +139,13 @@ v8::Local<v8::Function> WakeEventPage::GetForContext(ScriptContext* context) {
   return handle_scope.Escape(wake_event_page.As<v8::Function>());
 }
 
-WakeEventPage::RequestData::RequestData(const OnResponseCallback& on_response)
-    : on_response(on_response) {}
+WakeEventPage::RequestData::RequestData(int thread_id,
+                                        const OnResponseCallback& on_response)
+    : thread_id(thread_id), on_response(on_response) {}
 
 WakeEventPage::RequestData::~RequestData() {}
 
-WakeEventPage::WakeEventPage() : weak_ptr_factory_(this) {}
+WakeEventPage::WakeEventPage() {}
 
 WakeEventPage::~WakeEventPage() {}
 
@@ -149,7 +153,12 @@ void WakeEventPage::MakeRequest(const std::string& extension_id,
                                 const OnResponseCallback& on_response) {
   static base::AtomicSequenceNumber sequence_number;
   int request_id = sequence_number.GetNext();
-  requests_.set(request_id, make_scoped_ptr(new RequestData(on_response)));
+  {
+    scoped_ptr<RequestData> request_data(
+        new RequestData(content::WorkerThread::GetCurrentId(), on_response));
+    base::AutoLock lock(requests_lock_);
+    requests_.set(request_id, request_data.Pass());
+  }
   message_filter_->Send(
       new ExtensionHostMsg_WakeEventPage(request_id, extension_id));
 }
@@ -165,9 +174,21 @@ bool WakeEventPage::OnControlMessageReceived(const IPC::Message& message) {
 }
 
 void WakeEventPage::OnWakeEventPageResponse(int request_id, bool success) {
-  scoped_ptr<RequestData> request_data = requests_.take(request_id);
-  CHECK(request_data);
-  request_data->on_response.Run(success);
+  scoped_ptr<RequestData> request_data;
+  {
+    base::AutoLock lock(requests_lock_);
+    request_data = requests_.take(request_id);
+  }
+  CHECK(request_data) << "No request with ID " << request_id;
+  if (request_data->thread_id == 0) {
+    // Thread ID of 0 means it wasn't called on a worker thread, so safe to
+    // call immediately.
+    request_data->on_response.Run(success);
+  } else {
+    content::WorkerThread::PostTask(
+        request_data->thread_id,
+        base::Bind(request_data->on_response, success));
+  }
 }
 
 }  //  namespace extensions
diff --git a/extensions/renderer/wake_event_page.h b/extensions/renderer/wake_event_page.h
index 38ceda4f..48bfd4b 100644
--- a/extensions/renderer/wake_event_page.h
+++ b/extensions/renderer/wake_event_page.h
@@ -9,11 +9,11 @@
 
 #include "base/callback.h"
 #include "base/containers/scoped_ptr_hash_map.h"
-#include "base/lazy_instance.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
+#include "base/synchronization/lock.h"
 #include "content/public/renderer/render_process_observer.h"
 #include "ipc/ipc_sync_message_filter.h"
 #include "v8/include/v8.h"
@@ -72,8 +72,14 @@ class WakeEventPage : public content::RenderProcessObserver {
 
   // For |requests_|.
   struct RequestData {
-    explicit RequestData(const OnResponseCallback& on_response);
+    RequestData(int thread_id, const OnResponseCallback& on_response);
     ~RequestData();
+
+    // The thread ID the request was made on. |on_response| must be called on
+    // that thread.
+    int thread_id;
+
+    // Callback to run when the response to the request arrives.
     OnResponseCallback on_response;
   };
 
@@ -95,10 +101,12 @@ class WakeEventPage : public content::RenderProcessObserver {
   // IPC sender. Belongs to the render thread, but thread safe.
   scoped_refptr<IPC::SyncMessageFilter> message_filter_;
 
-  // All in-flight requests, keyed by request ID.
+  // All in-flight requests, keyed by request ID. Used on multiple threads, so
+  // must be guarded by |requests_lock_|.
   base::ScopedPtrHashMap<int, scoped_ptr<RequestData>> requests_;
 
-  base::WeakPtrFactory<WakeEventPage> weak_ptr_factory_;
+  // Lock for |requests_|.
+  base::Lock requests_lock_;
 
   DISALLOW_COPY_AND_ASSIGN(WakeEventPage);
 };