Browse Source

fix: loading dedicated/shared worker scripts over custom protocol (#24749)

* fix: add patch to avoid crash in worker with nodeintegration enabled

[worker_global_scope.cc(111)] Check failed: url_.IsValid().

* fix: loading dedicated/shared worker over custom protocols

Backports https://chromium-review.googlesource.com/c/chromium/src/+/1798250
that distinguishes loading the main script resource of dedicated/shared
worker, this allows us to register a custom URLLoaderFactory.

* spec: add crash test for worker with nodeIntegrationInWorker

* update patches

* Remove extra patchlist patches

* Fixup patch

* update patches

Co-authored-by: deepak1556 <[email protected]>
Co-authored-by: Shelley Vohr <[email protected]>
Co-authored-by: Electron Bot <[email protected]>
trop[bot] 4 years ago
parent
commit
04e3a95b85

+ 1 - 0
patches/chromium/.patches

@@ -97,3 +97,4 @@ disable_unnecessary_ischromefirstrun_check.patch
 use_electron_resources_in_icon_reader_service.patch
 fix_include_missing_header_file.patch
 cherrypick_future_chrome_commit_to_remove_superfluous_dcheck.patch
+worker_feat_add_hook_to_notify_script_ready.patch

+ 92 - 0
patches/chromium/worker_feat_add_hook_to_notify_script_ready.patch

@@ -0,0 +1,92 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: deepak1556 <[email protected]>
+Date: Thu, 17 Oct 2019 18:00:32 -0700
+Subject: feat: add hook to notify script ready from WorkerScriptController
+
+In Off-the-main-thread fetch, the WorkerGlobalScope will be in a half
+initialized state until the script is finished downloading.
+
+Doc: https://docs.google.com/document/d/1JCv8TD2nPLNC2iRCp_D1OM4I3uTS0HoEobuTymaMqgw/edit
+
+During this stage if the global object is transformed for ex: copying properties
+in DidInitializeWorkerContextOnWorkerThread hook then an access to property like
+location will result in a crash WorkerGlobalScope::Url() because the script has
+not been set with response URL yet.
+
+This issue cannot happen in chromium with existing usage, but can surface when an
+embedder tries to integrate Node.js in the worker. Hence, this new hook is proposed
+that clearly establishes the worker script is ready for evaluation with the scope
+initialized.
+
+diff --git a/content/public/renderer/content_renderer_client.h b/content/public/renderer/content_renderer_client.h
+index e56aae188d7d8f46a4438b8e99f7e9b714c5875a..56b1155e36ed29a1a406e3824b6903346ce40793 100644
+--- a/content/public/renderer/content_renderer_client.h
++++ b/content/public/renderer/content_renderer_client.h
+@@ -404,6 +404,11 @@ class CONTENT_EXPORT ContentRendererClient {
+   virtual void DidInitializeWorkerContextOnWorkerThread(
+       v8::Local<v8::Context> context) {}
+ 
++  // Notifies that a worker script has been downloaded, scope initialized and
++  // ready for evaluation. This function is called from the worker thread.
++  virtual void WorkerScriptReadyForEvaluationOnWorkerThread(
++      v8::Local<v8::Context> context) {}
++
+   // Notifies that a worker context will be destroyed. This function is called
+   // from the worker thread.
+   virtual void WillDestroyWorkerContextOnWorkerThread(
+diff --git a/content/renderer/renderer_blink_platform_impl.cc b/content/renderer/renderer_blink_platform_impl.cc
+index d5a84147b0479e18e53fc94b28c41393803c8af3..c140cc17d12b91bd8e2026751cc183a2880e79b4 100644
+--- a/content/renderer/renderer_blink_platform_impl.cc
++++ b/content/renderer/renderer_blink_platform_impl.cc
+@@ -872,6 +872,12 @@ void RendererBlinkPlatformImpl::WorkerContextCreated(
+       worker);
+ }
+ 
++void RendererBlinkPlatformImpl::WorkerScriptReadyForEvaluation(
++    const v8::Local<v8::Context>& worker) {
++  GetContentClient()->renderer()->WorkerScriptReadyForEvaluationOnWorkerThread(
++      worker);
++}
++
+ bool RendererBlinkPlatformImpl::IsExcludedHeaderForServiceWorkerFetchEvent(
+     const blink::WebString& header_name) {
+   return GetContentClient()
+diff --git a/content/renderer/renderer_blink_platform_impl.h b/content/renderer/renderer_blink_platform_impl.h
+index 6c8ef4c7c37e6b951f228500b178a53d91ebdfb9..4165e0cf3d3c5b0c289f2a5d83d72d698b71e5a3 100644
+--- a/content/renderer/renderer_blink_platform_impl.h
++++ b/content/renderer/renderer_blink_platform_impl.h
+@@ -181,6 +181,8 @@ class CONTENT_EXPORT RendererBlinkPlatformImpl : public BlinkPlatformImpl {
+   void DidStartWorkerThread() override;
+   void WillStopWorkerThread() override;
+   void WorkerContextCreated(const v8::Local<v8::Context>& worker) override;
++  void WorkerScriptReadyForEvaluation(
++      const v8::Local<v8::Context>& worker) override;
+   void WorkerContextWillDestroy(const v8::Local<v8::Context>& worker) override;
+   bool IsExcludedHeaderForServiceWorkerFetchEvent(
+       const blink::WebString& header_name) override;
+diff --git a/third_party/blink/public/platform/platform.h b/third_party/blink/public/platform/platform.h
+index 01930661416c1987790bb9b3fdd88a04e3662c7a..10d5a00c69f939b5871dea5ea9d6f059066c458d 100644
+--- a/third_party/blink/public/platform/platform.h
++++ b/third_party/blink/public/platform/platform.h
+@@ -619,6 +619,8 @@ class BLINK_PLATFORM_EXPORT Platform {
+   virtual void DidStartWorkerThread() {}
+   virtual void WillStopWorkerThread() {}
+   virtual void WorkerContextCreated(const v8::Local<v8::Context>& worker) {}
++  virtual void WorkerScriptReadyForEvaluation(
++      const v8::Local<v8::Context>& worker) {}
+   virtual void WorkerContextWillDestroy(const v8::Local<v8::Context>& worker) {}
+   virtual bool AllowScriptExtensionForServiceWorker(
+       const WebSecurityOrigin& script_origin) {
+diff --git a/third_party/blink/renderer/bindings/core/v8/worker_or_worklet_script_controller.cc b/third_party/blink/renderer/bindings/core/v8/worker_or_worklet_script_controller.cc
+index d5e60500c217a71762030688ea13c93c3a6e717c..8be5fc85456be85acbca78542106bdf6a98965a7 100644
+--- a/third_party/blink/renderer/bindings/core/v8/worker_or_worklet_script_controller.cc
++++ b/third_party/blink/renderer/bindings/core/v8/worker_or_worklet_script_controller.cc
+@@ -323,6 +323,8 @@ void WorkerOrWorkletScriptController::PrepareForEvaluation() {
+   wrapper_type_info->InstallConditionalFeatures(
+       context, *world_, global_object, v8::Local<v8::Object>(),
+       v8::Local<v8::Function>(), global_interface_template);
++
++  Platform::Current()->WorkerScriptReadyForEvaluation(context);
+ }
+ 
+ void WorkerOrWorkletScriptController::DisableEvalInternal(

+ 10 - 0
shell/browser/electron_browser_client.cc

@@ -1279,6 +1279,16 @@ void ElectronBrowserClient::RegisterNonNetworkNavigationURLLoaderFactories(
       URLLoaderFactoryType::kNavigation, factories);
 }
 
+void ElectronBrowserClient::
+    RegisterNonNetworkWorkerMainResourceURLLoaderFactories(
+        content::BrowserContext* browser_context,
+        NonNetworkURLLoaderFactoryMap* factories) {
+  auto* protocol_registry =
+      ProtocolRegistry::FromBrowserContext(browser_context);
+  protocol_registry->RegisterURLLoaderFactories(
+      URLLoaderFactoryType::kWorkerMainResource, factories);
+}
+
 #if BUILDFLAG(ENABLE_ELECTRON_EXTENSIONS)
 namespace {
 

+ 3 - 0
shell/browser/electron_browser_client.h

@@ -178,6 +178,9 @@ class ElectronBrowserClient : public content::ContentBrowserClient,
   void RegisterNonNetworkNavigationURLLoaderFactories(
       int frame_tree_node_id,
       NonNetworkURLLoaderFactoryMap* factories) override;
+  void RegisterNonNetworkWorkerMainResourceURLLoaderFactories(
+      content::BrowserContext* browser_context,
+      NonNetworkURLLoaderFactoryMap* factories) override;
   void RegisterNonNetworkSubresourceURLLoaderFactories(
       int render_process_id,
       int render_frame_id,

+ 2 - 2
shell/renderer/electron_renderer_client.cc

@@ -200,11 +200,11 @@ bool ElectronRendererClient::ShouldFork(blink::WebLocalFrame* frame,
   return http_method == "GET";
 }
 
-void ElectronRendererClient::DidInitializeWorkerContextOnWorkerThread(
+void ElectronRendererClient::WorkerScriptReadyForEvaluationOnWorkerThread(
     v8::Local<v8::Context> context) {
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kNodeIntegrationInWorker)) {
-    WebWorkerObserver::GetCurrent()->ContextCreated(context);
+    WebWorkerObserver::GetCurrent()->WorkerScriptReadyForEvaluation(context);
   }
 }
 

+ 1 - 1
shell/renderer/electron_renderer_client.h

@@ -47,7 +47,7 @@ class ElectronRendererClient : public RendererClientBase {
                   const std::string& http_method,
                   bool is_initial_navigation,
                   bool is_server_redirect) override;
-  void DidInitializeWorkerContextOnWorkerThread(
+  void WorkerScriptReadyForEvaluationOnWorkerThread(
       v8::Local<v8::Context> context) override;
   void WillDestroyWorkerContextOnWorkerThread(
       v8::Local<v8::Context> context) override;

+ 2 - 1
shell/renderer/web_worker_observer.cc

@@ -42,7 +42,8 @@ WebWorkerObserver::~WebWorkerObserver() {
   asar::ClearArchives();
 }
 
-void WebWorkerObserver::ContextCreated(v8::Local<v8::Context> worker_context) {
+void WebWorkerObserver::WorkerScriptReadyForEvaluation(
+    v8::Local<v8::Context> worker_context) {
   v8::Context::Scope context_scope(worker_context);
 
   // Start the embed thread.

+ 1 - 1
shell/renderer/web_worker_observer.h

@@ -21,7 +21,7 @@ class WebWorkerObserver {
   // Returns the WebWorkerObserver for current worker thread.
   static WebWorkerObserver* GetCurrent();
 
-  void ContextCreated(v8::Local<v8::Context> context);
+  void WorkerScriptReadyForEvaluation(v8::Local<v8::Context> context);
   void ContextWillDestroy(v8::Local<v8::Context> context);
 
  private:

+ 24 - 0
spec-main/chromium-spec.ts

@@ -441,6 +441,30 @@ describe('chromium features', () => {
       w.webContents.on('crashed', () => done(new Error('WebContents crashed.')));
       w.loadFile(path.join(fixturesPath, 'pages', 'service-worker', 'index.html'));
     });
+
+    it('should not crash when nodeIntegration is enabled', (done) => {
+      const w = new BrowserWindow({
+        show: false,
+        webPreferences: {
+          nodeIntegration: true,
+          nodeIntegrationInWorker: true
+        }
+      });
+
+      w.webContents.on('ipc-message', (event, channel, message) => {
+        if (channel === 'reload') {
+          w.webContents.reload();
+        } else if (channel === 'error') {
+          done(`unexpected error : ${message}`);
+        } else if (channel === 'response') {
+          expect(message).to.equal('Hello from serviceWorker!');
+          done();
+        }
+      });
+
+      w.webContents.on('crashed', () => done(new Error('WebContents crashed.')));
+      w.loadFile(path.join(fixturesPath, 'pages', 'service-worker', 'index.html'));
+    });
   });
 
   describe('navigator.geolocation', () => {