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

@@ -205,6 +205,7 @@ public:
     // 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);

dom/fetch/Fetch.cpp

@@ -58,6 +58,8 @@ namespace {
 void
 AbortStream(JSContext* aCx, JS::Handle<JSObject*> aStream, ErrorResult& aRv)
 {
+  aRv.MightThrowJSException();
+
   bool isReadable;
   if (!JS::ReadableStreamIsReadable(aCx, aStream, &isReadable)) {
     aRv.StealExceptionFromJSContext(aCx);
@@ -1119,8 +1121,10 @@ FetchBody<Derived>::GetBodyUsed(ErrorResult& aRv) const
     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);
@@ -1130,16 +1134,12 @@ FetchBody<Derived>::GetBodyUsed(ErrorResult& aRv) const
     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;
@@ -1182,6 +1182,8 @@ FetchBody<Derived>::SetBodyUsed(JSContext* aCx, ErrorResult& aRv)
   // 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;
@@ -1223,6 +1225,8 @@ 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);
@@ -1431,6 +1435,8 @@ FetchBody<Derived>::LockStream(JSContext* aCx,
                                JS::HandleObject aStream,
                                ErrorResult& aRv)
 {
+  aRv.MightThrowJSException();
+
 #if DEBUG
   JS::ReadableStreamMode streamMode;
   if (!JS::ReadableStreamGetMode(aCx, aStream, &streamMode)) {
@@ -1484,6 +1490,8 @@ FetchBody<Derived>::MaybeTeeReadableStreamBody(JSContext* aCx,
     return;
   }
 
+  aRv.MightThrowJSException();
+
   JS::Rooted<JSObject*> stream(aCx, mReadableStreamBody);
 
   // If this is a ReadableStream with an external source, this has been

dom/fetch/FetchStreamReader.cpp

@@ -119,9 +119,7 @@ FetchStreamReader::CloseAndRelease(JSContext* aCx, nsresult aStatus)
 
   RefPtr<FetchStreamReader> kungFuDeathGrip = this;
 
-  if (aCx) {
-    MOZ_ASSERT(mReader);
-
+  if (aCx && mReader) {
     RefPtr<DOMException> error = DOMException::Create(aStatus);
 
     JS::Rooted<JS::Value> errorValue(aCx);
@@ -157,6 +155,8 @@ FetchStreamReader::StartConsuming(JSContext* aCx,
   MOZ_DIAGNOSTIC_ASSERT(!mReader);
   MOZ_DIAGNOSTIC_ASSERT(aStream);
 
+  aRv.MightThrowJSException();
+
   JS::Rooted<JSObject*> reader(aCx,
                                JS::ReadableStreamGetReader(aCx, aStream,
                                                            JS::ReadableStreamReaderMode::Default));

dom/fetch/Response.cpp

@@ -239,6 +239,8 @@ Response::Constructor(const GlobalObject& aGlobal,
 
     const fetch::ResponseBodyInit& body = aBody.Value().Value();
     if (body.IsReadableStream()) {
+      aRv.MightThrowJSException();
+
       JSContext* cx = aGlobal.Context();
       const ReadableStream& readableStream = body.GetAsReadableStream();
 
@@ -246,14 +248,12 @@ Response::Constructor(const GlobalObject& aGlobal,
 
       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;
       }
@@ -341,6 +341,29 @@ Response::Clone(JSContext* aCx, ErrorResult& 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;

dom/serviceworkers/ServiceWorkerEvents.cpp

@@ -678,6 +678,7 @@ RespondWithHandler::ResolvedCallback(JSContext* aCx, JS::Handle<JS::Value> aValu
   {
     ErrorResult error;
     bool bodyUsed = response->GetBodyUsed(error);
+    error.WouldReportJSException();
     if (NS_WARN_IF(error.Failed())) {
       autoCancel.SetCancelErrorResult(aCx, error);
       return;
@@ -759,6 +760,7 @@ RespondWithHandler::ResolvedCallback(JSContext* aCx, JS::Handle<JS::Value> aValu
   if (body) {
     ErrorResult error;
     response->SetBodyUsed(aCx, error);
+    error.WouldReportJSException();
     if (NS_WARN_IF(error.Failed())) {
       autoCancel.SetCancelErrorResult(aCx, error);
       return;

dom/serviceworkers/ServiceWorkerScriptCache.cpp

@@ -618,13 +618,11 @@ private:
       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 =
@@ -650,7 +648,9 @@ private:
     // 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());

dom/workers/ScriptLoader.cpp

@@ -842,6 +842,7 @@ private:
     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);

testing/web-platform/meta/fetch/api/response/response-stream-disturbed-6.html.ini

@@ -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
-