1503602 - Use the Response's global to create the ReadableStream reader to consume the body

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

This bug is about fixing this comment:

https://searchfox.org/mozilla-central/rev/fc3d974254660b34638b2af9d5431618b191b233/dom/fetch/FetchStreamReader.cpp#195-196

and enter in the correct global when creating the Stream reader:

https://searchfox.org/mozilla-central/rev/fc3d974254660b34638b2af9d5431618b191b233/dom/fetch/FetchStreamReader.cpp#152

Comment 1

[Andrea Marchesini [:baku]]

Attachment: stream_global.patch

Bz, do you mind to take a look at this patch?
Asking you because you did the first review related to the global use in fetch API + Streams.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9021547 [details] [diff] [review]
stream_global.patch

>+++ b/dom/fetch/FetchStreamReader.cpp
>+  AutoEntryScript aes(mGlobal, "ReadableStreamReader.read", !mWorkerRef);

Why do we want a separate AutoEntryScript here, as opposed to just entering the right global on aCx?  Please document, at the very least.  If it's observable (via observing entry globals, exception-reporting  behavior, etc), please add tests too.

What does the spec say to do here?

>     CloseAndRelease(aCx, NS_ERROR_DOM_INVALID_STATE_ERR);

This CloseAndRelease function can leave exceptions on the JSContext if ToJSValue fails, and at least some of the callers (e.g. FetchStreamReader::ResolvedCallback) don't propagate that out in any way I can tell.  Please file a followup to get this fixed?

>@@ -187,18 +190,18 @@ FetchStreamReader::OnOutputStreamReady(n
>+  // We want to use the read data in the reader's global, which is also the
>+  // Response's global.

Is this defined by a spec.  Are there existing tests for this?

Comment 3

[Andrea Marchesini [:baku]]

Correct me if I'm wrong. We have 3 globals involved here:

Global A. the global of the ReadableStream.

Global B. the global where the Response object belongs to. If Response is constructed with a JS ReadableStream, we create a FetchStreamReader, which has mGlobal equal to the Response's global.

https://searchfox.org/mozilla-central/rev/fc3d974254660b34638b2af9d5431618b191b233/dom/fetch/FetchStreamReader.cpp#42,51,89-90

FetchStreamReader is created in 2 circumstances: new Response(readableStream) and response.clone().
In the first case FetchStreamReader uses the global of its Response's object:
https://searchfox.org/mozilla-central/rev/fc3d974254660b34638b2af9d5431618b191b233/dom/fetch/Response.cpp#161,294

In the second case FetchStreamReader uses the global of the Response's object, which uses the original Response's one:
https://searchfox.org/mozilla-central/rev/fc3d974254660b34638b2af9d5431618b191b233/dom/fetch/Response.cpp#161,294,336,351,366
https://searchfox.org/mozilla-central/rev/fc3d974254660b34638b2af9d5431618b191b233/dom/fetch/Fetch.cpp#1469,1513

The fetch spec doesn't say which global we should use when creating the cloned Response object.

Global C. Where Response.text() is executed (or any method that consumes the body). In this case, if we have a FetchStreamReader, we create a Reader:

https://searchfox.org/mozilla-central/rev/fc3d974254660b34638b2af9d5431618b191b233/dom/fetch/FetchStreamReader.cpp#160-162

Which global should we use? The spec doesn't say it:

https://fetch.spec.whatwg.org/#concept-body-consume-body
3. "Let reader be the result of getting a reader from stream. If that threw an exception, return a new promise rejected with that exception."

https://fetch.spec.whatwg.org/#concept-body-consume-body
1. "Let reader be the result of calling AcquireReadableStreamDefaultReader(stream)."

https://streams.spec.whatwg.org/#acquire-readable-stream-reader
1. "Return ? Construct(ReadableStreamDefaultReader, « stream »)."

The using of one global or another should not be observable (discussing this with jorendorff).
In this patch I force the using of the global B for 1 reason. When we read data from the stream we enter in global B here:

https://searchfox.org/mozilla-central/rev/fc3d974254660b34638b2af9d5431618b191b233/dom/fetch/FetchStreamReader.cpp#197-217

Here we receive a promise from the reader and we want that promise to be in global B. This is not specified in the spec:

https://fetch.spec.whatwg.org/#ref-for-concept-get-reader%E2%91%A0
4. "Let promise be the result of reading all bytes from stream with reader."

https://fetch.spec.whatwg.org/#concept-read-all-bytes-from-readablestream

So, just to avoid extra cross-compartment steps, I think it's better to create the reader is the same global where we will read data from.

Does it seem reasonable to you?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> The fetch spec doesn't say which global we should use when creating the cloned Response object.

Please file a spec bug?

>1. "Return ? Construct(ReadableStreamDefaultReader, « stream »)."

Per spec as currently written, that should use the current global at the time of the call.  That is, it should use the global of the "text" function involved if coming in via Response.prototype.text().  Whether this matches implementations, who knows.

> The using of one global or another should not be observable

Hmm.   Is that because the reader object is not exposed to script and we never access things off its prototype or anything else that would expose which global it's in?

> and we want that promise to be in global B

Ok.  That just needs to be clearly documented.

> This is not specified in the spec:

Please file a spec bug.

> Does it seem reasonable to you?

Yes.   Needs spec bugs and documentation.

Comment 5

[Andrea Marchesini [:baku]]

https://github.com/whatwg/fetch/issues/825
https://github.com/whatwg/fetch/issues/826

I'll continue to work on this bug when I have some answers.

Comment 6

[Andrea Marchesini [:baku]]

> https://github.com/whatwg/fetch/issues/825

Here we want to keep the firefox behavior: the cloned Response will be in the original object's global.

> https://github.com/whatwg/fetch/issues/826

The behavior is not observable. It's up to the implementation to use the better global.

I'll update the comments and I'll submit a new patch.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: stream_global.patch

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9021849 [details] [diff] [review]
stream_global.patch

This doesn't answer my question about why we need a separate AutoEntryScript.  Why do we need that?  And if we don't, why are we doing it?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9021849 [details] [diff] [review]
stream_global.patch

r=me.  Thank you.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9021849 [details] [diff] [review]
stream_global.patch

Wrong bug...

Comment 11

[Andrea Marchesini [:baku]]

Attachment: stream_global.patch

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 9021886 [details] [diff] [review]
stream_global.patch

r=me

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb6607c98119
Use the Response's global to create the ReadableStream reader to consume the body, r=bz

Comment 14

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/bb6607c98119