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

RESOLVED FIXED in Firefox 65

Status

()

enhancement
P1
normal
RESOLVED FIXED
7 months ago
2 months ago

People

(Reporter: baku, Assigned: baku)

Tracking

unspecified
mozilla65
Points:
---

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Comment 1

7 months ago
Posted patch stream_global.patch (obsolete) — Splinter Review
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.
Flags: needinfo?(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?
Flags: needinfo?(bzbarsky)
Priority: -- → P1
Assignee

Comment 3

7 months ago
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?
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
Assignee

Comment 5

7 months ago
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.
Assignee

Comment 6

7 months ago
> 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.
Assignee

Comment 7

7 months ago
Posted patch stream_global.patch (obsolete) — Splinter Review
Attachment #9021547 - Attachment is obsolete: true
Assignee

Updated

7 months ago
Flags: needinfo?(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?
Flags: needinfo?(bzbarsky)
Comment on attachment 9021849 [details] [diff] [review]
stream_global.patch

r=me.  Thank you.
Attachment #9021849 - Flags: review+
Comment on attachment 9021849 [details] [diff] [review]
stream_global.patch

Wrong bug...
Attachment #9021849 - Flags: review+
Assignee

Comment 11

7 months ago
Attachment #9021849 - Attachment is obsolete: true
Flags: needinfo?(bzbarsky)
Comment on attachment 9021886 [details] [diff] [review]
stream_global.patch

r=me
Flags: needinfo?(bzbarsky)
Attachment #9021886 - Flags: review+

Comment 13

7 months ago
Pushed by amarchesini@mozilla.com:
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

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bb6607c98119
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.