Bug 227673 – Use AbortSignal's [PrivateIdentifier] whenSignalAborted() static method

• Component: DOM
• Status: RESOLVED CONFIGURATION CHANGED

[Alexey Shvayka]

[WebIDL] Use AbortSignal's [PrivateIdentifier] whenSignalAborted() static method

[Alexey Shvayka]

Created attachment 432862 [details]
Patch

[Alexey Shvayka]

Created attachment 432864 [details]
Patch

Revert bindings/js/WebCoreBuiltinNames.h change.

[Alexey Shvayka]

Created attachment 432876 [details]
Patch

Adjust WPT expectations.

[youenn fablet]

Comment on attachment 432876 [details]
Patch

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

> LayoutTests/imported/w3c/web-platform-tests/streams/piping/abort.any.worker-expected.txt:8
> +FAIL a signal argument '[object AbortSignal]' should cause pipeTo() to reject Argument 1 ('object') to AbortSignal.whenSignalAborted must be an instance of AbortSignal

Hum, it seems that our check of (signal instanceof @AbortSignal) is not good enough since Object.create(AbortSignal.prototype) passes it.
That makes ReadableStream.js pipeTo and pipeThrough not throwing/rejecting when checking signal while they should do.

We should fix this as a follow-up.
I am not sure what the best way to do is. Is there a better way than adding a PrivateIdentifier method to AbortSignal itself?

[Alexey Shvayka]

(In reply to youenn fablet from comment #4)

Thanks for review!

> We should fix this as a follow-up.
> I am not sure what the best way to do is. Is there a better way than adding
> a PrivateIdentifier method to AbortSignal itself?

In a follow-up, I will introduce @isAbortSignal global private function, mimicking JSC intrinsics, because WebIDL code generator is not equipped to generate such brand check.

[Alexey Shvayka]

Committed r279567 (239398@main): <https://commits.webkit.org/239398@main>

[Radar WebKit Bug Importer]

<rdar://problem/80177224>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 227732

[Ahmad Saleem]

This got backed out in following commit:

https://github.com/WebKit/WebKit/commit/d0333311de3aab7c2067acc61e7b9e0eb881eb03

But Safari Technical Preview 150 passes all test but fail harness test:

https://wpt.fyi/results/streams/piping?label=master&label=experimental&aligned&view=subtest&q=pipe-through

Is something still required since all tests passes here? Thanks!

[Alexey Shvayka]

(In reply to Ahmad Saleem from comment #9)
> This got backed out in following commit:
> 
> https://github.com/WebKit/WebKit/commit/
> d0333311de3aab7c2067acc61e7b9e0eb881eb03
> 
> But Safari Technical Preview 150 passes all test but fail harness test:
> 
> https://wpt.fyi/results/streams/
> piping?label=master&label=experimental&aligned&view=subtest&q=pipe-through
> 
> Is something still required since all tests passes here? Thanks!

Purely a refactoring at this point: it would be nice to drop `whenSignalAborted` from JSDOMGlobalObject.cpp.

[Alexey Proskuryakov]

Comment on attachment 432876 [details]
Patch

Clearing r+ given the above.

[Alexey Shvayka]

Closing since whenSignalAborted() was removed in https://github.com/WebKit/WebKit/commit/cb01f4ca942d355c84e1c6248560cf63a58ebfd0.