1486698 - Throw for disturbed/locked streams more often

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Anne (:annevk)]

In https://github.com/whatwg/fetch/pull/801, tests synced by bug 1485601, the decision was made to throw for more ReadableStream states. It'd be good to align with that before streams are shipped.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: stream_busy.patch

Comment 2

[Andrea Marchesini [:baku]]

Attachment: stream_busy.patch

Comment 3

[Andrea Marchesini [:baku]]

Bz, can you review this patch? Thanks

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9021430 [details] [diff] [review]
stream_busy.patch

>+++ b/dom/cache/Cache.cpp
>-    ErrorResult result;
>+    IgnoredErrorResult result;

If this is just working around the JS exception asserts, then the  right thing is not this change but to call result.WouldReportJSException() right before the check that ensures that failures are handled.

>+++ b/dom/fetch/Fetch.cpp
> FetchBody<Derived>::GetBodyUsed(ErrorResult& aRv) const
>-  // If this stream is disturbed or locked, return true.
>+  // If this stream is disturbed, return true.

I don't see this change in the referenced PR.  Was this code wrong even before that?  I assume there are now tests that catch this (by passing in locked or non-readable streams and expecting this to not throw)?

>+++ b/dom/fetch/FetchStreamReader.cpp
>-    MOZ_ASSERT(mReader);
>+  if (aCx && mReader) {

Why this change?  Commit message should explain.

>+++ b/dom/fetch/Response.cpp
>-      if (disturbed || locked || !readable) {
>+      if (disturbed || locked) {

So this also used to not match the spec?  Again, there are tests covering this?

> Response::Clone(JSContext* aCx, ErrorResult& aRv)
>+++ b/dom/serviceworkers/ServiceWorkerEvents.cpp
>-    ErrorResult error;
>+    IgnoredErrorResult error;

Again, WouldReportJSException() before the Failed() check would be better.

>@@ -752,17 +752,17 @@ RespondWithHandler::ResolvedCallback(JSC
>-    ErrorResult error;
>+    IgnoredErrorResult error;

And here.

>+++ b/dom/serviceworkers/ServiceWorkerScriptCache.cpp
>@@ -645,26 +643,28 @@ private:
>+    IgnoredErrorResult result;
>     RefPtr<Promise> cachePromise = aCache->Put(aCx, request, *response, result);

And here.

>+    result.WouldReportJSException();

Doing this on the path where we're not reporting the exception is ... odd.  ;)   Again, do it right before the Failed() check.

>+++ b/dom/workers/ScriptLoader.cpp
>-    ErrorResult error;
>+    IgnoredErrorResult error;

Same thing as above.

r=me

Comment 5

[Andrea Marchesini [:baku]]

> I don't see this change in the referenced PR.  Was this code wrong even
> before that?  I assume there are now tests that catch this (by passing in
> locked or non-readable streams and expecting this to not throw)?

Yes. It was already broken. There are tests, and we were failing them all. The patch contains the removal of a .ini file for WPTs.

> >+++ b/dom/fetch/FetchStreamReader.cpp
> >-    MOZ_ASSERT(mReader);
> >+  if (aCx && mReader) {
> 
> Why this change?  Commit message should explain.

Because of this: https://searchfox.org/mozilla-central/rev/fc3d974254660b34638b2af9d5431618b191b233/dom/fetch/FetchStreamReader.cpp#160-167

if the reader creation fails, we call CloseAndRelease() without having mReader. I'll update the commit message.

> >+++ b/dom/fetch/Response.cpp
> >-      if (disturbed || locked || !readable) {
> >+      if (disturbed || locked) {
> 
> So this also used to not match the spec?  Again, there are tests covering
> this?

Yes. In response-stream-disturbed-6.html.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/544498045a9c
Update Fetch+Stream implementation to throw when the stream is disturbed or locked, r=bz

Comment 7

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/544498045a9c