1736460 - Read_ReadRequest::ChunkSteps resolves with the wrong type.

Status: RESOLVED INVALID

(Core :: DOM: Streams, defect)

Description

[Matthew Gaudet (he/him) [:mgaudet]]

ReadableStreamDefaultReader's read WebIDL is specified as returning a Promise<ReadableStreamDefaultReadResult>.

Currently we do not do this, instead creating regular JS Objects. This turns out to be observable in response-stream-with-broken-then.any.js, where Object.prototype.then causes errors.

This may be fixable simply by removing the prototype from the object created in CreateValueDonePair, but we should have a clearer understanding of exactly what's involved in returning Promise<ReadableStreamDefaultReadResult>

Comment 1

[Matthew Gaudet (he/him) [:mgaudet]]

Side note: Some archaeology:

• This PR added ReadableStreamDefaultReadResult.
• This was the fix for the JS streams implementation, which used a forAuthorCode bool removed by the above PR.

Comment 2

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1736460 - Remove prototype from ReadRequest chunk steps r?smaug

This is a basic fix for response-stream-with-broken-then. What I'm not 100%
clear on is whether or not the fact that the IDL says we return a
Promise<ReadableStreamDefaultReadResult> means that we ought to do something
different / more binding typed?

Comment 3

[Matthew Gaudet (he/him) [:mgaudet]]

So, the above patch has been abandoned. It would result in a user-visible, probably web-incompatible change where ReadableStreamDefaultReader.prototype.read would return an object with a null prototype.

The root cause of the problem is that the Streams specification is written as if we could easily resolve a promise with a dictionary value, avoiding all the messiness of resolving promises with objects, and having conversion from dictionary to object only occur at the WebIDL interface boundary. In reality, if we attempt to resolve a promise with a WebIDL dictionary value, we first convert that dictionary to an object, then resolve the promise.

In this case, rendering us vulnerable to the changes wrought by the special handling of the then property in the Promises specification (Steps 9-12) (thanks to Arai for pointing out the behaviour root).

I'm honestly not sure how to proceed here. From discussion about this issue on the Streams specification, it sounds like it's one of the few/only specs that uses Promises internally; but it seems like if more specs were written with Promises this could be a more pervasive issue: Any attempt to resolve a promise with an IDL type is vulnerable to this issue. It makes me feel as if the resolve path for IDL types needs specialization here, but I'm not sure where the lever ought to be applied.

Comment 4

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

(In reply to Matthew Gaudet (he/him) [:mgaudet] from comment #3)

The root cause of the problem is that the Streams specification is written as if we could easily resolve a promise with a dictionary value, avoiding all the messiness of resolving promises with objects, and having conversion from dictionary to object only occur at the WebIDL interface boundary. In reality, if we attempt to resolve a promise with a WebIDL dictionary value, we first convert that dictionary to an object, then resolve the promise.

This sounds like a spec issue. Or that the test is broken.

Comment 5

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

What part of the test fails? Or would it be possible to have a minimal testcase for this?

Comment 6

[Matthew Gaudet (he/him) [:mgaudet]]

I ran into this on my local stack which is a bit ahead of whats in central, and so the linked test case needs Response. I'll try to to rescue the test case from Bug 1503324.

Regarding response-stream-with-broken-then.any.js, all sub-tests fail by allowing substitution.

Comment 7

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: WIP: Bug 1736460 - Standalone Test Case

Comment 8

[Matthew Gaudet (he/him) [:mgaudet]]

The attached test case will fail on central, if the build is configured with --enable-dom-streams.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:mgaudet, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 10

[Matthew Gaudet (he/him) [:mgaudet]]

So, this bug really only exists as part of the Fetch integration, so I have a fix that exists as part of the Fetch integration.