Bug 215557 – Optimise resolution of promises with values in ReadableStream implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

Optimise resolution of promises with values in ReadableStream implementation

[youenn fablet]

<rdar://problem/66828616>

[youenn fablet]

Created attachment 406701 [details]
Patch

[youenn fablet]

Created attachment 406706 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 406706 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406706&action=review

> Source/WebCore/Modules/streams/ReadableByteStreamInternals.js:577
> +    @fulfillPromise(readIntoRequest.@promise, { value: chunk, done: done });

@fulfillPromise is raw implementation detail, and it is not designed to be called directly from the non-JSC code.
If we want to use it, we need to be extra careful.
For example, even if you call it, you can still call `requests[index].@resolve` and it causes broken promise behavior.
And you cannot call @fulfillPromise multiple times on the same promise (which should cause assertion crash).
Can you use @newPromise instead of @newPromiseCapability(@Promise) and consistently use @fulfillPromise, @resolvePromise, and @rejectPromise?

> Source/WebCore/Modules/streams/ReadableStreamInternals.js:449
> +            @fulfillPromise(requests[index].@promise, { value: @undefined, done: true });

Ditto.

> Source/WebCore/Modules/streams/ReadableStreamInternals.js:460
> +    const readRequest = @getByIdDirectPrivate(@getByIdDirectPrivate(stream, "reader"), "readRequests").@shift();
> +    @fulfillPromise(readRequest.@promise, { value: chunk, done: done });

Ditto.

> Source/WebCore/Modules/streams/StreamInternals.js:196
> +function createFulfilledPromise(value)
> +{
> +    const promise = @newPromise();
> +    @fulfillPromise(promise, value);
> +    return promise;
> +}

I think this is OK since we do not create a @resolve / @reject handlers for this promise since we are creating promise via @newPromise.

[youenn fablet]

Created attachment 406773 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 406773 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=406773&action=review

r=me

> Source/WebCore/Modules/streams/ReadableStreamInternals.js:305
>          const requests = @getByIdDirectPrivate(reader, "readRequests");
>          for (let index = 0, length = requests.length; index < length; ++index)
> -            requests[index].@reject.@call(@undefined, error);
> +            @rejectPromise(requests[index], error);
>          @putByIdDirectPrivate(reader, "readRequests", []);

I recommend doing,

const requests = @getByIdDirectPrivate(reader, "readRequests");
@putByIdDirectPrivate(reader, "readRequests", []);
for (let index = 0, length = requests.length; index < length; ++index)
    @rejectPromise(requests[index], error);

To ensure there is no reference to these promises from ReadableStream while calling @rejectPromise.

> Source/WebCore/Modules/streams/ReadableStreamInternals.js:311
>          const requests = @getByIdDirectPrivate(reader, "readIntoRequests");
>          for (let index = 0, length = requests.length; index < length; ++index)
> -            requests[index].@reject.@call(@undefined, error);
> +            @rejectPromise(requests[index], error);
>          @putByIdDirectPrivate(reader, "readIntoRequests", []);

Ditto.

> Source/WebCore/Modules/streams/ReadableStreamInternals.js:450
>          for (let index = 0, length = requests.length; index < length; ++index)
> -            requests[index].@resolve.@call(@undefined, {value:@undefined, done: true});
> +            @fulfillPromise(requests[index], { value: @undefined, done: true });
>          @putByIdDirectPrivate(reader, "readRequests", []);

Ditto.

[youenn fablet]

Created attachment 406831 [details]
Patch for landing

[EWS]

Committed r265854: <https://trac.webkit.org/changeset/265854>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406831 [details].