Bug 1381748 - Cleanup FetchConsumer workflow - part 3 - shutdown workflow, r=catalinb
authorAndrea Marchesini <amarchesini@mozilla.com>
Mon, 04 Sep 2017 17:06:47 +0200
changeset 428358 c309a6360b2f662758ce201d6aa034daf654cee9
parent 428357 30aeba7deb29b08965e8e76882cbc1a0c893b779
child 428359 60fd99ec9e7122492a243b8bce904db586a930ed
push id7761
push userjlund@mozilla.com
push dateFri, 15 Sep 2017 00:19:52 +0000
treeherdermozilla-beta@c38455951db4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerscatalinb
bugs1381748
milestone57.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1381748 - Cleanup FetchConsumer workflow - part 3 - shutdown workflow, r=catalinb
dom/fetch/FetchConsumer.cpp
dom/fetch/FetchConsumer.h
dom/tests/mochitest/fetch/mochitest.ini
--- a/dom/fetch/FetchConsumer.cpp
+++ b/dom/fetch/FetchConsumer.cpp
@@ -37,19 +37,17 @@ public:
 
   ~FetchBodyWorkerHolder() = default;
 
   bool Notify(workers::Status aStatus) override
   {
     MOZ_ASSERT(aStatus > workers::Running);
     if (!mWasNotified) {
       mWasNotified = true;
-      // This will probably cause the releasing of the consumer.
-      // The WorkerHolder will be released as well.
-      mConsumer->ContinueConsumeBody(NS_BINDING_ABORTED, 0, nullptr);
+      mConsumer->ShutDownMainThreadConsuming();
     }
 
     return true;
   }
 };
 
 template <class Derived>
 class BeginConsumeBodyRunnable final : public Runnable
@@ -210,22 +208,16 @@ public:
                    const uint8_t* aResult) override
   {
     MOZ_ASSERT(NS_IsMainThread());
 
     // The loading is completed. Let's nullify the pump before continuing the
     // consuming of the body.
     mFetchBodyConsumer->NullifyConsumeBodyPump();
 
-    // If the binding requested cancel, we don't need to call
-    // ContinueConsumeBody, since that is the originator.
-    if (aStatus == NS_BINDING_ABORTED) {
-      return NS_OK;
-    }
-
     uint8_t* nonconstResult = const_cast<uint8_t*>(aResult);
     if (mFetchBodyConsumer->GetWorkerPrivate()) {
       RefPtr<ContinueConsumeBodyRunnable<Derived>> r =
         new ContinueConsumeBodyRunnable<Derived>(mFetchBodyConsumer,
                                                  aStatus,
                                                  aResultLength,
                                                  nonconstResult);
       if (!r->Dispatch()) {
@@ -247,16 +239,20 @@ public:
                                   nsresult aRv) override
   {
     // On error.
     if (NS_FAILED(aRv)) {
       OnStreamComplete(nullptr, nullptr, aRv, 0, nullptr);
       return;
     }
 
+    // The loading is completed. Let's nullify the pump before continuing the
+    // consuming of the body.
+    mFetchBodyConsumer->NullifyConsumeBodyPump();
+
     MOZ_ASSERT(aBlob);
 
     if (mFetchBodyConsumer->GetWorkerPrivate()) {
       RefPtr<ContinueConsumeBlobBodyRunnable<Derived>> r =
         new ContinueConsumeBlobBodyRunnable<Derived>(mFetchBodyConsumer,
                                                      aBlob->Impl());
 
       if (!r->Dispatch()) {
@@ -278,36 +274,16 @@ NS_IMPL_ADDREF(ConsumeBodyDoneObserver<D
 template <class Derived>
 NS_IMPL_RELEASE(ConsumeBodyDoneObserver<Derived>)
 template <class Derived>
 NS_INTERFACE_MAP_BEGIN(ConsumeBodyDoneObserver<Derived>)
   NS_INTERFACE_MAP_ENTRY(nsIStreamLoaderObserver)
   NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIStreamLoaderObserver)
 NS_INTERFACE_MAP_END
 
-template <class Derived>
-class ShutDownMainThreadConsumingRunnable final : public WorkerMainThreadRunnable
-{
-  RefPtr<FetchBodyConsumer<Derived>> mBodyConsumer;
-
-public:
-  explicit ShutDownMainThreadConsumingRunnable(FetchBodyConsumer<Derived>* aBodyConsumer)
-    : WorkerMainThreadRunnable(aBodyConsumer->GetWorkerPrivate(),
-                               NS_LITERAL_CSTRING("Fetch :: Cancel Pump"))
-    , mBodyConsumer(aBodyConsumer)
-  {}
-
-  bool
-  MainThreadRun() override
-  {
-    mBodyConsumer->ShutDownMainThreadConsuming();
-    return true;
-  }
-};
-
 } // anonymous
 
 template <class Derived>
 /* static */ already_AddRefed<Promise>
 FetchBodyConsumer<Derived>::Create(nsIGlobalObject* aGlobal,
                                    nsIEventTarget* aMainThreadEventTarget,
                                    FetchBody<Derived>* aBody,
                                    AbortSignal* aSignal,
@@ -481,23 +457,24 @@ FetchBodyConsumer<Derived>::RegisterWork
  * reflected in a lack of error return code.
  */
 template <class Derived>
 void
 FetchBodyConsumer<Derived>::BeginConsumeBodyMainThread()
 {
   AssertIsOnMainThread();
 
-  // Nothing to do.
+  AutoFailConsumeBody<Derived> autoReject(this);
+
   if (mShuttingDown) {
+    // We haven't started yet, but we have been terminated. AutoFailConsumeBody
+    // will dispatch a runnable to release resources.
     return;
   }
 
-  AutoFailConsumeBody<Derived> autoReject(this);
-
   nsCOMPtr<nsIInputStreamPump> pump;
   nsresult rv = NS_NewInputStreamPump(getter_AddRefs(pump),
                                       mBodyStream, -1, -1, 0, 0, false,
                                       mMainThreadEventTarget);
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return;
   }
 
@@ -570,21 +547,16 @@ FetchBodyConsumer<Derived>::ContinueCons
   auto autoReleaseObject = mozilla::MakeScopeExit([&] {
     self->ReleaseObject();
   });
 
   if (NS_WARN_IF(NS_FAILED(aStatus))) {
     localPromise->MaybeReject(NS_ERROR_DOM_ABORT_ERR);
   }
 
-  // We need to nullify mConsumeBodyPump on the main-thread and, in case it has
-  // not been created yet, we need to block the creation setting mShuttingDown
-  // to true.
-  ShutDownMainThreadConsuming();
-
   // Don't warn here since we warned above.
   if (NS_FAILED(aStatus)) {
     return;
   }
 
   // Finish successfully consuming body according to type.
   MOZ_ASSERT(aResult);
 
@@ -670,47 +642,36 @@ FetchBodyConsumer<Derived>::ContinueCons
 
   // Just a precaution to ensure ContinueConsumeBody is not called out of
   // sync with a body read.
   MOZ_ASSERT(mBody->BodyUsed());
 
   MOZ_ASSERT(mConsumePromise);
   RefPtr<Promise> localPromise = mConsumePromise.forget();
 
-  // Release the pump.
-  ShutDownMainThreadConsuming();
-
   RefPtr<dom::Blob> blob = dom::Blob::Create(mGlobal, aBlobImpl);
   MOZ_ASSERT(blob);
 
   localPromise->MaybeResolve(blob);
 
   ReleaseObject();
 }
 
 template <class Derived>
 void
 FetchBodyConsumer<Derived>::ShutDownMainThreadConsuming()
 {
   if (!NS_IsMainThread()) {
-    MOZ_ASSERT(mWorkerPrivate);
-    // In case of worker thread, we block the worker while the request is
-    // canceled on the main thread. This ensures that OnStreamComplete has a
-    // valid FetchConsumer around to call ShutDownMainThreadConsuming and we
-    // don't release the FetchConsumer on the main thread.
-    RefPtr<ShutDownMainThreadConsumingRunnable<Derived>> r =
-      new ShutDownMainThreadConsumingRunnable<Derived>(this);
+    RefPtr<FetchBodyConsumer<Derived>> self = this;
 
-    IgnoredErrorResult rv;
-    r->Dispatch(Killing, rv);
-    if (NS_WARN_IF(rv.Failed())) {
-      return;
-    }
+    nsCOMPtr<nsIRunnable> r = NS_NewRunnableFunction(
+      "FetchBodyConsumer::ShutDownMainThreadConsuming",
+      [self] () { self->ShutDownMainThreadConsuming(); });
 
-    MOZ_DIAGNOSTIC_ASSERT(mShuttingDown);
+    mMainThreadEventTarget->Dispatch(r.forget(), NS_DISPATCH_NORMAL);
     return;
   }
 
   // We need this because maybe, mConsumeBodyPump has not been created yet. We
   // must be sure that we don't try to do it.
   mShuttingDown = true;
 
   if (mConsumeBodyPump) {
--- a/dom/fetch/FetchConsumer.h
+++ b/dom/fetch/FetchConsumer.h
@@ -66,16 +66,17 @@ public:
   GetWorkerPrivate() const
   {
     return mWorkerPrivate;
   }
 
   void
   NullifyConsumeBodyPump()
   {
+    mShuttingDown = true;
     mConsumeBodyPump = nullptr;
   }
 
   // AbortFollower
   void Abort() override;
 
 private:
   FetchBodyConsumer(nsIEventTarget* aMainThreadEventTarget,
--- a/dom/tests/mochitest/fetch/mochitest.ini
+++ b/dom/tests/mochitest/fetch/mochitest.ini
@@ -51,21 +51,21 @@ support-files =
 [test_fetch_basic.html]
 [test_fetch_basic_sw_reroute.html]
 [test_fetch_basic_sw_empty_reroute.html]
 [test_fetch_basic_http.html]
 [test_fetch_basic_http_sw_reroute.html]
 [test_fetch_basic_http_sw_empty_reroute.html]
 [test_fetch_cached_redirect.html]
 [test_fetch_cors.html]
-skip-if = toolkit == 'android' && debug # Bug 1210282
+skip-if = toolkit == 'android' # Bug 1210282
 [test_fetch_cors_sw_reroute.html]
-skip-if = toolkit == 'android' && debug # Bug 1210282
+skip-if = toolkit == 'android' # Bug 1210282
 [test_fetch_cors_sw_empty_reroute.html]
-skip-if = toolkit == 'android' && debug # Bug 1210282
+skip-if = toolkit == 'android' # Bug 1210282
 [test_fetch_csp_block.html]
 [test_fetch_observer.html]
 [test_fetch_user_control_rp.html]
 [test_formdataparsing.html]
 skip-if = asan # Bug 1325942
 [test_formdataparsing_sw_reroute.html]
 skip-if = asan # Bug 1325942
 [test_request.html]