Bug 1662925 - Make SW fetch events properly queue. r=dom-workers-and-storage-reviewers,janv

Excerpting some documentation I wrote up for an OVERVIEW.md for Service
Workers for this exact use situation below.  The basic situation of this patch
is that we were trying to create the FetchEventOpProxyParent immediately
before the managing PRemoteWorker instance existed, which isn't a thing we can
do.  Because FetchEvent ops are more complicated they require somewhat more
unique handling, but it should have been unified with the PendingOp
infrastructure and was not.  The one notable thing going on actor-wise is that
the request body (if present) requires special RemoteLazyInputStream
serialization and this is something that can only be done once we have the
RemoteWorkerParent.  See https://phabricator.services.mozilla.com/D73173 and
its commit message and my "Restating" blocks for more context.


### Threads and Proxies

#### Main Thread

ServiceWorkerManager's authoritative ServiceWorker state lives on the main
thread in the parent process.  This is due to a combination of legacy factors
and that currently the nsHttpChannel AsyncOpen logic where navigation
interception occurs must be on the main thread.

#### IPDL Background Thread

The IPDL Background Thread is the thread where PBackground parent actors are
created.  Because IPDL actors are explicitly tied to the thread they are created
on and PBackground is the only top-level protocol created for use by DOM
Workers, this thread is the natural home for book-keeping and authoritative
state for APIs that are accessed via PBackground-managed protocols.  For
example, IndexedDB and other QuotaManager-managed storage APIs.

The Remote Worker APIs are all PBackground managed and so all messages sent via
the Remote Worker API need to be sent from the IPDL Background thread.

#### Main Thread to IPDL Background Proxies

There are 2 ways to get data from the main thread to the IPDL Background thread:
either via direct runnable dispatch or via IPDL IPC.  We use IPDL IPC (which
has optimizations for same-process communication).

The following interfaces exist exclusively to proxy requests from the
ServiceWorkerManager on the main thread to the IPDL Background thread.
- `PRemoteWorkerController` is a proxy wrapper around the
  `RemoteWorkerController` API exposed on the IPDL Background thread.
- `PFetchEventOp` is paired with `PFetchEventOpProxy` managed by the above
  `PRemoteWorkerController`.  `PFetchEventOp` gets the data to the IPDL
  Background thread from the main thread, then `PFetchEventOpProxy` gets the
  data down to the content process.

## Non-Fetch ServiceWorker events AKA ExtendableEvents
How non-fetch events are dispatched to the serviceworker (including the IPC).

Because ServiceWorkers are intended to be shutdown and restarted on demand and
most event processing is asynchronous, there needs to be a way to track
outstanding requests and for content logic to indicate when it is done
processing requests.  `ExtendableEvent`s are the mechanism for this.  A method
`waitUntil(promise)` adds promises to be track as long as the event is still
"active".

This straightforward lifecycle means that these events map well to our IPC
async return value mechanism.  This is used by
`PRemoteWorker::ExecServiceWorkerOp`.

## Fetch events and FetchEvent.respondWith()

`FetchEvent`s have a different lifecycle and dataflow than regular
`ExtendableEvents`.  They expose a `respondWith()` method that will eventually
resolve with a fetch `Response` object that potentially needs to be propagated
before the ExtendableEvent is no longer active.  (The response will never be
propagated after `waitUntil()` settles because every call to `respondWith()`
implicitly calls `waitUntil()`.)

From an IPC perspective, this means that `FetchEvent` instances need their own
IPC actor rather than being able to depend on the async return value mechanism
of IPDL.  That's why `PFetchEventOpProxy` exists and is managed by
`PRemoteWorker`.

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

dom/serviceworkers/FetchEventOpParent.cpp

@@ -22,26 +22,6 @@ using namespace ipc;
 
 namespace dom {
 
-void FetchEventOpParent::Initialize(
-    const ServiceWorkerFetchEventOpArgs& aArgs) {
-  AssertIsInMainProcess();
-  AssertIsOnBackgroundThread();
-
-  RemoteWorkerControllerParent* manager =
-      static_cast<RemoteWorkerControllerParent*>(Manager());
-  MOZ_ASSERT(manager);
-
-  // This will be null when the manager's RemoteWorkerController has shutdown.
-  RefPtr<RemoteWorkerParent> proxyManager = manager->GetRemoteWorkerParent();
-  if (NS_WARN_IF(!proxyManager)) {
-    Unused << Send__delete__(this, NS_ERROR_DOM_ABORT_ERR);
-
-    return;
-  }
-
-  FetchEventOpProxyParent::Create(proxyManager.get(), aArgs, this);
-}
-
 void FetchEventOpParent::ActorDestroy(ActorDestroyReason) {
   AssertIsOnBackgroundThread();
 }

dom/serviceworkers/FetchEventOpParent.h

@@ -24,8 +24,6 @@ class FetchEventOpParent final : public PFetchEventOpParent {
 
   FetchEventOpParent() = default;
 
-  void Initialize(const ServiceWorkerFetchEventOpArgs& aArgs);
-
  private:
   ~FetchEventOpParent() = default;
 

dom/serviceworkers/FetchEventOpProxyParent.cpp

@@ -61,38 +61,22 @@ nsresult MaybeDeserializeAndWrapForMainThread(
 }  // anonymous namespace
 
 /* static */ void FetchEventOpProxyParent::Create(
-    PRemoteWorkerParent* aManager, const ServiceWorkerFetchEventOpArgs& aArgs,
-    RefPtr<FetchEventOpParent> aReal) {
+    PRemoteWorkerParent* aManager,
+    RefPtr<ServiceWorkerFetchEventOpPromise::Private>&& aPromise,
+    const ServiceWorkerFetchEventOpArgs& aArgs,
+    RefPtr<FetchEventOpParent> aReal, nsCOMPtr<nsIInputStream> aBodyStream) {
   AssertIsInMainProcess();
   AssertIsOnBackgroundThread();
   MOZ_ASSERT(aManager);
   MOZ_ASSERT(aReal);
 
   FetchEventOpProxyParent* actor =
-      new FetchEventOpProxyParent(std::move(aReal));
-
-  if (aArgs.internalRequest().body().isNothing()) {
-    Unused << aManager->SendPFetchEventOpProxyConstructor(actor, aArgs);
-    return;
-  }
+      new FetchEventOpProxyParent(std::move(aReal), std::move(aPromise));
 
   ServiceWorkerFetchEventOpArgs copyArgs = aArgs;
   IPCInternalRequest& copyRequest = copyArgs.internalRequest();
 
-  if (copyRequest.body().ref().type() ==
-      BodyStreamVariant::TParentToParentStream) {
-    nsCOMPtr<nsIInputStream> stream;
-    auto streamLength = copyRequest.bodySize();
-    const auto& uuid =
-        copyRequest.body().ref().get_ParentToParentStream().uuid();
-
-    auto storage = RemoteLazyInputStreamStorage::Get().unwrapOr(nullptr);
-    MOZ_DIAGNOSTIC_ASSERT(storage);
-    storage->GetStream(uuid, 0, streamLength, getter_AddRefs(stream));
-    storage->ForgetStream(uuid);
-
-    MOZ_DIAGNOSTIC_ASSERT(stream);
-
+  if (aBodyStream) {
     PBackgroundParent* bgParent = aManager->Manager();
     MOZ_ASSERT(bgParent);
 
@@ -100,7 +84,7 @@ nsresult MaybeDeserializeAndWrapForMainThread(
 
     RemoteLazyStream ipdlStream;
     MOZ_ALWAYS_SUCCEEDS(RemoteLazyInputStreamUtils::SerializeInputStream(
-        stream, streamLength, ipdlStream, bgParent));
+        aBodyStream, copyRequest.bodySize(), ipdlStream, bgParent));
 
     copyRequest.body().ref().get_ParentToChildStream().actorParent() =
         ipdlStream;
@@ -114,8 +98,9 @@ FetchEventOpProxyParent::~FetchEventOpProxyParent() {
 }
 
 FetchEventOpProxyParent::FetchEventOpProxyParent(
-    RefPtr<FetchEventOpParent>&& aReal)
-    : mReal(std::move(aReal)) {}
+    RefPtr<FetchEventOpParent>&& aReal,
+    RefPtr<ServiceWorkerFetchEventOpPromise::Private>&& aPromise)
+    : mReal(std::move(aReal)), mLifetimePromise(std::move(aPromise)) {}
 
 mozilla::ipc::IPCResult FetchEventOpProxyParent::RecvAsyncLog(
     const nsCString& aScriptSpec, const uint32_t& aLineNumber,
@@ -177,16 +162,25 @@ mozilla::ipc::IPCResult FetchEventOpProxyParent::RecvRespondWith(
 mozilla::ipc::IPCResult FetchEventOpProxyParent::Recv__delete__(
     const ServiceWorkerFetchEventOpResult& aResult) {
   AssertIsOnBackgroundThread();
+  MOZ_ASSERT(mLifetimePromise);
   MOZ_ASSERT(mReal);
 
-  Unused << mReal->Send__delete__(mReal, aResult);
-  mReal = nullptr;
+  if (mLifetimePromise) {
+    mLifetimePromise->Resolve(aResult, __func__);
+    mLifetimePromise = nullptr;
+    mReal = nullptr;
+  }
 
   return IPC_OK();
 }
 
 void FetchEventOpProxyParent::ActorDestroy(ActorDestroyReason) {
   AssertIsOnBackgroundThread();
+  if (mLifetimePromise) {
+    mLifetimePromise->Reject(NS_ERROR_DOM_ABORT_ERR, __func__);
+    mLifetimePromise = nullptr;
+    mReal = nullptr;
+  }
 }
 
 }  // namespace dom

dom/serviceworkers/FetchEventOpProxyParent.h

@@ -9,6 +9,7 @@
 
 #include "mozilla/RefPtr.h"
 #include "mozilla/dom/PFetchEventOpProxyParent.h"
+#include "mozilla/dom/ServiceWorkerOpPromise.h"
 
 namespace mozilla {
 namespace dom {
@@ -18,21 +19,30 @@ class PRemoteWorkerParent;
 class ServiceWorkerFetchEventOpArgs;
 
 /**
- * FetchEventOpProxyParent owns a FetchEventOpParent and is responsible for
- * calling PFetchEventOpParent::Send__delete__.
+ * FetchEventOpProxyParent owns a FetchEventOpParent in order to propagate
+ * the respondWith() value by directly calling SendRespondWith on the
+ * FetchEventOpParent, but the call to Send__delete__ is handled via MozPromise.
+ * This is done because this actor may only be created after its managing
+ * PRemoteWorker is created, which is asynchronous and may fail.  We take on
+ * responsibility for the promise once we are created, but we may not be created
+ * if the RemoteWorker is never successfully launched.
  */
 class FetchEventOpProxyParent final : public PFetchEventOpProxyParent {
   friend class PFetchEventOpProxyParent;
 
  public:
-  static void Create(PRemoteWorkerParent* aManager,
-                     const ServiceWorkerFetchEventOpArgs& aArgs,
-                     RefPtr<FetchEventOpParent> aReal);
+  static void Create(
+      PRemoteWorkerParent* aManager,
+      RefPtr<ServiceWorkerFetchEventOpPromise::Private>&& aPromise,
+      const ServiceWorkerFetchEventOpArgs& aArgs,
+      RefPtr<FetchEventOpParent> aReal, nsCOMPtr<nsIInputStream> aBodyStream);
 
   ~FetchEventOpProxyParent();
 
  private:
-  explicit FetchEventOpProxyParent(RefPtr<FetchEventOpParent>&& aReal);
+  FetchEventOpProxyParent(
+      RefPtr<FetchEventOpParent>&& aReal,
+      RefPtr<ServiceWorkerFetchEventOpPromise::Private>&& aPromise);
 
   mozilla::ipc::IPCResult RecvAsyncLog(const nsCString& aScriptSpec,
                                        const uint32_t& aLineNumber,
@@ -49,6 +59,7 @@ class FetchEventOpProxyParent final : public PFetchEventOpProxyParent {
   void ActorDestroy(ActorDestroyReason) override;
 
   RefPtr<FetchEventOpParent> mReal;
+  RefPtr<ServiceWorkerFetchEventOpPromise::Private> mLifetimePromise;
 };
 
 }  // namespace dom

dom/serviceworkers/ServiceWorkerOpPromise.h

@@ -31,6 +31,9 @@ using FetchEventRespondWithPromise =
 using ServiceWorkerOpPromise =
     MozPromise<ServiceWorkerOpResult, nsresult, true>;
 
+using ServiceWorkerFetchEventOpPromise =
+    MozPromise<ServiceWorkerFetchEventOpResult, nsresult, true>;
+
 }  // namespace dom
 }  // namespace mozilla
 

dom/workers/remoteworkers/RemoteWorkerController.cpp

@@ -12,7 +12,11 @@
 
 #include "mozilla/Assertions.h"
 #include "mozilla/DebugOnly.h"
+#include "mozilla/Maybe.h"
+#include "mozilla/RemoteLazyInputStreamUtils.h"
+#include "mozilla/RemoteLazyInputStreamStorage.h"
 #include "mozilla/ScopeExit.h"
+#include "mozilla/dom/FetchEventOpProxyParent.h"
 #include "mozilla/dom/MessagePortParent.h"
 #include "mozilla/dom/RemoteWorkerTypes.h"
 #include "mozilla/dom/ServiceWorkerCloneData.h"
@@ -279,6 +283,26 @@ RefPtr<ServiceWorkerOpPromise> RemoteWorkerController::ExecServiceWorkerOp(
   return promise;
 }
 
+RefPtr<ServiceWorkerFetchEventOpPromise>
+RemoteWorkerController::ExecServiceWorkerFetchEventOp(
+    const ServiceWorkerFetchEventOpArgs& aArgs,
+    RefPtr<FetchEventOpParent> aReal) {
+  AssertIsOnBackgroundThread();
+  MOZ_ASSERT(mIsServiceWorker);
+
+  RefPtr<ServiceWorkerFetchEventOpPromise::Private> promise =
+      new ServiceWorkerFetchEventOpPromise::Private(__func__);
+
+  UniquePtr<PendingSWFetchEventOp> op =
+      MakeUnique<PendingSWFetchEventOp>(aArgs, promise, std::move(aReal));
+
+  if (!op->MaybeStart(this)) {
+    mPendingOps.AppendElement(std::move(op));
+  }
+
+  return promise;
+}
+
 RefPtr<GenericPromise> RemoteWorkerController::SetServiceWorkerSkipWaitingFlag()
     const {
   AssertIsOnBackgroundThread();
@@ -485,5 +509,77 @@ void RemoteWorkerController::PendingServiceWorkerOp::Cancel() {
   mPromise = nullptr;
 }
 
+RemoteWorkerController::PendingSWFetchEventOp::PendingSWFetchEventOp(
+    const ServiceWorkerFetchEventOpArgs& aArgs,
+    RefPtr<ServiceWorkerFetchEventOpPromise::Private> aPromise,
+    RefPtr<FetchEventOpParent>&& aReal)
+    : mArgs(aArgs), mPromise(std::move(aPromise)), mReal(aReal) {
+  AssertIsOnBackgroundThread();
+  MOZ_ASSERT(mPromise);
+
+  // If there is a TParentToParentStream in the request body, we need to
+  // save it to our stream.
+  IPCInternalRequest& req = mArgs.internalRequest();
+  if (req.body().isSome() &&
+      req.body().ref().type() == BodyStreamVariant::TParentToParentStream) {
+    nsCOMPtr<nsIInputStream> stream;
+    auto streamLength = req.bodySize();
+    const auto& uuid = req.body().ref().get_ParentToParentStream().uuid();
+
+    auto storage = RemoteLazyInputStreamStorage::Get().unwrapOr(nullptr);
+    MOZ_DIAGNOSTIC_ASSERT(storage);
+    storage->GetStream(uuid, 0, streamLength, getter_AddRefs(mBodyStream));
+    storage->ForgetStream(uuid);
+
+    MOZ_DIAGNOSTIC_ASSERT(mBodyStream);
+
+    req.body() = Nothing();
+  }
+}
+
+RemoteWorkerController::PendingSWFetchEventOp::~PendingSWFetchEventOp() {
+  AssertIsOnBackgroundThread();
+  MOZ_DIAGNOSTIC_ASSERT(!mPromise);
+}
+
+bool RemoteWorkerController::PendingSWFetchEventOp::MaybeStart(
+    RemoteWorkerController* const aOwner) {
+  AssertIsOnBackgroundThread();
+  MOZ_ASSERT(mPromise);
+  MOZ_ASSERT(aOwner);
+
+  if (NS_WARN_IF(aOwner->mState == RemoteWorkerController::eTerminated)) {
+    mPromise->Reject(NS_ERROR_DOM_ABORT_ERR, __func__);
+    mPromise = nullptr;
+    // Because the worker has transitioned to terminated, this operation is moot
+    // and so we should return true because there's no need to queue it.
+    return true;
+  }
+
+  // The target content process must still be starting up.
+  if (!aOwner->mActor) {
+    MOZ_ASSERT(aOwner->mState == RemoteWorkerController::ePending);
+    return false;
+  }
+
+  // At this point we are handing off responsibility for the promise to the
+  // actor.
+  FetchEventOpProxyParent::Create(aOwner->mActor.get(), std::move(mPromise),
+                                  mArgs, std::move(mReal),
+                                  std::move(mBodyStream));
+
+  return true;
+}
+
+void RemoteWorkerController::PendingSWFetchEventOp::Cancel() {
+  AssertIsOnBackgroundThread();
+  MOZ_ASSERT(mPromise);
+
+  if (mPromise) {
+    mPromise->Reject(NS_ERROR_DOM_ABORT_ERR, __func__);
+    mPromise = nullptr;
+  }
+}
+
 }  // namespace dom
 }  // namespace mozilla

dom/workers/remoteworkers/RemoteWorkerController.h

@@ -86,6 +86,7 @@ namespace dom {
  */
 
 class ErrorValue;
+class FetchEventOpParent;
 class RemoteWorkerControllerParent;
 class RemoteWorkerData;
 class RemoteWorkerManager;
@@ -135,6 +136,10 @@ class RemoteWorkerController final {
   RefPtr<ServiceWorkerOpPromise> ExecServiceWorkerOp(
       ServiceWorkerOpArgs&& aArgs);
 
+  RefPtr<ServiceWorkerFetchEventOpPromise> ExecServiceWorkerFetchEventOp(
+      const ServiceWorkerFetchEventOpArgs& aArgs,
+      RefPtr<FetchEventOpParent> aReal);
+
   RefPtr<GenericPromise> SetServiceWorkerSkipWaitingFlag() const;
 
   bool IsTerminated() const;
@@ -193,12 +198,24 @@ class RemoteWorkerController final {
     virtual ~PendingOp() = default;
 
     /**
-     * Returns `true` if execution has started and `false` otherwise.
+     * Returns `true` if execution has started or the operation is moot and
+     * doesn't need to be queued, `false` if execution hasn't started and the
+     * operation should be queued.  In general, operations should only return
+     * false when a remote worker is first starting up.  Operations may also
+     * somewhat non-intuitively return true without doing anything if the worker
+     * has already been told to shutdown.
      *
      * Starting execution may depend the state of `aOwner.`
      */
     virtual bool MaybeStart(RemoteWorkerController* const aOwner) = 0;
 
+    /**
+     * Invoked if the operation will never have MaybeStart() called again
+     * because the RemoteWorkerController has terminated (or will never start).
+     * This should be used by PendingOps to clean up any resources they own and
+     * may also be called internally by their MaybeStart() methods if they
+     * determine the worker has been terminated.  This should be idempotent.
+     */
     virtual void Cancel() = 0;
   };
 
@@ -249,6 +266,36 @@ class RemoteWorkerController final {
     RefPtr<ServiceWorkerOpPromise::Private> mPromise;
   };
 
+  /**
+   * Custom pending op type to deal with the complexities of FetchEvents having
+   * their own actor.
+   *
+   * FetchEvent Ops have their own actor type because their lifecycle is more
+   * complex than IPDL's async return value mechanism allows.  Additionally,
+   * its IPC struct potentially has to serialize RemoteLazyStreams which
+   * requires us to hold an nsIInputStream when at rest and serialize it when
+   * eventually sending.
+   */
+  class PendingSWFetchEventOp final : public PendingOp {
+   public:
+    PendingSWFetchEventOp(
+        const ServiceWorkerFetchEventOpArgs& aArgs,
+        RefPtr<ServiceWorkerFetchEventOpPromise::Private> aPromise,
+        RefPtr<FetchEventOpParent>&& aReal);
+
+    ~PendingSWFetchEventOp();
+
+    bool MaybeStart(RemoteWorkerController* const aOwner) override;
+
+    void Cancel() override;
+
+   private:
+    ServiceWorkerFetchEventOpArgs mArgs;
+    RefPtr<ServiceWorkerFetchEventOpPromise::Private> mPromise;
+    RefPtr<FetchEventOpParent> mReal;
+    nsCOMPtr<nsIInputStream> mBodyStream;
+  };
+
   nsTArray<UniquePtr<PendingOp>> mPendingOps;
 };
 

dom/workers/remoteworkers/RemoteWorkerControllerParent.cpp

@@ -80,7 +80,22 @@ IPCResult RemoteWorkerControllerParent::RecvPFetchEventOpConstructor(
   AssertIsOnBackgroundThread();
   MOZ_ASSERT(aActor);
 
-  (static_cast<FetchEventOpParent*>(aActor))->Initialize(aArgs);
+  RefPtr<FetchEventOpParent> realFetchOp =
+      static_cast<FetchEventOpParent*>(aActor);
+  mRemoteWorkerController->ExecServiceWorkerFetchEventOp(aArgs, realFetchOp)
+      ->Then(
+          GetCurrentSerialEventTarget(), __func__,
+          [fetchOp = std::move(realFetchOp)](
+              ServiceWorkerFetchEventOpPromise::ResolveOrRejectValue&&
+                  aResult) {
+            if (NS_WARN_IF(aResult.IsReject())) {
+              MOZ_ASSERT(NS_FAILED(aResult.RejectValue()));
+              Unused << fetchOp->Send__delete__(fetchOp, aResult.RejectValue());
+              return;
+            }
+
+            Unused << fetchOp->Send__delete__(fetchOp, aResult.ResolveValue());
+          });
 
   return IPC_OK();
 }