Bug 1858809 - Enforce Worker::PostEventWithOptions invariants. r=bwc,dom-worker-reviewers,smaug

Under debug/fuzzing builds WorkerRunnable will assert in its constructor
if passed a null WorkerPrivate.  While there is no runtime problem under
non-debug builds, it is a reasonable invariant for us to have until we
are able to eliminate to turn all of this into normal runnables.

`RTCRtpScriptTransform::Constructor` currently can get into this situation
where the method is invoked but the Worker is in a closing / terminated
state.  The spec has a "FIXME" to specify what to do in this situation.

This patch implements an early return with an error based on adding a new
`Worker::IsEligibleForMessaging` which is analogous to
`nsIGlobalObject::IsEligibleForMessaging`.  This is potentially
observable since JS getters or proxies passed to the constructor as the
`options` arg will not be serialized in this case, but it is not a
secret to the inherently same-origin Worker whether the Worker is
closing; the same information is available via WebLocks among other
methods.  I chose the simplest path here given the absence of any spec
language or even an issue on the repo that I can see.

Differential Revision: https://phabricator.services.mozilla.com/D205779

dom/media/webrtc/jsapi/RTCRtpScriptTransform.cpp

@@ -47,6 +47,22 @@ already_AddRefed<RTCRtpScriptTransform> RTCRtpScriptTransform::Constructor(
     aRv.Throw(NS_ERROR_FAILURE);
     return nullptr;
   }
+
+  // The spec currently fails to describe what to do when the worker is closing
+  // or closed; the following placeholder text can be found in the spec at:
+  // https://w3c.github.io/webrtc-encoded-transform/#dom-rtcrtpscripttransform-rtcrtpscripttransform
+  //
+  // > FIXME: Describe error handling (worker closing flag true at
+  // > RTCRtpScriptTransform creation time. And worker being terminated while
+  // > transform is processing data).
+  //
+  // Because our worker runnables do not like to be pointed at a nonexistant
+  // worker, we throw in this case.
+  if (!aWorker.IsEligibleForMessaging()) {
+    aRv.Throw(NS_ERROR_FAILURE);
+    return nullptr;
+  }
+
   auto newTransform = MakeRefPtr<RTCRtpScriptTransform>(ownerWindow);
   RefPtr<RTCTransformEventRunnable> runnable =
       new RTCTransformEventRunnable(aWorker, &newTransform->GetProxy());

dom/workers/Worker.cpp

@@ -83,6 +83,12 @@ JSObject* Worker::WrapObject(JSContext* aCx,
   return wrapper;
 }
 
+bool Worker::IsEligibleForMessaging() {
+  NS_ASSERT_OWNINGTHREAD(Worker);
+
+  return mWorkerPrivate && mWorkerPrivate->ParentStatusProtected() <= Running;
+}
+
 void Worker::PostMessage(JSContext* aCx, JS::Handle<JS::Value> aMessage,
                          const Sequence<JSObject*>& aTransferable,
                          ErrorResult& aRv) {

dom/workers/Worker.h

@@ -42,6 +42,31 @@ class Worker : public DOMEventTargetHelper, public SupportsWeakPtr {
     return Some(EventCallbackDebuggerNotificationType::Worker);
   }
 
+  // True if the worker is not yet closing from the perspective of this, the
+  // owning thread, and therefore it's okay to post a message to the worker.
+  // This is not a guarantee that the worker will process the message.
+  //
+  // This method will return false if `globalThis.close()` is invoked on the
+  // worker before that method returns control to the caller and without waiting
+  // for any task to be queued on this thread and run; this biases us to avoid
+  // doing wasteful work but does mean if you are exposing something to content
+  // that is specified to only transition as the result of a task, then you
+  // should not use this method.
+  //
+  // The method name comes from
+  // https://html.spec.whatwg.org/multipage/web-messaging.html#eligible-for-messaging
+  // and is intended to convey whether it's okay to begin to take the steps to
+  // create an `EventWithOptionsRunnable` to pass to `PostEventWithOptions`.
+  // Note that early returning based on calling this method without performing
+  // the structured serialization steps that would otherwise run is potentially
+  // observable to content if content is in control of any of the payload in
+  // such a way that an object with getters or a proxy could be provided.
+  //
+  // There is an identically named method on nsIGlobalObject and the semantics
+  // are intentionally similar but please make sure you document your
+  // assumptions when calling either method.
+  bool IsEligibleForMessaging();
+
   void PostMessage(JSContext* aCx, JS::Handle<JS::Value> aMessage,
                    const Sequence<JSObject*>& aTransferable, ErrorResult& aRv);
 
@@ -49,6 +74,8 @@ class Worker : public DOMEventTargetHelper, public SupportsWeakPtr {
                    const StructuredSerializeOptions& aOptions,
                    ErrorResult& aRv);
 
+  // Callers must call `IsEligibleForMessaging` before constructing an
+  // `EventWithOptionsRunnable` subclass.
   void PostEventWithOptions(JSContext* aCx, JS::Handle<JS::Value> aOptions,
                             const Sequence<JSObject*>& aTransferable,
                             EventWithOptionsRunnable* aRunnable,

dom/workers/test/crashtests/1858809.html

@@ -0,0 +1,14 @@
+<!DOCTYPE>
+<html>
+<head>
+<meta charset="UTF-8">
+<script>
+document.addEventListener("DOMContentLoaded", () => {
+  const a = new Worker("", {})
+  const b = new Blob([""], {})
+  a.terminate()
+  new RTCRtpScriptTransform(a, b, [{}])
+})
+</script>
+</head>
+</html>

dom/workers/test/crashtests/crashtests.list

@@ -6,3 +6,4 @@ load 1228456.html
 load 1348882.html
 load 1821066.html
 load 1819146.html
+load 1858809.html