Bug 1416879 - Part 5: FetchStreamReader needs to cancel its reader when it encounters write errors. r=baku, f=bkelly
authorAndrew Sutherland <asutherland@asutherland.org>
Thu, 04 Jan 2018 18:09:32 -0500
changeset 450660 0cd43f3de5fb904e27d0dfefdfc10d1c58fea9da
parent 450659 58c617de48f4b47ffc4cd0f3f03537ff9baa0576
child 450661 1cc9cf654ea3bad4ce3a0e4e41d616da0c997625
push id8531
push userryanvm@gmail.com
push dateFri, 12 Jan 2018 16:47:01 +0000
treeherdermozilla-beta@0bc627ade5a0 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbaku
bugs1416879
milestone59.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 1416879 - Part 5: FetchStreamReader needs to cancel its reader when it encounters write errors. r=baku, f=bkelly Currently, FetchStreamReader never signals to the JS stream code that the reader has been closed. This means that when a ServiceWorker passes a ReadableStream to respondWith and the HTTP Channel gets canceled, the JS code will keep generating the stream without ever realizing the data's not going anywhere. It's necessary to cancel the reader. Or do something like that, this seems to work!
dom/fetch/FetchStreamReader.cpp
dom/fetch/FetchStreamReader.h
--- a/dom/fetch/FetchStreamReader.cpp
+++ b/dom/fetch/FetchStreamReader.cpp
@@ -29,17 +29,20 @@ public:
     , mReader(aReader)
     , mWasNotified(false)
   {}
 
   bool Notify(Status aStatus) override
   {
     if (!mWasNotified) {
       mWasNotified = true;
-      mReader->CloseAndRelease(NS_ERROR_DOM_INVALID_STATE_ERR);
+      // The WorkerPrivate does have a context available, and we could pass it
+      // here to trigger cancellation of the reader, but the author of this
+      // comment chickened out.
+      mReader->CloseAndRelease(nullptr, NS_ERROR_DOM_INVALID_STATE_ERR);
     }
 
     return true;
   }
 
 private:
   RefPtr<FetchStreamReader> mReader;
   bool mWasNotified;
@@ -119,31 +122,53 @@ FetchStreamReader::FetchStreamReader(nsI
   , mBufferOffset(0)
   , mStreamClosed(false)
 {
   MOZ_ASSERT(aGlobal);
 }
 
 FetchStreamReader::~FetchStreamReader()
 {
-  CloseAndRelease(NS_BASE_STREAM_CLOSED);
+  CloseAndRelease(nullptr, NS_BASE_STREAM_CLOSED);
 }
 
+// If a context is provided, an attempt will be made to cancel the reader.  The
+// only situation where we don't expect to have a context is when closure is
+// being triggered from the destructor or the WorkerHolder is notifying.  If
+// we're at the destructor, it's far too late to cancel anything.  And if the
+// WorkerHolder is being notified, the global is going away, so there's also
+// no need to do further JS work.
 void
-FetchStreamReader::CloseAndRelease(nsresult aStatus)
+FetchStreamReader::CloseAndRelease(JSContext* aCx, nsresult aStatus)
 {
   NS_ASSERT_OWNINGTHREAD(FetchStreamReader);
 
   if (mStreamClosed) {
     // Already closed.
     return;
   }
 
   RefPtr<FetchStreamReader> kungFuDeathGrip = this;
 
+  if (aCx) {
+    MOZ_ASSERT(mReader);
+
+    RefPtr<DOMException> error = DOMException::Create(aStatus);
+
+    JS::Rooted<JS::Value> errorValue(aCx);
+    if (ToJSValue(aCx, error, &errorValue)) {
+      JS::Rooted<JSObject*> reader(aCx, mReader);
+      // It's currently safe to cancel an already closed reader because, per the
+      // comments in ReadableStream::cancel() conveying the spec, step 2 of
+      // 3.4.3 that specified ReadableStreamCancel is: If stream.[[state]] is
+      // "closed", return a new promise resolved with undefined.
+      JS::ReadableStreamReaderCancel(aCx, reader, errorValue);
+    }
+  }
+
   mStreamClosed = true;
 
   mGlobal = nullptr;
 
   mPipeOut->CloseWithStatus(aStatus);
   mPipeOut = nullptr;
 
   mWorkerHolder = nullptr;
@@ -161,17 +186,17 @@ FetchStreamReader::StartConsuming(JSCont
   MOZ_DIAGNOSTIC_ASSERT(!mReader);
   MOZ_DIAGNOSTIC_ASSERT(aStream);
 
   JS::Rooted<JSObject*> reader(aCx,
                                JS::ReadableStreamGetReader(aCx, aStream,
                                                            JS::ReadableStreamReaderMode::Default));
   if (!reader) {
     aRv.StealExceptionFromJSContext(aCx);
-    CloseAndRelease(NS_ERROR_DOM_INVALID_STATE_ERR);
+    CloseAndRelease(aCx, NS_ERROR_DOM_INVALID_STATE_ERR);
     return;
   }
 
   mReader = reader;
   aReader.set(reader);
 
   aRv = mPipeOut->AsyncWait(this, 0, 0, mOwningEventTarget);
   if (NS_WARN_IF(aRv.Failed())) {
@@ -201,24 +226,24 @@ FetchStreamReader::OnOutputStreamReady(n
   AutoEntryScript aes(mGlobal, "ReadableStreamReader.read", !mWorkerHolder);
 
   JS::Rooted<JSObject*> reader(aes.cx(), mReader);
   JS::Rooted<JSObject*> promise(aes.cx(),
                                 JS::ReadableStreamDefaultReaderRead(aes.cx(),
                                                                     reader));
   if (NS_WARN_IF(!promise)) {
     // Let's close the stream.
-    CloseAndRelease(NS_ERROR_DOM_INVALID_STATE_ERR);
+    CloseAndRelease(aes.cx(), NS_ERROR_DOM_INVALID_STATE_ERR);
     return NS_ERROR_FAILURE;
   }
 
   RefPtr<Promise> domPromise = Promise::CreateFromExisting(mGlobal, promise);
   if (NS_WARN_IF(!domPromise)) {
     // Let's close the stream.
-    CloseAndRelease(NS_ERROR_DOM_INVALID_STATE_ERR);
+    CloseAndRelease(aes.cx(), NS_ERROR_DOM_INVALID_STATE_ERR);
     return NS_ERROR_FAILURE;
   }
 
   // Let's wait.
   domPromise->AppendNativeHandler(this);
   return NS_OK;
 }
 
@@ -235,31 +260,31 @@ FetchStreamReader::ResolvedCallback(JSCo
 
   // We don't want to play with JS api, let's WebIDL bindings doing it for us.
   // FetchReadableStreamReadDataDone is a dictionary with just a boolean, if the
   // parsing succeeded, we can proceed with the parsing of the "value", which it
   // must be a Uint8Array.
   FetchReadableStreamReadDataDone valueDone;
   if (!valueDone.Init(aCx, aValue)) {
     JS_ClearPendingException(aCx);
-    CloseAndRelease(NS_ERROR_DOM_INVALID_STATE_ERR);
+    CloseAndRelease(aCx, NS_ERROR_DOM_INVALID_STATE_ERR);
     return;
   }
 
   if (valueDone.mDone) {
     // Stream is completed.
-    CloseAndRelease(NS_BASE_STREAM_CLOSED);
+    CloseAndRelease(aCx, NS_BASE_STREAM_CLOSED);
     return;
   }
 
   UniquePtr<FetchReadableStreamReadDataArray> value(
     new FetchReadableStreamReadDataArray);
   if (!value->Init(aCx, aValue) || !value->mValue.WasPassed()) {
     JS_ClearPendingException(aCx);
-    CloseAndRelease(NS_ERROR_DOM_INVALID_STATE_ERR);
+    CloseAndRelease(aCx, NS_ERROR_DOM_INVALID_STATE_ERR);
     return;
   }
 
   Uint8Array& array = value->mValue.Value();
   array.ComputeLengthAndData();
   uint32_t len = array.Length();
 
   if (len == 0) {
@@ -269,17 +294,22 @@ FetchStreamReader::ResolvedCallback(JSCo
   }
 
   MOZ_DIAGNOSTIC_ASSERT(!mBuffer);
   mBuffer = Move(value);
 
   mBufferOffset = 0;
   mBufferRemaining = len;
 
-  WriteBuffer();
+  nsresult rv = WriteBuffer();
+  if (NS_FAILED(rv)) {
+    // DOMException only understands errors from domerr.msg, so we normalize to
+    // identifying an abort if the write fails.
+    CloseAndRelease(aCx, NS_ERROR_DOM_ABORT_ERR);
+  }
 }
 
 nsresult
 FetchStreamReader::WriteBuffer()
 {
   MOZ_ASSERT(mBuffer);
   MOZ_ASSERT(mBuffer->mValue.WasPassed());
 
@@ -291,45 +321,43 @@ FetchStreamReader::WriteBuffer()
     nsresult rv =
       mPipeOut->Write(data + mBufferOffset, mBufferRemaining, &written);
 
     if (rv == NS_BASE_STREAM_WOULD_BLOCK) {
       break;
     }
 
     if (NS_WARN_IF(NS_FAILED(rv))) {
-      CloseAndRelease(rv);
       return rv;
     }
 
     MOZ_ASSERT(written <= mBufferRemaining);
     mBufferRemaining -= written;
     mBufferOffset += written;
 
     if (mBufferRemaining == 0) {
       mBuffer = nullptr;
       break;
     }
   }
 
   nsresult rv = mPipeOut->AsyncWait(this, 0, 0, mOwningEventTarget);
   if (NS_WARN_IF(NS_FAILED(rv))) {
-    CloseAndRelease(rv);
     return rv;
   }
 
   return NS_OK;
 }
 
 void
 FetchStreamReader::RejectedCallback(JSContext* aCx,
                                     JS::Handle<JS::Value> aValue)
 {
   ReportErrorToConsole(aCx, aValue);
-  CloseAndRelease(NS_ERROR_FAILURE);
+  CloseAndRelease(aCx, NS_ERROR_FAILURE);
 }
 
 void
 FetchStreamReader::ReportErrorToConsole(JSContext* aCx,
                                         JS::Handle<JS::Value> aValue)
 {
   nsCString sourceSpec;
   uint32_t line = 0;
--- a/dom/fetch/FetchStreamReader.h
+++ b/dom/fetch/FetchStreamReader.h
@@ -35,18 +35,22 @@ public:
          nsIInputStream** aInputStream);
 
   void
   ResolvedCallback(JSContext* aCx, JS::Handle<JS::Value> aValue) override;
 
   void
   RejectedCallback(JSContext* aCx, JS::Handle<JS::Value> aValue) override;
 
+  // Idempotently close the output stream and null out all state. If aCx is
+  // provided, the reader will also be canceled.  aStatus must be a DOM error
+  // as understood by DOMException because it will be provided as the
+  // cancellation reason.
   void
-  CloseAndRelease(nsresult aStatus);
+  CloseAndRelease(JSContext* aCx, nsresult aStatus);
 
   void
   StartConsuming(JSContext* aCx,
                  JS::HandleObject aStream,
                  JS::MutableHandle<JSObject*> aReader,
                  ErrorResult& aRv);
 
 private: