worker: fix Web Worker spec deviations

lib/internal/webidl.js

@@ -815,16 +815,19 @@ function createDictionaryConverter(
  * @returns {Converter}
  */
 function createSequenceConverter(converter) {
-  return function(V, options = kEmptyObject) {
+  return function(V, options = kEmptyObject, method = undefined) {
     // Web IDL sequence conversion step 1: require an ECMA-262 Object.
     if (type(V) !== 'Object') {
       throw makeException(
         'cannot be converted to sequence.',
         options);
     }
 
-    // Step 2: GetMethod(V, %Symbol.iterator%).
-    const method = V[SymbolIterator];
+    // Step 2: GetMethod(V, %Symbol.iterator%). Overload resolution can
+    // supply the method it already retrieved, avoiding a second lookup.
+    if (method === undefined) {
+      method = V[SymbolIterator];
+    }
     // Step 3: throw if the iterator method is undefined, null, or not callable.
     if (typeof method !== 'function') {
       throw makeException(

lib/internal/webworker.js

@@ -20,8 +20,6 @@ const {
   SymbolFor,
   SymbolIterator,
   SymbolToStringTag,
-  TypedArrayPrototypeGetLength,
-  Uint8Array,
   globalThis,
 } = primordials;
 
@@ -70,17 +68,14 @@ const {
 } = require('internal/worker');
 
 const {
+  MessageChannel,
   lazyMessageEvent,
 } = require('internal/worker/io');
 
 const {
   vm_dynamic_import_default_internal,
 } = internalBinding('symbols');
 
-const {
-  base64Slice,
-} = internalBinding('buffer');
-
 const {
   hasOpenSSL,
 } = internalBinding('config');
@@ -113,6 +108,8 @@ const kLocation = Symbol('kLocation');
 const kName = Symbol('kName');
 const kNavigator = Symbol('kNavigator');
 const kNavigatorBrand = Symbol('kNavigatorBrand');
+const kOutsidePort = Symbol('kOutsidePort');
+const kRemoveMessageListeners = Symbol('kRemoveMessageListeners');
 const kType = Symbol('kType');
 const kURL = Symbol('kURL');
 const kWorker = Symbol('kWorker');
@@ -137,15 +134,21 @@ function createErrorEvent(init) {
 // "If this throws an exception, catch it, fire an event named messageerror
 // at messageEventTarget, using MessageEvent, and then return."
 function forwardMessageEvents(port, target) {
-  port.on('message', (data) =>
+  const onMessage = (data) =>
     target.dispatchEvent(lazyMessageEvent('message', {
       data,
       ports: port[kCurrentlyReceivingPorts],
-    })));
-  port.on('messageerror', (data) =>
+    }));
+  const onMessageError = (data) =>
     target.dispatchEvent(lazyMessageEvent('messageerror', {
       data,
-    })));
+    }));
+  port.on('message', onMessage);
+  port.on('messageerror', onMessageError);
+  return () => {
+    port.removeListener('message', onMessage);
+    port.removeListener('messageerror', onMessageError);
+  };
 }
 
 // Web IDL [Replaceable]: the setter replaces the accessor with an own
@@ -319,9 +322,12 @@ function fetchClassicScriptSourceSync(url, blob) {
 // sequence<object> overload, everything else is converted as a
 // StructuredSerializeOptions dictionary.
 function normalizeTransfer(transferOrOptions, options) {
-  if (typeof transferOrOptions === 'object' && transferOrOptions !== null &&
-      transferOrOptions[SymbolIterator] !== undefined) {
-    return converters['sequence<object>'](transferOrOptions, options);
+  if ((typeof transferOrOptions === 'object' && transferOrOptions !== null) ||
+      typeof transferOrOptions === 'function') {
+    const method = transferOrOptions[SymbolIterator];
+    if (method != null) {
+      return converters['sequence<object>'](transferOrOptions, options, method);
+    }
   }
   return converters.StructuredSerializeOptions(transferOrOptions, options)
     .transfer;
@@ -504,6 +510,11 @@ class WorkerGlobalScope extends EventTarget {
   importScripts(...urls) {
     validateThisInternalField(this, kLocation, 'WorkerGlobalScope');
     const prefix = "Failed to execute 'importScripts' on 'WorkerGlobalScope'";
+    // Web IDL converts every argument before running the method steps.
+    for (let i = 0; i < urls.length; i++) {
+      urls[i] = converters.USVString(
+        urls[i], { prefix, context: `Argument ${i + 1}` });
+    }
     // "To import scripts into worker global scope, given a
     // WorkerGlobalScope object worker global scope, a list of scalar value
     // strings urls, and an optional perform the fetch hook performFetch:"
@@ -522,8 +533,7 @@ class WorkerGlobalScope extends EventTarget {
     const urlRecords = [];
     // "For each url of urls:"
     for (let i = 0; i < urls.length; i++) {
-      const url = converters.USVString(
-        urls[i], { prefix, context: `Argument ${i + 1}` });
+      const url = urls[i];
       // "Let urlRecord be the result of encoding-parsing a URL given url,
       // relative to settings object."
       const urlRecord = URLParse(url, this[kLocation].href);
@@ -750,10 +760,9 @@ const convertWorkerOptions = createDictionaryConverter(
 
 /**
  * @param {URL} workerURL The parsed URL of the worker script.
- * @param {'classic'|'module'} type The worker's type.
  * @returns {{ value?: URL, source?: string }|null}
  */
-function resolveWorkerEntry(workerURL, type) {
+function resolveWorkerEntry(workerURL) {
   switch (workerURL.protocol) {
     case 'file:':
       return { value: workerURL };
@@ -772,17 +781,6 @@ function resolveWorkerEntry(workerURL, type) {
       if (data === undefined) {
         return null;
       }
-      if (type === 'module') {
-        // The blob's contents are re-wrapped as a data: URL so that the
-        // module loader evaluates them as a module script.
-        // TODO(@avivkeller): What if we update the ESM loader to accept blob:
-        // urls?
-        const bytes = new Uint8Array(data);
-        return {
-          value: new URL('data:text/javascript;base64,' + base64Slice(
-            bytes, 0, TypedArrayPrototypeGetLength(bytes))),
-        };
-      }
       return { source: utf8Decode(data) };
     }
     default:
@@ -817,8 +815,14 @@ class Worker extends EventTarget {
     }
 
     this[kWorker] = null;
-    const entry = resolveWorkerEntry(workerURL, options.type);
+    const entry = resolveWorkerEntry(workerURL);
     if (entry === null) {
+      // Even a failed worker has an outside port. Keep a closed port so
+      // postMessage() still serializes its message and transfers objects.
+      const { port1, port2 } = new MessageChannel();
+      port1.close();
+      port2.close();
+      this[kOutsidePort] = port1;
       // "If the algorithm asynchronously completes with null or with a
       // script whose error to rethrow is non-null, then: Queue a global
       // task on the DOM manipulation task source given worker's relevant
@@ -848,7 +852,8 @@ class Worker extends EventTarget {
       },
     });
 
-    forwardMessageEvents(this[kWorker][kPublicPort], this);
+    this[kOutsidePort] = this[kWorker][kPublicPort];
+    this[kRemoveMessageListeners] = forwardMessageEvents(this[kOutsidePort], this);
 
     // "Set notHandled to the result of firing an event named error at
     // workerObject, using ErrorEvent, with the cancelable attribute
@@ -867,6 +872,10 @@ class Worker extends EventTarget {
     validateThisInternalField(this, kWorker, 'Worker');
     // "The terminate() method steps are to terminate a worker given this's
     // worker."
+    // Closing a port is asynchronous. Remove its forwarding listeners too,
+    // since terminate() can run while the backing thread drains queued messages.
+    this[kRemoveMessageListeners]?.();
+    this[kOutsidePort].close();
     this[kWorker]?.terminate();
   }
 
@@ -883,7 +892,7 @@ class Worker extends EventTarget {
     // invoked the respective postMessage(message, transfer) and
     // postMessage(message, options) on this's outside port, with the same
     // arguments, and returned the same return value."
-    this[kWorker]?.postMessage(message, transfer);
+    this[kOutsidePort].postMessage(message, transfer);
   }
 
   // The following properties are non-standard, Node.js extensions

test/parallel/test-webworker-blob-module-url.js

@@ -0,0 +1,19 @@
+// Flags: --experimental-web-worker
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto) common.skip('missing crypto');
+
+const assert = require('node:assert');
+const source = `
+  import value from 'data:text/javascript,export default 42';
+  postMessage([import.meta.url, location.href, value]);
+`;
+const url = URL.createObjectURL(new Blob([source], { type: 'text/javascript' }));
+const worker = new Worker(url, { type: 'module' });
+URL.revokeObjectURL(url);
+worker.onerror = common.mustNotCall('worker failed');
+worker.onmessage = common.mustCall(({ data }) => {
+  worker.terminate();
+  assert.deepStrictEqual(data, [url, url, 42]);
+});

test/parallel/test-webworker-importscripts-conversion.js

@@ -0,0 +1,67 @@
+// Flags: --experimental-web-worker
+'use strict';
+
+const common = require('../common');
+const assert = require('node:assert');
+
+for (const type of ['classic', 'module']) {
+  const source = `
+    const results = [];
+    const conversions = [];
+    const sentinel = new Error('conversion');
+    try {
+      importScripts('https://[', {
+        toString() { conversions.push('converted'); throw sentinel; }
+      });
+    } catch (error) {
+      results.push([conversions, error === sentinel]);
+    }
+    const order = [];
+    try {
+      importScripts(
+        { toString() { order.push(1); return 'https://['; } },
+        { toString() { order.push(2); return 'data:text/javascript,'; } }
+      );
+    } catch (error) {
+      results.push([order, error.name]);
+    }
+    postMessage(results);
+  `;
+  const worker = new Worker(`data:text/javascript,${encodeURIComponent(source)}`, { type });
+  worker.onerror = common.mustNotCall('worker failed');
+  worker.onmessage = common.mustCall(({ data }) => {
+    worker.terminate();
+    assert.deepStrictEqual(data, [
+      [['converted'], true],
+      [[1, 2], type === 'module' ? 'TypeError' : 'SyntaxError'],
+    ]);
+  });
+}
+
+if (common.hasCrypto) {
+  // URL parsing must capture blob entries after all arguments are converted.
+  const source = `
+    self.ran = false;
+    const url = URL.createObjectURL(new Blob(['self.ran = true'], {
+      type: 'text/javascript'
+    }));
+    let errorName;
+    try {
+      importScripts(url, {
+        toString() {
+          URL.revokeObjectURL(url);
+          return 'data:text/javascript,';
+        }
+      });
+    } catch (error) {
+      errorName = error.name;
+    }
+    postMessage([self.ran, errorName]);
+  `;
+  const worker = new Worker(`data:text/javascript,${encodeURIComponent(source)}`);
+  worker.onerror = common.mustNotCall('worker failed');
+  worker.onmessage = common.mustCall(({ data }) => {
+    worker.terminate();
+    assert.deepStrictEqual(data, [false, 'NetworkError']);
+  });
+}

test/parallel/test-webworker-postmessage-lifecycle.js

@@ -0,0 +1,78 @@
+// Flags: --experimental-web-worker
+'use strict';
+
+const common = require('../common');
+const assert = require('node:assert');
+
+function checkSerialization(worker) {
+  assert.throws(() => worker.postMessage(() => {}), { name: 'DataCloneError' });
+  const buffer = new ArrayBuffer(8);
+  assert.throws(() => worker.postMessage(null, [buffer, buffer]), { name: 'DataCloneError' });
+  assert.strictEqual(buffer.byteLength, 8);
+  worker.postMessage(null, [buffer]);
+  assert.strictEqual(buffer.byteLength, 0);
+}
+
+// A failed script fetch still leaves an outside port that serializes messages.
+{
+  const worker = new Worker('data:text/plain,');
+  worker.onerror = common.mustCall(() => checkSerialization(worker));
+  checkSerialization(worker);
+}
+
+// Serialization is required even after the backing thread has exited.
+{
+  const worker = new Worker('data:text/javascript,close()');
+  worker.onerror = common.mustNotCall('worker failed');
+  process.once('worker', common.mustCall((thread) => {
+    thread.once('exit', common.mustCall(() => checkSerialization(worker)));
+  }));
+}
+
+// Queue messages before terminating so the test does not depend on thread speed.
+{
+  const source = `
+    onmessage = ({ data }) => {
+      for (let i = 0; i < 3; i++) postMessage(i);
+      const state = new Int32Array(data);
+      Atomics.store(state, 0, 1);
+      Atomics.notify(state, 0);
+    };
+  `;
+  const worker = new Worker(`data:text/javascript,${encodeURIComponent(source)}`);
+  worker.onerror = common.mustNotCall('worker failed');
+  worker.onmessage = common.mustNotCall('message delivered after terminate()');
+  const state = new Int32Array(new SharedArrayBuffer(4));
+  worker.postMessage(state.buffer);
+  assert.notStrictEqual(Atomics.wait(state, 0, 0, common.platformTimeout(10000)), 'timed-out');
+  worker.terminate();
+  worker.terminate();
+  checkSerialization(worker);
+}
+
+// terminate() can run during message dispatch, including while the backing
+// thread is draining messages on exit. Finish this event but discard later ones.
+for (const closeAfterPosting of [false, true]) {
+  const source = `
+    onmessage = ({ data }) => {
+      for (let i = 0; i < 3; i++) postMessage(i);
+      const state = new Int32Array(data);
+      Atomics.store(state, 0, 1);
+      Atomics.notify(state, 0);
+      if (${closeAfterPosting}) close();
+    };
+  `;
+  const worker = new Worker(`data:text/javascript,${encodeURIComponent(source)}`);
+  worker.onerror = common.mustNotCall('worker failed');
+  worker.addEventListener('message', common.mustCall(({ data }) => {
+    assert.strictEqual(data, 0);
+    worker.terminate();
+    checkSerialization(worker);
+  }));
+  worker.addEventListener('message', common.mustCall(({ data }) => {
+    assert.strictEqual(data, 0);
+  }));
+  const state = new Int32Array(new SharedArrayBuffer(4));
+  worker.postMessage(state.buffer);
+  assert.notStrictEqual(Atomics.wait(state, 0, 0, common.platformTimeout(10000)), 'timed-out');
+}