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
Created attachment 450704 [details] Patch
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.
(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.
Created attachment 450771 [details] Patch
Requesting review again because I simplified the patch a bit by leveraging DOMTimer instead of introducing an internal Timer for AbortSignal.
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&.
(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.
Committed r289058 (?): <https://commits.webkit.org/r289058>
<rdar://problem/88441350>
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.
(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.
(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.