ServiceWorker: Pass WebEmbeddedWorkerStartData as a function argument

This is a cleanup CL, and doesn't change existing behavior.

Before this CL, WebEmbeddedWorkerStartData was kept as a member of
WebEmbeddedWorkerImpl, and passed when a new service worker thread starts.
After this CL, the data is directly passed through WebEmbeddedWorkerImpl as a
function argument.

Keeping the data as a member was needed when asynchronous script fetch was done
on the initiator thread. Now, script fetch is done on the worker thread, and
operations on the initiator thread are synchronous. It's no longer necessary to
keep it as a member.

In addition, this CL...

- passes the data as std::unique_ptr not const-reference because it's modified
  during being passed through WebEmbeddedWorkerImpl.
- also makes |devtools_worker_token_| and |wait_for_debuffer_mode_| non-member.


Bug: n/a
Change-Id: I30b606c5543c4a8776561a10da9972bb29eff83a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1813798Reviewed-by: Kenichi Ishibashi <bashi@chromium.org>
Commit-Queue: Hiroki Nakagawa <nhiroki@chromium.org>
Cr-Commit-Position: refs/heads/master@{#699569}

content/renderer/service_worker/embedded_worker_instance_client_impl.cc

@@ -64,7 +64,8 @@ void EmbeddedWorkerInstanceClientImpl::StartWorker(
                "EmbeddedWorkerInstanceClientImpl::StartWorker");
   auto start_timing = blink::mojom::EmbeddedWorkerStartTiming::New();
   start_timing->start_worker_received_time = base::TimeTicks::Now();
-  auto start_data = BuildStartData(*params);
+  std::unique_ptr<blink::WebEmbeddedWorkerStartData> start_data =
+      BuildStartData(*params);
 
   DCHECK(!params->provider_info->cache_storage ||
          base::FeatureList::IsEnabled(
@@ -119,7 +120,7 @@ void EmbeddedWorkerInstanceClientImpl::StartWorker(
       service_worker_context_client_.get(), cache_storage.PassPipe(),
       interface_provider.PassHandle(), browser_interface_broker.PassPipe());
   service_worker_context_client_->StartWorkerContextOnInitiatorThread(
-      std::move(worker), start_data,
+      std::move(worker), std::move(start_data),
       std::move(installed_scripts_manager_params),
       params->content_settings_proxy.PassHandle());
 }
@@ -165,25 +166,24 @@ void EmbeddedWorkerInstanceClientImpl::OnError() {
   delete this;
 }
 
-blink::WebEmbeddedWorkerStartData
+std::unique_ptr<blink::WebEmbeddedWorkerStartData>
 EmbeddedWorkerInstanceClientImpl::BuildStartData(
     const blink::mojom::EmbeddedWorkerStartParams& params) {
   DCHECK(initiator_thread_task_runner_->BelongsToCurrentThread());
-  blink::WebEmbeddedWorkerStartData start_data;
-  start_data.script_url = params.script_url;
-  start_data.user_agent = blink::WebString::FromUTF8(params.user_agent);
-  start_data.script_type = params.script_type;
-  start_data.wait_for_debugger_mode =
+  auto start_data = std::make_unique<blink::WebEmbeddedWorkerStartData>();
+  start_data->script_url = params.script_url;
+  start_data->user_agent = blink::WebString::FromUTF8(params.user_agent);
+  start_data->script_type = params.script_type;
+  start_data->wait_for_debugger_mode =
       params.wait_for_debugger
           ? blink::WebEmbeddedWorkerStartData::kWaitForDebugger
           : blink::WebEmbeddedWorkerStartData::kDontWaitForDebugger;
-  start_data.devtools_worker_token = params.devtools_worker_token;
-  start_data.v8_cache_options =
+  start_data->devtools_worker_token = params.devtools_worker_token;
+  start_data->v8_cache_options =
       static_cast<blink::WebSettings::V8CacheOptions>(params.v8_cache_options);
-  start_data.privacy_preferences = blink::PrivacyPreferences(
+  start_data->privacy_preferences = blink::PrivacyPreferences(
       params.renderer_preferences->enable_do_not_track,
       params.renderer_preferences->enable_referrers);
-
   return start_data;
 }
 

content/renderer/service_worker/embedded_worker_instance_client_impl.h

@@ -81,7 +81,7 @@ class CONTENT_EXPORT EmbeddedWorkerInstanceClientImpl
   // Handler of connection error bound to |receiver_|.
   void OnError();
 
-  blink::WebEmbeddedWorkerStartData BuildStartData(
+  std::unique_ptr<blink::WebEmbeddedWorkerStartData> BuildStartData(
       const blink::mojom::EmbeddedWorkerStartParams& params);
 
   mojo::Receiver<blink::mojom::EmbeddedWorkerInstanceClient> receiver_;

content/renderer/service_worker/service_worker_context_client.cc

@@ -171,14 +171,14 @@ ServiceWorkerContextClient::~ServiceWorkerContextClient() {
 
 void ServiceWorkerContextClient::StartWorkerContextOnInitiatorThread(
     std::unique_ptr<blink::WebEmbeddedWorker> worker,
-    const blink::WebEmbeddedWorkerStartData& start_data,
+    std::unique_ptr<blink::WebEmbeddedWorkerStartData> start_data,
     std::unique_ptr<blink::WebServiceWorkerInstalledScriptsManagerParams>
         installed_scripts_manager_params,
     mojo::ScopedMessagePipeHandle content_settings_handle) {
   DCHECK(initiator_thread_task_runner_->RunsTasksInCurrentSequence());
   worker_ = std::move(worker);
   worker_->StartWorkerContext(
-      start_data, std::move(installed_scripts_manager_params),
+      std::move(start_data), std::move(installed_scripts_manager_params),
       std::move(content_settings_handle), initiator_thread_task_runner_);
 }
 

content/renderer/service_worker/service_worker_context_client.h

@@ -115,7 +115,7 @@ class CONTENT_EXPORT ServiceWorkerContextClient
   // Called on the initiator thread.
   void StartWorkerContextOnInitiatorThread(
       std::unique_ptr<blink::WebEmbeddedWorker> worker,
-      const blink::WebEmbeddedWorkerStartData& start_data,
+      std::unique_ptr<blink::WebEmbeddedWorkerStartData> start_data,
       std::unique_ptr<blink::WebServiceWorkerInstalledScriptsManagerParams>,
       mojo::ScopedMessagePipeHandle content_settings_handle);
   // Called on the initiator thread.

third_party/blink/public/web/web_embedded_worker.h

@@ -72,7 +72,7 @@ class BLINK_EXPORT WebEmbeddedWorker {
 
   // Starts and terminates WorkerThread and WorkerGlobalScope.
   virtual void StartWorkerContext(
-      const WebEmbeddedWorkerStartData&,
+      std::unique_ptr<WebEmbeddedWorkerStartData>,
       std::unique_ptr<WebServiceWorkerInstalledScriptsManagerParams>,
       mojo::ScopedMessagePipeHandle content_settings_handle,
       scoped_refptr<base::SingleThreadTaskRunner>

third_party/blink/renderer/modules/exported/web_embedded_worker_impl.cc

@@ -106,13 +106,12 @@ WebEmbeddedWorkerImpl::~WebEmbeddedWorkerImpl() {
 }
 
 void WebEmbeddedWorkerImpl::StartWorkerContext(
-    const WebEmbeddedWorkerStartData& data,
+    std::unique_ptr<WebEmbeddedWorkerStartData> worker_start_data,
     std::unique_ptr<WebServiceWorkerInstalledScriptsManagerParams>
         installed_scripts_manager_params,
     mojo::ScopedMessagePipeHandle content_settings_handle,
     scoped_refptr<base::SingleThreadTaskRunner> initiator_thread_task_runner) {
   DCHECK(!asked_to_terminate_);
-  worker_start_data_ = data;
 
   std::unique_ptr<ServiceWorkerInstalledScriptsManager>
       installed_scripts_manager;
@@ -144,17 +143,15 @@ void WebEmbeddedWorkerImpl::StartWorkerContext(
   // we should fix, but we're taking this shortcut for the prototype.
   //
   // https://crbug.com/590714
-  KURL script_url = worker_start_data_.script_url;
-  worker_start_data_.address_space = network::mojom::IPAddressSpace::kPublic;
+  KURL script_url = worker_start_data->script_url;
+  worker_start_data->address_space = network::mojom::IPAddressSpace::kPublic;
   if (network_utils::IsReservedIPAddress(script_url.Host()))
-    worker_start_data_.address_space = network::mojom::IPAddressSpace::kPrivate;
+    worker_start_data->address_space = network::mojom::IPAddressSpace::kPrivate;
   if (SecurityOrigin::Create(script_url)->IsLocalhost())
-    worker_start_data_.address_space = network::mojom::IPAddressSpace::kLocal;
+    worker_start_data->address_space = network::mojom::IPAddressSpace::kLocal;
 
-  devtools_worker_token_ = data.devtools_worker_token;
-  wait_for_debugger_mode_ = worker_start_data_.wait_for_debugger_mode;
   StartWorkerThread(
-      std::move(installed_scripts_manager),
+      std::move(worker_start_data), std::move(installed_scripts_manager),
       std::make_unique<ServiceWorkerContentSettingsProxy>(
           // Chrome doesn't use interface versioning.
           // TODO(falken): Is that comment about versioning correct?
@@ -178,6 +175,7 @@ void WebEmbeddedWorkerImpl::ResumeAfterDownload() {
 }
 
 void WebEmbeddedWorkerImpl::StartWorkerThread(
+    std::unique_ptr<WebEmbeddedWorkerStartData> worker_start_data,
     std::unique_ptr<ServiceWorkerInstalledScriptsManager>
         installed_scripts_manager,
     std::unique_ptr<ServiceWorkerContentSettingsProxy> content_settings_proxy,
@@ -191,7 +189,7 @@ void WebEmbeddedWorkerImpl::StartWorkerThread(
   // appropriate Document. See comment in CreateFetchClientSettingsObject() for
   // details.
   scoped_refptr<const SecurityOrigin> starter_origin =
-      SecurityOrigin::Create(worker_start_data_.script_url);
+      SecurityOrigin::Create(worker_start_data->script_url);
   // This roughly equals to shadow document's IsSecureContext() as a shadow
   // document have a frame with no parent.
   // See also Document::InitSecureContextState().
@@ -223,22 +221,22 @@ void WebEmbeddedWorkerImpl::StartWorkerThread(
 
   bool is_script_installed =
       installed_scripts_manager && installed_scripts_manager->IsScriptInstalled(
-                                       worker_start_data_.script_url);
+                                       worker_start_data->script_url);
 
   // We don't have to set ContentSecurityPolicy and ReferrerPolicy. They're
   // served by the worker script loader or the installed scripts manager on the
   // worker thread.
   global_scope_creation_params = std::make_unique<GlobalScopeCreationParams>(
-      worker_start_data_.script_url, worker_start_data_.script_type,
+      worker_start_data->script_url, worker_start_data->script_type,
       OffMainThreadWorkerScriptFetchOption::kEnabled, global_scope_name,
-      worker_start_data_.user_agent, std::move(web_worker_fetch_context),
+      worker_start_data->user_agent, std::move(web_worker_fetch_context),
       Vector<CSPHeaderAndType>(), network::mojom::ReferrerPolicy::kDefault,
       starter_origin.get(), starter_secure_context, starter_https_state,
       nullptr /* worker_clients */, std::move(content_settings_proxy),
       base::nullopt /* response_address_space */,
-      nullptr /* OriginTrialTokens */, devtools_worker_token_,
+      nullptr /* OriginTrialTokens */, worker_start_data->devtools_worker_token,
       std::move(worker_settings),
-      static_cast<V8CacheOptions>(worker_start_data_.v8_cache_options),
+      static_cast<V8CacheOptions>(worker_start_data->v8_cache_options),
       nullptr /* worklet_module_respones_map */,
       std::move(interface_provider_info_), std::move(browser_interface_broker_),
       BeginFrameProviderParams(), nullptr /* parent_feature_policy */,
@@ -255,9 +253,11 @@ void WebEmbeddedWorkerImpl::StartWorkerThread(
       initiator_thread_task_runner);
 
   auto devtools_params = std::make_unique<WorkerDevToolsParams>();
-  devtools_params->devtools_worker_token = devtools_worker_token_;
+  devtools_params->devtools_worker_token =
+      worker_start_data->devtools_worker_token;
   devtools_params->wait_for_debugger =
-      wait_for_debugger_mode_ == WebEmbeddedWorkerStartData::kWaitForDebugger;
+      worker_start_data->wait_for_debugger_mode ==
+      WebEmbeddedWorkerStartData::kWaitForDebugger;
   mojo::PendingRemote<mojom::blink::DevToolsAgent> devtools_agent_remote;
   devtools_params->agent_receiver =
       devtools_agent_remote.InitWithNewPipeAndPassReceiver();
@@ -272,36 +272,38 @@ void WebEmbeddedWorkerImpl::StartWorkerThread(
   // If this is an installed service worker, the installed script will be read
   // from the service worker script storage on the worker thread.
   if (is_script_installed) {
-    switch (worker_start_data_.script_type) {
+    switch (worker_start_data->script_type) {
       case mojom::ScriptType::kClassic:
         worker_thread_->RunInstalledClassicScript(
-            worker_start_data_.script_url, v8_inspector::V8StackTraceId());
+            worker_start_data->script_url, v8_inspector::V8StackTraceId());
         break;
       case mojom::ScriptType::kModule:
         worker_thread_->RunInstalledModuleScript(
-            worker_start_data_.script_url,
-            CreateFetchClientSettingsObjectData(starter_origin.get(),
-                                                starter_https_state),
+            worker_start_data->script_url,
+            CreateFetchClientSettingsObjectData(
+                worker_start_data->script_url, starter_origin.get(),
+                starter_https_state, worker_start_data->address_space),
             network::mojom::CredentialsMode::kOmit);
         break;
     }
   } else {
     std::unique_ptr<CrossThreadFetchClientSettingsObjectData>
         fetch_client_setting_object_data = CreateFetchClientSettingsObjectData(
-            starter_origin.get(), starter_https_state);
+            worker_start_data->script_url, starter_origin.get(),
+            starter_https_state, worker_start_data->address_space);
 
     // If this is a new (not installed) service worker, we are in the Update
     // algorithm here:
     // > Switching on job's worker type, run these substeps with the following
     // > options:
     // https://w3c.github.io/ServiceWorker/#update-algorithm
-    switch (worker_start_data_.script_type) {
+    switch (worker_start_data->script_type) {
       // > "classic": Fetch a classic worker script given job's serialized
       // > script url, job's client, "serviceworker", and the to-be-created
       // > environment settings object for this service worker.
       case mojom::ScriptType::kClassic:
         worker_thread_->FetchAndRunClassicScript(
-            worker_start_data_.script_url,
+            worker_start_data->script_url,
             std::move(fetch_client_setting_object_data),
             nullptr /* outside_resource_timing_notifier */,
             v8_inspector::V8StackTraceId());
@@ -312,7 +314,7 @@ void WebEmbeddedWorkerImpl::StartWorkerThread(
       // > to-be-created environment settings object for this service worker.
       case mojom::ScriptType::kModule:
         worker_thread_->FetchAndRunModuleScript(
-            worker_start_data_.script_url,
+            worker_start_data->script_url,
             std::move(fetch_client_setting_object_data),
             nullptr /* outside_resource_timing_notifier */,
             network::mojom::CredentialsMode::kOmit);
@@ -327,10 +329,12 @@ void WebEmbeddedWorkerImpl::StartWorkerThread(
 
 std::unique_ptr<CrossThreadFetchClientSettingsObjectData>
 WebEmbeddedWorkerImpl::CreateFetchClientSettingsObjectData(
+    const KURL& script_url,
     const SecurityOrigin* security_origin,
-    const HttpsState& https_state) {
+    const HttpsState& https_state,
+    network::mojom::IPAddressSpace address_space) {
   // TODO(crbug.com/967265): Currently we create an incomplete outside settings
-  // object from |worker_start_data_| but we should create a proper outside
+  // object from |worker_start_data| but we should create a proper outside
   // settings objects depending on the situation. For new worker case, this
   // should be the Document that called navigator.serviceWorker.register(). For
   // ServiceWorkerRegistration#update() case, it should be the Document that
@@ -339,14 +343,12 @@ WebEmbeddedWorkerImpl::CreateFetchClientSettingsObjectData(
   // To get a correct settings, we need to make a way to pass the settings
   // object over mojo IPCs.
 
-  const KURL& script_url = worker_start_data_.script_url;
   return std::make_unique<CrossThreadFetchClientSettingsObjectData>(
       script_url.Copy() /* global_object_url */,
       script_url.Copy() /* base_url */, security_origin->IsolatedCopy(),
       network::mojom::ReferrerPolicy::kDefault,
       script_url.GetString().IsolatedCopy() /* outgoing_referrer */,
-      https_state, AllowedByNosniff::MimeTypeCheck::kLax,
-      worker_start_data_.address_space,
+      https_state, AllowedByNosniff::MimeTypeCheck::kLax, address_space,
       kBlockAllMixedContent /* insecure_requests_policy */,
       FetchClientSettingsObject::InsecureNavigationsSet(),
       false /* mixed_autoupgrade_opt_out */);

third_party/blink/renderer/modules/exported/web_embedded_worker_impl.h

@@ -70,7 +70,7 @@ class MODULES_EXPORT WebEmbeddedWorkerImpl final : public WebEmbeddedWorker {
 
   // WebEmbeddedWorker overrides.
   void StartWorkerContext(
-      const WebEmbeddedWorkerStartData&,
+      std::unique_ptr<WebEmbeddedWorkerStartData>,
       std::unique_ptr<WebServiceWorkerInstalledScriptsManagerParams>,
       mojo::ScopedMessagePipeHandle content_settings_handle,
       scoped_refptr<base::SingleThreadTaskRunner> initiator_thread_task_runner)
@@ -82,6 +82,7 @@ class MODULES_EXPORT WebEmbeddedWorkerImpl final : public WebEmbeddedWorker {
 
  private:
   void StartWorkerThread(
+      std::unique_ptr<WebEmbeddedWorkerStartData> worker_start_data,
       std::unique_ptr<ServiceWorkerInstalledScriptsManager>,
       std::unique_ptr<ServiceWorkerContentSettingsProxy>,
       scoped_refptr<base::SingleThreadTaskRunner> initiator_thread_task_runner);
@@ -89,9 +90,10 @@ class MODULES_EXPORT WebEmbeddedWorkerImpl final : public WebEmbeddedWorker {
   // Creates a cross-thread copyable outside settings object for top-level
   // worker script fetch.
   std::unique_ptr<CrossThreadFetchClientSettingsObjectData>
-  CreateFetchClientSettingsObjectData(const SecurityOrigin*, const HttpsState&);
-
-  WebEmbeddedWorkerStartData worker_start_data_;
+  CreateFetchClientSettingsObjectData(const KURL& script_url,
+                                      const SecurityOrigin*,
+                                      const HttpsState&,
+                                      network::mojom::IPAddressSpace);
 
   // Client must remain valid through the entire life time of the worker.
   WebServiceWorkerContextClient* const worker_context_client_;
@@ -100,12 +102,6 @@ class MODULES_EXPORT WebEmbeddedWorkerImpl final : public WebEmbeddedWorker {
 
   bool asked_to_terminate_ = false;
 
-  // Unique worker token used by DevTools to attribute different instrumentation
-  // to the same worker.
-  base::UnguessableToken devtools_worker_token_;
-  WebEmbeddedWorkerStartData::WaitForDebuggerMode wait_for_debugger_mode_ =
-      WebEmbeddedWorkerStartData::kDontWaitForDebugger;
-
   mojo::PendingRemote<mojom::blink::CacheStorage> cache_storage_remote_;
 
   service_manager::mojom::blink::InterfaceProviderPtrInfo

third_party/blink/renderer/modules/service_worker/web_embedded_worker_impl_test.cc

@@ -200,20 +200,23 @@ class WebEmbeddedWorkerImplTest : public testing::Test {
         /*interface_provider_info=*/nullptr,
         /*browser_interface_broker=*/mojo::NullRemote());
 
-    WebURL script_url =
-        url_test_helpers::ToKURL("https://www.example.com/sw.js");
-    WebURLResponse response(script_url);
+    script_url_ = url_test_helpers::ToKURL("https://www.example.com/sw.js");
+    WebURLResponse response(script_url_);
     response.SetMimeType("text/javascript");
     response.SetHttpStatusCode(200);
-    Platform::Current()->GetURLLoaderMockFactory()->RegisterURL(script_url,
+    Platform::Current()->GetURLLoaderMockFactory()->RegisterURL(script_url_,
                                                                 response, "");
+  }
 
-    start_data_.script_url = script_url;
-    start_data_.user_agent = WebString("dummy user agent");
-    start_data_.script_type = mojom::ScriptType::kClassic;
-    start_data_.wait_for_debugger_mode =
+  std::unique_ptr<WebEmbeddedWorkerStartData> CreateStartData() {
+    auto start_data = std::make_unique<WebEmbeddedWorkerStartData>();
+    start_data->script_url = script_url_;
+    start_data->user_agent = WebString("dummy user agent");
+    start_data->script_type = mojom::ScriptType::kClassic;
+    start_data->wait_for_debugger_mode =
         WebEmbeddedWorkerStartData::kDontWaitForDebugger;
-    start_data_.v8_cache_options = WebSettings::V8CacheOptions::kDefault;
+    start_data->v8_cache_options = WebSettings::V8CacheOptions::kDefault;
+    return start_data;
   }
 
   void TearDown() override {
@@ -222,7 +225,7 @@ class WebEmbeddedWorkerImplTest : public testing::Test {
         ->UnregisterAllURLsAndClearMemoryCache();
   }
 
-  WebEmbeddedWorkerStartData start_data_;
+  WebURL script_url_;
   std::unique_ptr<MockServiceWorkerContextClient> mock_client_;
   std::unique_ptr<WebEmbeddedWorkerImpl> worker_;
 };
@@ -231,7 +234,7 @@ class WebEmbeddedWorkerImplTest : public testing::Test {
 
 TEST_F(WebEmbeddedWorkerImplTest, TerminateSoonAfterStart) {
   worker_->StartWorkerContext(
-      start_data_,
+      CreateStartData(),
       /*installed_scripts_manager_params=*/nullptr,
       /*content_settings_proxy=*/mojo::ScopedMessagePipeHandle(),
       Thread::Current()->GetTaskRunner());
@@ -243,10 +246,11 @@ TEST_F(WebEmbeddedWorkerImplTest, TerminateSoonAfterStart) {
 }
 
 TEST_F(WebEmbeddedWorkerImplTest, TerminateWhileWaitingForDebugger) {
-  start_data_.wait_for_debugger_mode =
+  std::unique_ptr<WebEmbeddedWorkerStartData> start_data = CreateStartData();
+  start_data->wait_for_debugger_mode =
       WebEmbeddedWorkerStartData::kWaitForDebugger;
   worker_->StartWorkerContext(
-      start_data_,
+      std::move(start_data),
       /*installed_scripts_manager_params=*/nullptr,
       /*content_settings_proxy=*/mojo::ScopedMessagePipeHandle(),
       Thread::Current()->GetTaskRunner());
@@ -259,7 +263,7 @@ TEST_F(WebEmbeddedWorkerImplTest, TerminateWhileWaitingForDebugger) {
 TEST_F(WebEmbeddedWorkerImplTest, TerminateWhileLoadingScript) {
   // Load the shadow page.
   worker_->StartWorkerContext(
-      start_data_,
+      CreateStartData(),
       /*installed_scripts_manager_params=*/nullptr,
       /*content_settings_proxy=*/mojo::ScopedMessagePipeHandle(),
       Thread::Current()->GetTaskRunner());
@@ -280,11 +284,12 @@ TEST_F(WebEmbeddedWorkerImplTest, ScriptNotFound) {
   ResourceError error = ResourceError::Failure(script_url);
   Platform::Current()->GetURLLoaderMockFactory()->RegisterErrorURL(
       script_url, response, error);
-  start_data_.script_url = script_url;
+  std::unique_ptr<WebEmbeddedWorkerStartData> start_data = CreateStartData();
+  start_data->script_url = script_url;
 
   // Start worker and load the script.
   worker_->StartWorkerContext(
-      start_data_,
+      std::move(start_data),
       /*installed_scripts_manager_params=*/nullptr,
       /*content_settings_proxy=*/mojo::ScopedMessagePipeHandle(),
       Thread::Current()->GetTaskRunner());