Bug 236039 – Implement AbortSignal.timeout()

• Component: DOM
• Status: RESOLVED FIXED

[Domenic Denicola]

Spec PR/mini-explainer (landed): https://github.com/whatwg/dom/pull/1032

Spec: https://dom.spec.whatwg.org/#dom-abortsignal-timeout

Web platform tests PR (landed): https://github.com/web-platform-tests/wpt/pull/32622

[Chris Dumez]

Created attachment 450704 [details]
Patch

[Darin Adler]

Comment on attachment 450704 [details]
Patch

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

> Source/WebCore/dom/AbortSignal.cpp:135
> +    auto* context = scriptExecutionContext();
> +    if (!context || context->activeDOMObjectsAreStopped())
> +        return;
> +
> +    auto* globalObject = wrapper() ? wrapper()->globalObject() : nullptr;
> +    if (!globalObject)
> +        return;
> +
> +    auto& vm = globalObject->vm();
> +    Locker locker { vm.apiLock() };
> +    signalAbort(toJS(globalObject, globalObject, DOMException::create(TimeoutError)));

Annoying we have to write so much custom binding code for this.

[Chris Dumez]

(In reply to Darin Adler from comment #2)
> Comment on attachment 450704 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450704&action=review
> 
> > Source/WebCore/dom/AbortSignal.cpp:135
> > +    auto* context = scriptExecutionContext();
> > +    if (!context || context->activeDOMObjectsAreStopped())
> > +        return;
> > +
> > +    auto* globalObject = wrapper() ? wrapper()->globalObject() : nullptr;
> > +    if (!globalObject)
> > +        return;
> > +
> > +    auto& vm = globalObject->vm();
> > +    Locker locker { vm.apiLock() };
> > +    signalAbort(toJS(globalObject, globalObject, DOMException::create(TimeoutError)));
> 
> Annoying we have to write so much custom binding code for this.

Yes. I suspect we may be able to use ActiveDOMObject instead. I'll look into it in a follow-up.

[Chris Dumez]

Created attachment 450771 [details]
Patch

[Chris Dumez]

Requesting review again because I simplified the patch a bit by leveraging DOMTimer instead of introducing an internal Timer for AbortSignal.

[Darin Adler]

Comment on attachment 450771 [details]
Patch

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

> Source/WebCore/dom/AbortSignal.cpp:65
> +        auto* globalObject = context.globalObject();

We should do the jsCast here, not wait until below to do it:

    auto* globalObject = jsCast<JSDOMGlobalObject*>(context.globalObject());

But also, and this is the patch I am working on, ScriptExecutionContext’s globalObject function should return JSDOMGlobalObject* or JSDOMGlobalObject&.

[Chris Dumez]

(In reply to Darin Adler from comment #6)
> Comment on attachment 450771 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450771&action=review
> 
> > Source/WebCore/dom/AbortSignal.cpp:65
> > +        auto* globalObject = context.globalObject();
> 
> We should do the jsCast here, not wait until below to do it:
> 
>     auto* globalObject = jsCast<JSDOMGlobalObject*>(context.globalObject());
> 
> But also, and this is the patch I am working on, ScriptExecutionContext’s
> globalObject function should return JSDOMGlobalObject* or JSDOMGlobalObject&.

Agreed.

Thanks for reviewing.

[Chris Dumez]

Committed r289058 (?): <https://commits.webkit.org/r289058>

[Radar WebKit Bug Importer]

<rdar://problem/88441350>

[Alexey Shvayka]

Comment on attachment 450771 [details]
Patch

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

> Source/WebCore/dom/AbortSignal.idl:36
> +    [NewObject, CallWith=ScriptExecutionContext] static AbortSignal timeout([EnforceRange] unsigned long long milliseconds);

Shouldn't this be `CallWith=RelevantScriptExecutionContext` as step 2 explicitly requires _relevant_?
I believe the difference could be captured by 2 test cases:
  a) timeout() method from detached realm should still abort the signal, and
  b) timeout() method from another realm should create DOMException in signal's realm.

CallWith values naming is currently quite confusing (my bad). I will rename ScriptExecutionContext => CurrentScriptExecutionContext soonish.

[Chris Dumez]

(In reply to Alexey Shvayka from comment #10)
> Comment on attachment 450771 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=450771&action=review
> 
> > Source/WebCore/dom/AbortSignal.idl:36
> > +    [NewObject, CallWith=ScriptExecutionContext] static AbortSignal timeout([EnforceRange] unsigned long long milliseconds);
> 
> Shouldn't this be `CallWith=RelevantScriptExecutionContext` as step 2
> explicitly requires _relevant_?
> I believe the difference could be captured by 2 test cases:
>   a) timeout() method from detached realm should still abort the signal, and
>   b) timeout() method from another realm should create DOMException in
> signal's realm.
> 
> CallWith values naming is currently quite confusing (my bad). I will rename
> ScriptExecutionContext => CurrentScriptExecutionContext soonish.

FYI, CallWith=RelevantScriptExecutionContext doesn't build:
/Volumes/Data/WebKit/OpenSource/WebKitBuild/Debug/DerivedSources/WebCore/JSAbortSignal.cpp:297:23: error: use of undeclared identifier 'castedThis'
    auto* context = (*castedThis).globalObject()->scriptExecutionContext();

Probably because this is a static function.


Note that there is one test that involves a detached iframe (LayoutTests/imported/w3c/web-platform-tests/dom/abort/abort-signal-timeout.html) which seems to be passing with my patch.

[Alexey Shvayka]

(In reply to Chris Dumez from comment #11)
> (In reply to Alexey Shvayka from comment #10)
> > Comment on attachment 450771 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=450771&action=review
> > 
> > > Source/WebCore/dom/AbortSignal.idl:36
> > > +    [NewObject, CallWith=ScriptExecutionContext] static AbortSignal timeout([EnforceRange] unsigned long long milliseconds);
> > 
> > Shouldn't this be `CallWith=RelevantScriptExecutionContext` as step 2
> > explicitly requires _relevant_?
> > I believe the difference could be captured by 2 test cases:
> >   a) timeout() method from detached realm should still abort the signal, and
> >   b) timeout() method from another realm should create DOMException in
> > signal's realm.
> > 
> > CallWith values naming is currently quite confusing (my bad). I will rename
> > ScriptExecutionContext => CurrentScriptExecutionContext soonish.
> 
> FYI, CallWith=RelevantScriptExecutionContext doesn't build:
> /Volumes/Data/WebKit/OpenSource/WebKitBuild/Debug/DerivedSources/WebCore/
> JSAbortSignal.cpp:297:23: error: use of undeclared identifier 'castedThis'
>     auto* context = (*castedThis).globalObject()->scriptExecutionContext();
> 
> Probably because this is a static function.

Ooh, sorry for the noise, that "signal’s relevant global object" spec wording got me all confused.

The signal is a newly-created object in a static function, it can't possibly have any other script execution context.