1752880 - Read_ReadIntoRequest::CloseSteps resolves with no value field

Status: RESOLVED FIXED

(Core :: DOM: Streams, defect, P3)

Description

[Kagami Rosylight [:saschanaz] (they/them)]

It seems the bindings layer requires optional dictionary fields to have real values when constructed? Since Read_ReadRequest in ReadableStreamDefaultReader uses CreateValueDonePair to create a pure JS object, I wonder we should do the same here.

Comment 1

[Kagami Rosylight [:saschanaz] (they/them)]

Do you have an objection or any other idea here? 👀

Comment 2

[Tom S [:evilpie]]

Firstly I think this is probably a spec bug. It looks like an undefined value wasn't anticipated. So maybe we should look into changing the WebIDL type.

Comment 3

[Kagami Rosylight [:saschanaz] (they/them)]

Oops, the spec indeed uses dictionary here. I thought we were "abusing" the bindings layer, but we are not!

The current generated bindings code forces us to have a real value:

  if (mValue.WasPassed()) {
    do {
      // block for our 'break' successCode and scope for 'temp' and 'currentValue'
      JS::Rooted<JS::Value> temp(cx);
      ArrayBufferView const & currentValue = mValue.InternalValue();
      temp.setObject(*currentValue.Obj()); // <-- crashes if we have no real value here
      if (!MaybeWrapNonDOMObjectValue(cx, &temp)) {
        return false;
      }
      if (!JS_DefinePropertyById(cx, obj, atomsCache->value_id, temp, JSPROP_ENUMERATE)) {
        return false;
      }
      break;
    } while(false);
  }

But it should be okay to return with an undefined field here, although I'm not sure that's practical: https://webidl.spec.whatwg.org/#es-dictionary

Pinging bindings people to double check:

Comment 4

[Kagami Rosylight [:saschanaz] (they/them)]

The lack of value field is causing test failures in streams/readable-byte-streams/tee.any.js:

• ReadableStream teeing with byte source: canceling both branches in sequence with delay
• ReadableStream teeing with byte source: failing to cancel when canceling both branches in sequence with delay

Comment 5

[Peter Van der Beken [:peterv]]

Could you give some context? Like a link to the spec's WebIDL that's causing the issue, what value you're passing in and what you expected to happen?

AFAICT you want to pass undefined to an optional dictionary member with type ArrayBufferView? I don't think that makes sense in terms of WebIDL values, undefined is not of type ArrayBufferView.

Comment 6

[Tom S [:evilpie]]

The WebIDL comes from here: https://streams.spec.whatwg.org/#dictdef-readablestreambyobreadresult. ReadableStreamBYOBReadResult is returned by read. The dictionary is "constructed" by Step 6. of https://streams.spec.whatwg.org/#byob-reader-read. chunk can be undefined because https://streams.spec.whatwg.org/#readable-stream-cancel calls "close steps" with undefined.

Comment 7

[Kagami Rosylight [:saschanaz] (they/them)]

AFAICT you want to pass undefined to an optional dictionary member with type ArrayBufferView? I don't think that makes sense in terms of WebIDL values, undefined is not of type ArrayBufferView.

Re-reading the IDL spec after having some sleep...

I still think nothing prevents undefined be a value of an optional dictionary member in IDL->JS conversion (because, you know, undefined is a valid value for every optional one in JS->IDL conversion), but then I'm not sure the spec actually allows it. I guess I'll ask the spec authors directly.

Comment 8

[Kagami Rosylight [:saschanaz] (they/them)]

Posted: https://github.com/whatwg/webidl/issues/1094

Comment 9

[Peter Van der Beken [:peterv]]

Thanks for the context. I think the issue is that in terms of WebIDL types a WebIDL ArrayBufferView type can't contain a WebIDL undefined value, regardless of conversions. So it seems to me that the Streams spec has a bug here. If after conversion to ECMAScript an actual value property with value undefined is needed here, then the Streams spec could allow for undefined by making the value property a union of ArrayBufferView or undefined (and yes Kagami, I haven't forgotten about bug 1659158! :-))

I'll note that for ECMAScript->WebIDL conversions an ECMAScript undefined value for a property means that the entry is missing in the WebIDL dictionary, so the end result is the same as not having that property at all.

Comment 10

[Kagami Rosylight [:saschanaz] (they/them)]

Thanks for the great explanation! Adding the dependency.

Comment 12

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

Attachment: Bug 1752880 - Cleanup ErrorResult naming style r?saschanaz

Comment 13

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

Attachment: Bug 1752880 - Add missing error handling around JS_WrapObject r?saschanaz

Depends on D143413

Comment 14

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

Attachment: Bug 1752880 - Ensure ReadableStreamBYOBReadResult has 'value' property in light of binding challenges r?saschanaz

Depends on D143414

Comment 15

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

https://github.com/whatwg/streams/pull/1227

Comment 16

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

Attachment: Bug 1752880 - Unify ReadableStream reader ReadResult type r?saschanaz,smaug

This change fixes the behaviour of user-defined byte streams, allowing us to
ship them to users.

The changes here are not yet specified, but are proposed for inclusion into the
specification here: https://github.com/whatwg/streams/pull/1227

Depends on D143414

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/40f51fdbc6c4
Cleanup ErrorResult naming style  r=saschanaz
https://hg.mozilla.org/integration/autoland/rev/4f04a14a53c0
Add missing error handling around JS_WrapObject  r=saschanaz
https://hg.mozilla.org/integration/autoland/rev/1f1510c8527c
Unify ReadableStream reader ReadResult type r=saschanaz,smaug

Comment 18

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/40f51fdbc6c4
https://hg.mozilla.org/mozilla-central/rev/4f04a14a53c0
https://hg.mozilla.org/mozilla-central/rev/1f1510c8527c