[Extensions] Enable the pageCapture API for SW-based extensions.

Bug: 1093066
Change-Id: I2cfb8124ee962c57c253771e10538e993337f85c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2354829Reviewed-by: Greg Kerr <kerrnel@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Commit-Queue: David Bertoni <dbertoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#804345}

chrome/browser/extensions/api/page_capture/page_capture_api.cc

@@ -98,14 +98,16 @@ ExtensionFunction::ResponseAction PageCaptureSaveAsMHTMLFunction::Run() {
   // renderer has it's reference, so we can release ours.
   // TODO(crbug.com/1050887): Potential memory leak here.
   AddRef();  // Balanced in OnMessageReceived()
+  if (is_from_service_worker())
+    AddWorkerResponseTarget();
 
+#if defined(OS_CHROMEOS)
   // In Public Sessions, extensions (and apps) are force-installed by admin
   // policy so the user does not get a chance to review the permissions for
   // these extensions. This is not acceptable from a security/privacy
   // standpoint, so when an extension uses the PageCapture API for the first
-  // time, we show the user a dialog where they can choose whether to allow the
-  // extension access to the API.
-#if defined(OS_CHROMEOS)
+  // time, we show the user a dialog where they can choose whether to allow
+  // the extension access to the API.
   if (profiles::ArePublicSessionRestrictionsEnabled()) {
     WebContents* web_contents = GetWebContents();
     if (!web_contents) {
@@ -184,6 +186,15 @@ bool PageCaptureSaveAsMHTMLFunction::OnMessageReceived(
   return true;
 }
 
+void PageCaptureSaveAsMHTMLFunction::OnServiceWorkerAck() {
+  DCHECK(is_from_service_worker());
+  // The extension process has processed the response and has created a
+  // reference to the blob, it is safe for us to go away.
+  // This instance may be deleted after this call, so no code goes after
+  // this!!!
+  Release();  // Balanced in Run()
+}
+
 #if defined(OS_CHROMEOS)
 void PageCaptureSaveAsMHTMLFunction::ResolvePermissionRequest(
     const PermissionIDSet& allowed_permissions) {
@@ -281,7 +292,7 @@ void PageCaptureSaveAsMHTMLFunction::ReturnSuccess(int64_t file_size) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   WebContents* web_contents = GetWebContents();
-  if (!web_contents || !render_frame_host()) {
+  if (!web_contents) {
     ReturnFailure(kTabClosedError);
     return;
   }

chrome/browser/extensions/api/page_capture/page_capture_api.h

@@ -42,6 +42,9 @@ class PageCaptureSaveAsMHTMLFunction : public ExtensionFunction {
   };
   static void SetTestDelegate(TestDelegate* delegate);
 
+  // ExtensionFunction:
+  void OnServiceWorkerAck() override;
+
  private:
   ~PageCaptureSaveAsMHTMLFunction() override;
   ResponseAction Run() override;

chrome/browser/extensions/api/page_capture/page_capture_apitest.cc

@@ -24,6 +24,7 @@
 #include "extensions/browser/extension_dialog_auto_confirm.h"
 #include "extensions/common/permissions/permission_set.h"
 #include "extensions/common/permissions/permissions_data.h"
+#include "extensions/common/scoped_worker_based_extensions_channel.h"
 #include "extensions/common/url_pattern_set.h"
 #include "extensions/test/extension_test_message_listener.h"
 #include "extensions/test/result_catcher.h"
@@ -33,23 +34,9 @@
 #include "chromeos/login/login_state/login_state.h"
 #endif  // defined(OS_CHROMEOS)
 
-using extensions::Extension;
-using extensions::ExtensionActionRunner;
-using extensions::PageCaptureSaveAsMHTMLFunction;
-using extensions::ResultCatcher;
-using extensions::ScopedTestDialogAutoConfirm;
+namespace extensions {
 
-class ExtensionPageCaptureApiTest : public extensions::ExtensionApiTest {
- public:
-  void SetUpCommandLine(base::CommandLine* command_line) override {
-    extensions::ExtensionApiTest::SetUpCommandLine(command_line);
-    command_line->AppendSwitchASCII(switches::kJavaScriptFlags, "--expose-gc");
-  }
-  void SetUpOnMainThread() override {
-    extensions::ExtensionApiTest::SetUpOnMainThread();
-    host_resolver()->AddRule("*", "127.0.0.1");
-  }
-};
+using ContextType = ExtensionApiTest::ContextType;
 
 class PageCaptureSaveAsMHTMLDelegate
     : public PageCaptureSaveAsMHTMLFunction::TestDelegate {
@@ -88,44 +75,104 @@ class PageCaptureSaveAsMHTMLDelegate
   std::atomic<int> temp_file_count_{0};
 };
 
-IN_PROC_BROWSER_TEST_F(ExtensionPageCaptureApiTest,
+class ExtensionPageCaptureApiTest
+    : public ExtensionApiTest,
+      public testing::WithParamInterface<ContextType> {
+ public:
+  ExtensionPageCaptureApiTest() {
+    // Service Workers are currently only available on certain channels, so set
+    // the channel for those tests.
+    if (GetParam() == ContextType::kServiceWorker)
+      current_channel_ = std::make_unique<ScopedWorkerBasedExtensionsChannel>();
+  }
+
+  void SetUpCommandLine(base::CommandLine* command_line) override {
+    ExtensionApiTest::SetUpCommandLine(command_line);
+    command_line->AppendSwitchASCII(switches::kJavaScriptFlags, "--expose-gc");
+  }
+
+  void SetUpOnMainThread() override {
+    ExtensionApiTest::SetUpOnMainThread();
+    host_resolver()->AddRule("*", "127.0.0.1");
+  }
+
+  bool RunTest(const std::string& extension_name) {
+    return RunTestWithArg(extension_name, nullptr);
+  }
+
+  bool RunTestWithArg(const std::string& extension_name,
+                      const char* custom_arg) {
+    return RunTestWithFlagsAndArg(extension_name, custom_arg,
+                                  kFlagEnableFileAccess);
+  }
+
+  bool RunTestWithFlagsAndArg(const std::string& extension_name,
+                              const char* custom_arg,
+                              int browser_test_flags) {
+    if (GetParam() == ContextType::kServiceWorker)
+      browser_test_flags |= kFlagRunAsServiceWorkerBasedExtension;
+
+    return RunExtensionTestWithFlagsAndArg(extension_name, custom_arg,
+                                           browser_test_flags, kFlagNone);
+  }
+
+  void WaitForFileCleanup(PageCaptureSaveAsMHTMLDelegate* delegate) {
+    // Garbage collection in SW-based extensions doesn't clean up the temp
+    // file.
+    if (GetParam() != ContextType::kServiceWorker)
+      delegate->WaitForFinalRelease();
+  }
+
+ private:
+  std::unique_ptr<ScopedWorkerBasedExtensionsChannel> current_channel_;
+};
+
+INSTANTIATE_TEST_SUITE_P(PersistentBackground,
+                         ExtensionPageCaptureApiTest,
+                         ::testing::Values(ContextType::kPersistentBackground));
+INSTANTIATE_TEST_SUITE_P(ServiceWorker,
+                         ExtensionPageCaptureApiTest,
+                         ::testing::Values(ContextType::kServiceWorker));
+
+IN_PROC_BROWSER_TEST_P(ExtensionPageCaptureApiTest,
                        SaveAsMHTMLWithoutFileAccess) {
   ASSERT_TRUE(StartEmbeddedTestServer());
   PageCaptureSaveAsMHTMLDelegate delegate;
-  ASSERT_TRUE(RunExtensionTestWithFlagsAndArg(
-      "page_capture", "ONLY_PAGE_CAPTURE_PERMISSION", kFlagNone, kFlagNone))
+  ASSERT_TRUE(RunTestWithFlagsAndArg("page_capture",
+                                     "ONLY_PAGE_CAPTURE_PERMISSION", kFlagNone))
       << message_;
-  EXPECT_EQ(0, delegate.temp_file_count());
+  WaitForFileCleanup(&delegate);
 }
 
-IN_PROC_BROWSER_TEST_F(ExtensionPageCaptureApiTest, SaveAsMHTMLWithFileAccess) {
+IN_PROC_BROWSER_TEST_P(ExtensionPageCaptureApiTest, SaveAsMHTMLWithFileAccess) {
   ASSERT_TRUE(StartEmbeddedTestServer());
   PageCaptureSaveAsMHTMLDelegate delegate;
-  ASSERT_TRUE(RunExtensionTest("page_capture")) << message_;
-  delegate.WaitForFinalRelease();
+  ASSERT_TRUE(RunTest("page_capture")) << message_;
+  WaitForFileCleanup(&delegate);
 }
 
 #if defined(OS_CHROMEOS)
-IN_PROC_BROWSER_TEST_F(ExtensionPageCaptureApiTest,
+IN_PROC_BROWSER_TEST_P(ExtensionPageCaptureApiTest,
                        PublicSessionRequestAllowed) {
   ASSERT_TRUE(StartEmbeddedTestServer());
   PageCaptureSaveAsMHTMLDelegate delegate;
   chromeos::ScopedTestPublicSessionLoginState login_state;
   // Resolve Permission dialog with Allow.
   ScopedTestDialogAutoConfirm auto_confirm(ScopedTestDialogAutoConfirm::ACCEPT);
-  ASSERT_TRUE(RunExtensionTest("page_capture")) << message_;
-  delegate.WaitForFinalRelease();
+  ASSERT_TRUE(RunTest("page_capture")) << message_;
+  WaitForFileCleanup(&delegate);
 }
 
-IN_PROC_BROWSER_TEST_F(ExtensionPageCaptureApiTest,
+IN_PROC_BROWSER_TEST_P(ExtensionPageCaptureApiTest,
                        PublicSessionRequestDenied) {
   ASSERT_TRUE(StartEmbeddedTestServer());
   PageCaptureSaveAsMHTMLDelegate delegate;
   chromeos::ScopedTestPublicSessionLoginState login_state;
   // Resolve Permission dialog with Deny.
   ScopedTestDialogAutoConfirm auto_confirm(ScopedTestDialogAutoConfirm::CANCEL);
-  ASSERT_TRUE(RunExtensionTestWithArg("page_capture", "REQUEST_DENIED"))
-      << message_;
+  ASSERT_TRUE(RunTestWithArg("page_capture", "REQUEST_DENIED")) << message_;
   EXPECT_EQ(0, delegate.temp_file_count());
 }
 #endif  // defined(OS_CHROMEOS)
+
+}  // namespace extensions

chrome/common/extensions/api/_api_features.json

@@ -605,8 +605,7 @@
   },
   "pageCapture": {
     "dependencies": ["permission:pageCapture"],
-    "contexts": ["blessed_extension"],
-    "disallow_for_service_workers": true
+    "contexts": ["blessed_extension"]
   },
   "passwordsPrivate": [{
     "dependencies": ["permission:passwordsPrivate"],

chrome/renderer/extensions/page_capture_custom_bindings.cc

@@ -9,6 +9,7 @@
 #include "content/public/renderer/render_frame.h"
 #include "extensions/common/extension_messages.h"
 #include "extensions/renderer/script_context.h"
+#include "extensions/renderer/worker_thread_dispatcher.h"
 #include "third_party/blink/public/web/web_blob.h"
 #include "v8/include/v8.h"
 
@@ -51,6 +52,10 @@ void PageCaptureCustomBindings::SendResponseAck(
   if (render_frame) {
     render_frame->Send(new ExtensionHostMsg_ResponseAck(
         render_frame->GetRoutingID(), args[0].As<v8::Int32>()->Value()));
+  } else if (context()->IsForServiceWorker()) {
+    WorkerThreadDispatcher::Get()->Send(new ExtensionHostMsg_WorkerResponseAck(
+        args[0].As<v8::Int32>()->Value(),
+        context()->service_worker_version_id()));
   }
 }
 

chrome/test/data/extensions/api_test/page_capture/background.html

-<!--
- * Copyright (c) 2011 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.
--->
-<script src="test.js"></script>

chrome/test/data/extensions/api_test/page_capture/manifest.json

@@ -4,7 +4,8 @@
   "manifest_version": 2,
   "description": "Tests that the pageCapture extension API work.",
   "background": {
-    "page": "background.html"
+    "scripts": ["test.js"],
+    "persistent": true
   },
   "permissions": ["pageCapture", "tabs"]
 }

chrome/test/data/extensions/api_test/page_capture/test.js

@@ -19,10 +19,13 @@ function testPageCapture(data, isFile) {
   var reader = new FileReader();
   // Let's make sure it contains some well known strings.
   reader.onload = function(e) {
-    if (!isFile) {
     var text = e.target.result;
+    if (!isFile) {
       assertTrue(text.indexOf(testUrl) != -1);
       assertTrue(text.indexOf('logo.png') != -1);
+    } else {
+      assertTrue(text.indexOf('app_background_page') != -1);
+      assertTrue(text.indexOf('service_worker') != -1);
     }
     // Run the GC so the blob is deleted.
     setTimeout(function() {
@@ -59,9 +62,10 @@ chrome.test.getConfig(function(config) {
     },
 
     function saveFileUrlAsMHTML() {
+      var captureUrl = config.testDataDirectory + '/';
       chrome.tabs.onUpdated.addListener(function listener(
           tabId, changeInfo, tab) {
-        if (tab.status == 'complete') {
+        if (tab.status == 'complete' && tab.url == captureUrl) {
           chrome.tabs.onUpdated.removeListener(listener);
           chrome.pageCapture.saveAsMHTML(
               {tabId: tab.id}, function(data) {
@@ -79,7 +83,7 @@ chrome.test.getConfig(function(config) {
           });
         }
       });
-      chrome.tabs.create({url: config.testDataDirectory});
+      chrome.tabs.create({url: captureUrl});
     }
   ]);
 });

extensions/browser/extension_function.cc

@@ -321,7 +321,6 @@ void UserGestureForTests::DecrementCount() {
   --count_;
 }
 
-
 }  // namespace
 
 void ExtensionFunction::ResponseValueObject::SetFunctionResults(
@@ -399,6 +398,13 @@ ExtensionFunction::~ExtensionFunction() {
       << name();
 }
 
+void ExtensionFunction::AddWorkerResponseTarget() {
+  DCHECK(is_from_service_worker());
+
+  if (dispatcher())
+    dispatcher()->AddWorkerResponseTarget(this);
+}
+
 bool ExtensionFunction::HasPermission() const {
   Feature::Availability availability =
       ExtensionAPI::GetSharedInstance()->IsAvailable(
@@ -511,6 +517,12 @@ content::WebContents* ExtensionFunction::GetSenderWebContents() {
              : nullptr;
 }
 
+void ExtensionFunction::OnServiceWorkerAck() {
+  // Derived classes must override this if they require and implement an
+  // ACK from the Service Worker.
+  NOTREACHED();
+}
+
 ExtensionFunction::ResponseValue ExtensionFunction::NoArguments() {
   return ResponseValue(
       new ArgumentListResponseValue(this, std::make_unique<base::ListValue>()));

extensions/browser/extension_function.h

@@ -349,6 +349,10 @@ class ExtensionFunction : public base::RefCountedThreadSafe<
   // Same as above, but global. Yuck. Do not add any more uses of this.
   static bool ignore_all_did_respond_for_testing_do_not_use;
 
+  // Called when the service worker in the renderer ACKS the function's
+  // response.
+  virtual void OnServiceWorkerAck();
+
  protected:
   // ResponseValues.
   //
@@ -436,6 +440,10 @@ class ExtensionFunction : public base::RefCountedThreadSafe<
   // RespondLater(), and Respond(...) hasn't already been called.
   void Respond(ResponseValue result);
 
+  // Adds this instance to the set of targets waiting for an ACK from the
+  // renderer.
+  void AddWorkerResponseTarget();
+
   virtual ~ExtensionFunction();
 
   // Called after the response is sent, allowing the function to perform any

extensions/browser/extension_function_dispatcher.cc

@@ -450,6 +450,29 @@ ExtensionFunctionDispatcher::GetVisibleWebContents() const {
       GetAssociatedWebContents();
 }
 
+void ExtensionFunctionDispatcher::AddWorkerResponseTarget(
+    ExtensionFunction* func) {
+  DCHECK(func->is_from_service_worker());
+  worker_response_targets_.insert(func);
+}
+
+void ExtensionFunctionDispatcher::ProcessServiceWorkerResponse(
+    int request_id,
+    int64_t service_worker_version_id) {
+  for (auto it = worker_response_targets_.begin();
+       it != worker_response_targets_.end(); ++it) {
+    ExtensionFunction* func = *it;
+    if (func->request_id() == request_id &&
+        func->service_worker_version_id() == service_worker_version_id) {
+      // Calling this may cause the instance to delete itself, so no
+      // referencing it after this!
+      func->OnServiceWorkerAck();
+      worker_response_targets_.erase(it);
+      break;
+    }
+  }
+}
+
 // static
 scoped_refptr<ExtensionFunction>
 ExtensionFunctionDispatcher::CreateExtensionFunction(

extensions/browser/extension_function_dispatcher.h

@@ -6,6 +6,7 @@
 #define EXTENSIONS_BROWSER_EXTENSION_FUNCTION_DISPATCHER_H_
 
 #include <map>
+#include <set>
 #include <string>
 #include <vector>
 
@@ -97,6 +98,14 @@ class ExtensionFunctionDispatcher
 
   void set_delegate(Delegate* delegate) { delegate_ = delegate; }
 
+  // Adds a function object to the set of objects waiting for
+  // responses from the renderer.
+  void AddWorkerResponseTarget(ExtensionFunction* func);
+
+  // Processes a Service Worker response from a renderer.
+  void ProcessServiceWorkerResponse(int request_id,
+                                    int64_t service_worker_version_id);
+
  private:
   // For a given RenderFrameHost instance, ResponseCallbackWrapper
   // creates ExtensionFunction::ResponseCallback instances which send responses
@@ -152,6 +161,11 @@ class ExtensionFunctionDispatcher
   // TODO(lazyboy): The map entries are cleared upon RenderProcessHost shutown,
   // we should really be clearing it on service worker shutdown.
   WorkerResponseCallbackWrapperMap response_callback_wrappers_for_worker_;
+
+  // The set of ExtensionFunction instances waiting for responses from
+  // the renderer. These are removed once the response is processed.
+  // The lifetimes of the instances are managed by the instances themselves.
+  std::set<ExtensionFunction*> worker_response_targets_;
 };
 
 }  // namespace extensions

extensions/browser/extension_service_worker_message_filter.cc

@@ -40,7 +40,8 @@ void ExtensionServiceWorkerMessageFilter::OverrideThreadForMessage(
       message.type() ==
           ExtensionHostMsg_DidInitializeServiceWorkerContext::ID ||
       message.type() == ExtensionHostMsg_DidStartServiceWorkerContext::ID ||
-      message.type() == ExtensionHostMsg_DidStopServiceWorkerContext::ID) {
+      message.type() == ExtensionHostMsg_DidStopServiceWorkerContext::ID ||
+      message.type() == ExtensionHostMsg_WorkerResponseAck::ID) {
     *thread = content::BrowserThread::UI;
   }
 
@@ -67,6 +68,7 @@ bool ExtensionServiceWorkerMessageFilter::OnMessageReceived(
                         OnDidStartServiceWorkerContext)
     IPC_MESSAGE_HANDLER(ExtensionHostMsg_DidStopServiceWorkerContext,
                         OnDidStopServiceWorkerContext)
+    IPC_MESSAGE_HANDLER(ExtensionHostMsg_WorkerResponseAck, OnResponseWorker)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
@@ -78,6 +80,13 @@ void ExtensionServiceWorkerMessageFilter::OnRequestWorker(
   dispatcher_->Dispatch(params, nullptr, render_process_id_);
 }
 
+void ExtensionServiceWorkerMessageFilter::OnResponseWorker(
+    int request_id,
+    int64_t service_worker_version_id) {
+  dispatcher_->ProcessServiceWorkerResponse(request_id,
+                                            service_worker_version_id);
+}
+
 void ExtensionServiceWorkerMessageFilter::OnIncrementServiceWorkerActivity(
     int64_t service_worker_version_id,
     const std::string& request_uuid) {

extensions/browser/extension_service_worker_message_filter.h

@@ -45,6 +45,7 @@ class ExtensionServiceWorkerMessageFilter
 
   // Message handlers.
   void OnRequestWorker(const ExtensionHostMsg_Request_Params& params);
+  void OnResponseWorker(int request_id, int64_t service_worker_version_id);
   void OnIncrementServiceWorkerActivity(int64_t service_worker_version_id,
                                         const std::string& request_uuid);
   void OnDecrementServiceWorkerActivity(int64_t service_worker_version_id,

extensions/common/extension_messages.h

@@ -1088,6 +1088,12 @@ IPC_MESSAGE_CONTROL5(ExtensionHostMsg_DidStopServiceWorkerContext,
                      int64_t /* service_worker_version_id */,
                      int /* worker_thread_id */)
 
+// Optional Ack message sent to the browser to notify that the response to a
+// function has been processed.
+IPC_MESSAGE_CONTROL2(ExtensionHostMsg_WorkerResponseAck,
+                     int /* request_id */,
+                     int64_t /* service_worker_version_id */)
+
 IPC_STRUCT_BEGIN(ExtensionMsg_AccessibilityEventBundleParams)
   // ID of the accessibility tree that this event applies to.
   IPC_STRUCT_MEMBER(ui::AXTreeID, tree_id)