Bug 1486698 - Update Fetch+Stream implementation to throw when the stream is disturbed or locked, r=bz
authorAndrea Marchesini <amarchesini@mozilla.com>
Wed, 31 Oct 2018 18:30:18 +0100
changeset 500246 544498045a9cfe55968fa6500bffbc3181869fce
parent 500245 e20a3ab2ca2ca3326233cee26c2bed5c3c647e0e
child 500247 23850b9ba24db1a49ca572dc4d290e5772e171d9
push id10290
push userffxbld-merge
push dateMon, 03 Dec 2018 16:23:23 +0000
treeherdermozilla-beta@700bed2445e6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbz
bugs1486698
milestone65.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 1486698 - Update Fetch+Stream implementation to throw when the stream is disturbed or locked, r=bz In this patch, I went through any place in DOM fetch code, where there are ReadableStreams and update the locked, disturbed, readable checks. Because we expose streams more often, we need an extra care in the use of ErrorResult objects. JS streams can now throw exceptions and we need to handle them. This patch also fixes a bug in FileStreamReader::CloseAndRelease() which could be called in case mReader creation fails.
dom/cache/Cache.cpp
dom/fetch/Fetch.cpp
dom/fetch/FetchStreamReader.cpp
dom/fetch/Response.cpp
dom/serviceworkers/ServiceWorkerEvents.cpp
dom/serviceworkers/ServiceWorkerScriptCache.cpp
dom/workers/ScriptLoader.cpp
testing/web-platform/meta/fetch/api/response/response-stream-disturbed-6.html.ini
--- a/dom/cache/Cache.cpp
+++ b/dom/cache/Cache.cpp
@@ -200,16 +200,17 @@ public:
     MOZ_DIAGNOSTIC_ASSERT(mRequestList.Length() == responseList.Length());
 
     // Now store the unwrapped Response list in the Cache.
     ErrorResult result;
     // TODO: Here we use the JSContext as received by the ResolvedCallback, and
     // its state could be the wrong one. The spec doesn't say anything
     // about it, yet (bug 1384006)
     RefPtr<Promise> put = mCache->PutAll(aCx, mRequestList, responseList, result);
+    result.WouldReportJSException();
     if (NS_WARN_IF(result.Failed())) {
       // TODO: abort the fetch requests we have running (bug 1157434)
       mPromise->MaybeReject(result);
       return;
     }
 
     // Chain the Cache::Put() promise to the original promise returned to
     // the content script.
--- a/dom/fetch/Fetch.cpp
+++ b/dom/fetch/Fetch.cpp
@@ -53,16 +53,18 @@
 namespace mozilla {
 namespace dom {
 
 namespace {
 
 void
 AbortStream(JSContext* aCx, JS::Handle<JSObject*> aStream, ErrorResult& aRv)
 {
+  aRv.MightThrowJSException();
+
   bool isReadable;
   if (!JS::ReadableStreamIsReadable(aCx, aStream, &isReadable)) {
     aRv.StealExceptionFromJSContext(aCx);
     return;
   }
   if (!isReadable) {
     return;
   }
@@ -1114,37 +1116,35 @@ FetchBody<Response>::~FetchBody();
 template <class Derived>
 bool
 FetchBody<Derived>::GetBodyUsed(ErrorResult& aRv) const
 {
   if (mBodyUsed) {
     return true;
   }
 
-  // If this stream is disturbed or locked, return true.
+  // If this stream is disturbed, return true.
   if (mReadableStreamBody) {
+    aRv.MightThrowJSException();
+
     AutoJSAPI jsapi;
     if (!jsapi.Init(mOwner)) {
       aRv.Throw(NS_ERROR_FAILURE);
       return true;
     }
 
     JSContext* cx = jsapi.cx();
     JS::Rooted<JSObject*> body(cx, mReadableStreamBody);
     bool disturbed;
-    bool locked;
-    bool readable;
-    if (!JS::ReadableStreamIsDisturbed(cx, body, &disturbed) ||
-        !JS::ReadableStreamIsLocked(cx, body, &locked) ||
-        !JS::ReadableStreamIsReadable(cx, body, &readable)) {
+    if (!JS::ReadableStreamIsDisturbed(cx, body, &disturbed)) {
       aRv.StealExceptionFromJSContext(cx);
       return false;
     }
 
-    return disturbed || locked || !readable;
+    return disturbed;
   }
 
   return false;
 }
 
 template
 bool
 FetchBody<Request>::GetBodyUsed(ErrorResult&) const;
@@ -1177,16 +1177,18 @@ FetchBody<Derived>::SetBodyUsed(JSContex
     return;
   }
 
   mBodyUsed = true;
 
   // If we already have a ReadableStreamBody and it has been created by DOM, we
   // have to lock it now because it can have been shared with other objects.
   if (mReadableStreamBody) {
+    aRv.MightThrowJSException();
+
     JS::Rooted<JSObject*> readableStreamObj(aCx, mReadableStreamBody);
 
     JS::ReadableStreamMode mode;
     if (!JS::ReadableStreamGetMode(aCx, readableStreamObj, &mode)) {
       aRv.StealExceptionFromJSContext(aCx);
       return;
     }
 
@@ -1218,16 +1220,18 @@ template
 void
 FetchBody<Response>::SetBodyUsed(JSContext* aCx, ErrorResult& aRv);
 
 template <class Derived>
 already_AddRefed<Promise>
 FetchBody<Derived>::ConsumeBody(JSContext* aCx, FetchConsumeType aType,
                                 ErrorResult& aRv)
 {
+  aRv.MightThrowJSException();
+
   RefPtr<AbortSignalImpl> signalImpl = DerivedClass()->GetSignalImpl();
   if (signalImpl && signalImpl->Aborted()) {
     aRv.Throw(NS_ERROR_DOM_ABORT_ERR);
     return nullptr;
   }
 
   bool bodyUsed = GetBodyUsed(aRv);
   if (NS_WARN_IF(aRv.Failed())) {
@@ -1426,16 +1430,18 @@ FetchBody<Response>::GetBody(JSContext* 
                              ErrorResult& aRv);
 
 template <class Derived>
 void
 FetchBody<Derived>::LockStream(JSContext* aCx,
                                JS::HandleObject aStream,
                                ErrorResult& aRv)
 {
+  aRv.MightThrowJSException();
+
 #if DEBUG
   JS::ReadableStreamMode streamMode;
   if (!JS::ReadableStreamGetMode(aCx, aStream, &streamMode)) {
     aRv.StealExceptionFromJSContext(aCx);
     return;
   }
   MOZ_ASSERT(streamMode == JS::ReadableStreamMode::ExternalSource);
 #endif // DEBUG
@@ -1479,16 +1485,18 @@ FetchBody<Derived>::MaybeTeeReadableStre
   aBodyOut.set(nullptr);
   *aStreamReader = nullptr;
   *aInputStream = nullptr;
 
   if (!mReadableStreamBody) {
     return;
   }
 
+  aRv.MightThrowJSException();
+
   JS::Rooted<JSObject*> stream(aCx, mReadableStreamBody);
 
   // If this is a ReadableStream with an external source, this has been
   // generated by a Fetch. In this case, Fetch will be able to recreate it
   // again when GetBody() is called.
   JS::ReadableStreamMode streamMode;
   if (!JS::ReadableStreamGetMode(aCx, stream, &streamMode)) {
     aRv.StealExceptionFromJSContext(aCx);
--- a/dom/fetch/FetchStreamReader.cpp
+++ b/dom/fetch/FetchStreamReader.cpp
@@ -114,19 +114,17 @@ FetchStreamReader::CloseAndRelease(JSCon
 
   if (mStreamClosed) {
     // Already closed.
     return;
   }
 
   RefPtr<FetchStreamReader> kungFuDeathGrip = this;
 
-  if (aCx) {
-    MOZ_ASSERT(mReader);
-
+  if (aCx && 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
@@ -152,16 +150,18 @@ void
 FetchStreamReader::StartConsuming(JSContext* aCx,
                                   JS::HandleObject aStream,
                                   JS::MutableHandle<JSObject*> aReader,
                                   ErrorResult& aRv)
 {
   MOZ_DIAGNOSTIC_ASSERT(!mReader);
   MOZ_DIAGNOSTIC_ASSERT(aStream);
 
+  aRv.MightThrowJSException();
+
   JS::Rooted<JSObject*> reader(aCx,
                                JS::ReadableStreamGetReader(aCx, aStream,
                                                            JS::ReadableStreamReaderMode::Default));
   if (!reader) {
     aRv.StealExceptionFromJSContext(aCx);
     CloseAndRelease(aCx, NS_ERROR_DOM_INVALID_STATE_ERR);
     return;
   }
--- a/dom/fetch/Response.cpp
+++ b/dom/fetch/Response.cpp
@@ -234,31 +234,31 @@ Response::Constructor(const GlobalObject
     }
 
     nsCString contentTypeWithCharset;
     nsCOMPtr<nsIInputStream> bodyStream;
     int64_t bodySize = InternalResponse::UNKNOWN_BODY_SIZE;
 
     const fetch::ResponseBodyInit& body = aBody.Value().Value();
     if (body.IsReadableStream()) {
+      aRv.MightThrowJSException();
+
       JSContext* cx = aGlobal.Context();
       const ReadableStream& readableStream = body.GetAsReadableStream();
 
       JS::Rooted<JSObject*> readableStreamObj(cx, readableStream.Obj());
 
       bool disturbed;
       bool locked;
-      bool readable;
       if (!JS::ReadableStreamIsDisturbed(cx, readableStreamObj, &disturbed) ||
-          !JS::ReadableStreamIsLocked(cx, readableStreamObj, &locked) ||
-          !JS::ReadableStreamIsReadable(cx, readableStreamObj, &readable)) {
+          !JS::ReadableStreamIsLocked(cx, readableStreamObj, &locked)) {
         aRv.StealExceptionFromJSContext(cx);
         return nullptr;
       }
-      if (disturbed || locked || !readable) {
+      if (disturbed || locked) {
         aRv.ThrowTypeError<MSG_FETCH_BODY_CONSUMED_ERROR>();
         return nullptr;
       }
 
       r->SetReadableStreamBody(cx, readableStreamObj);
 
       JS::ReadableStreamMode streamMode;
       if (!JS::ReadableStreamGetMode(cx, readableStreamObj, &streamMode)) {
@@ -336,16 +336,39 @@ Response::Constructor(const GlobalObject
 
 already_AddRefed<Response>
 Response::Clone(JSContext* aCx, ErrorResult& aRv)
 {
   bool bodyUsed = GetBodyUsed(aRv);
   if (NS_WARN_IF(aRv.Failed())) {
     return nullptr;
   }
+
+  if (!bodyUsed && mReadableStreamBody) {
+    aRv.MightThrowJSException();
+
+    AutoJSAPI jsapi;
+    if (!jsapi.Init(mOwner)) {
+      aRv.Throw(NS_ERROR_FAILURE);
+      return nullptr;
+    }
+
+    JSContext* cx = jsapi.cx();
+    JS::Rooted<JSObject*> body(cx, mReadableStreamBody);
+    bool locked;
+    // We just need to check the 'locked' state because GetBodyUsed() already
+    // checked the 'disturbed' state.
+    if (!JS::ReadableStreamIsLocked(cx, body, &locked)) {
+      aRv.StealExceptionFromJSContext(cx);
+      return nullptr;
+    }
+
+    bodyUsed = locked;
+  }
+
   if (bodyUsed) {
     aRv.ThrowTypeError<MSG_FETCH_BODY_CONSUMED_ERROR>();
     return nullptr;
   }
 
   RefPtr<FetchStreamReader> streamReader;
   nsCOMPtr<nsIInputStream> inputStream;
 
--- a/dom/serviceworkers/ServiceWorkerEvents.cpp
+++ b/dom/serviceworkers/ServiceWorkerEvents.cpp
@@ -673,16 +673,17 @@ RespondWithHandler::ResolvedCallback(JSC
     autoCancel.SetCancelMessage(
       NS_LITERAL_CSTRING("BadRedirectModeInterceptionWithURL"), mRequestURL);
     return;
   }
 
   {
     ErrorResult error;
     bool bodyUsed = response->GetBodyUsed(error);
+    error.WouldReportJSException();
     if (NS_WARN_IF(error.Failed())) {
       autoCancel.SetCancelErrorResult(aCx, error);
       return;
     }
     if (NS_WARN_IF(bodyUsed)) {
       autoCancel.SetCancelMessage(
         NS_LITERAL_CSTRING("InterceptedUsedResponseWithURL"), mRequestURL);
       return;
@@ -754,16 +755,17 @@ RespondWithHandler::ResolvedCallback(JSC
                                                           std::move(closure));
 
   nsCOMPtr<nsIInputStream> body;
   ir->GetUnfilteredBody(getter_AddRefs(body));
   // Errors and redirects may not have a body.
   if (body) {
     ErrorResult error;
     response->SetBodyUsed(aCx, error);
+    error.WouldReportJSException();
     if (NS_WARN_IF(error.Failed())) {
       autoCancel.SetCancelErrorResult(aCx, error);
       return;
     }
   }
 
   MOZ_ALWAYS_SUCCEEDS(worker->DispatchToMainThread(startRunnable.forget()));
 
--- a/dom/serviceworkers/ServiceWorkerScriptCache.cpp
+++ b/dom/serviceworkers/ServiceWorkerScriptCache.cpp
@@ -613,23 +613,21 @@ private:
     MOZ_ASSERT(aCN);
     MOZ_DIAGNOSTIC_ASSERT(mState == WaitingForOpen);
 
     // We don't have to save any information from a failed CompareNetwork.
     if (!aCN->Succeeded()) {
       return NS_OK;
     }
 
-    ErrorResult result;
     nsCOMPtr<nsIInputStream> body;
-    result = NS_NewCStringInputStream(getter_AddRefs(body),
-                                      NS_ConvertUTF16toUTF8(aCN->Buffer()));
-    if (NS_WARN_IF(result.Failed())) {
-      MOZ_ASSERT(!result.IsErrorWithMessage());
-      return result.StealNSResult();
+    nsresult rv = NS_NewCStringInputStream(getter_AddRefs(body),
+                                           NS_ConvertUTF16toUTF8(aCN->Buffer()));
+    if (NS_WARN_IF(NS_FAILED(rv))) {
+      return rv;
     }
 
     RefPtr<InternalResponse> ir =
       new InternalResponse(200, NS_LITERAL_CSTRING("OK"));
     ir->SetBody(body, aCN->Buffer().Length());
     ir->SetURLList(aCN->URLList());
 
     ir->InitChannelInfo(aCN->GetChannelInfo());
@@ -645,17 +643,19 @@ private:
       new Response(aCache->GetGlobalObject(), ir, nullptr);
 
     RequestOrUSVString request;
     request.SetAsUSVString().Rebind(aCN->URL().Data(), aCN->URL().Length());
 
     // For now we have to wait until the Put Promise is fulfilled before we can
     // continue since Cache does not yet support starting a read that is being
     // written to.
+    ErrorResult result;
     RefPtr<Promise> cachePromise = aCache->Put(aCx, request, *response, result);
+    result.WouldReportJSException();
     if (NS_WARN_IF(result.Failed())) {
       // No exception here because there are no ReadableStreams involved here.
       MOZ_ASSERT(!result.IsJSException());
       MOZ_ASSERT(!result.IsErrorWithMessage());
       return result.StealNSResult();
     }
 
     mPendingCount += 1;
--- a/dom/workers/ScriptLoader.cpp
+++ b/dom/workers/ScriptLoader.cpp
@@ -837,16 +837,17 @@ private:
     // This JSContext will not end up executing JS code because here there are
     // no ReadableStreams involved.
     AutoJSAPI jsapi;
     jsapi.Init();
 
     ErrorResult error;
     RefPtr<Promise> cachePromise =
       mCacheCreator->Cache_()->Put(jsapi.cx(), request, *response, error);
+    error.WouldReportJSException();
     if (NS_WARN_IF(error.Failed())) {
       nsresult rv = error.StealNSResult();
       channel->Cancel(rv);
       return rv;
     }
 
     RefPtr<CachePromiseHandler> promiseHandler =
       new CachePromiseHandler(this, loadInfo, aIndex);
deleted file mode 100644
--- a/testing/web-platform/meta/fetch/api/response/response-stream-disturbed-6.html.ini
+++ /dev/null
@@ -1,16 +0,0 @@
-[response-stream-disturbed-6.html]
-  [A non-closed stream on which read() has been called]
-    expected: FAIL
-
-  [A non-closed stream on which cancel() has been called]
-    expected: FAIL
-
-  [A closed stream on which read() has been called]
-    expected: FAIL
-
-  [An errored stream on which read() has been called]
-    expected: FAIL
-
-  [An errored stream on which cancel() has been called]
-    expected: FAIL
-