[webkit-reviews] review granted: [Bug 214508] Finalizers shouldn't run if events can't fire : [Attachment 410910] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted Keith Miller
<keith_miller at apple.com>'s request for review:
Bug 214508: Finalizers shouldn't run if events can't fire
https://bugs.webkit.org/show_bug.cgi?id=214508

Attachment 410910: Patch

https://bugs.webkit.org/attachment.cgi?id=410910&action=review




--- Comment #13 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 410910
  --> https://bugs.webkit.org/attachment.cgi?id=410910
Patch

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

> Source/JavaScriptCore/runtime/DeferredWorkTimer.cpp:65
> +	   ScriptExecutionStatus status =
globalObject->globalObjectMethodTable()->scriptExecutionStatus(globalObject,
pendingTicket->value.scriptExecutionOwner.get());

It seems like this local variable is unnecessary. You can call the whole thing
in the switch.
If you're doing this for readability, I'd suggest using auto for the type name
here.

> Source/JavaScriptCore/runtime/JSGlobalObject.h:241
> +    // For most contexts this is just the global object. For JSDOMWindow,
however, this is the JSDocument.

This is a verbose way of saying that it's either JSGlobalObject or JSDocument.

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:240
> +

This blank line seems unnecessary?

> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:247
> +    auto* document = jsCast<JSDocument*>(owner);
> +    return document->wrapped().jscScriptExecutionStatus();

I would have done this in a single line instead.

> Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:140
> +    JSWorkerGlobalScopeBase* thisObject =
jsCast<JSWorkerGlobalScopeBase*>(globalObject);
> +    return thisObject->scriptExecutionContext()->jscScriptExecutionStatus();

Ditto.

> Source/WebCore/bindings/js/JSWorkletGlobalScopeBase.cpp:109
> +    auto* thisObject = jsCast<JSWorkletGlobalScopeBase*>(globalObject);
> +    return thisObject->scriptExecutionContext()->jscScriptExecutionStatus();

Ditto.

> Source/WebCore/dom/ScriptExecutionContext.cpp:80
> -static HashMap<ScriptExecutionContextIdentifier, ScriptExecutionContext*>&
allScriptExecutionContextsMap()
> +static HashMap<ScriptExecutionContextIdentifier, ScriptExecutionContext*>&
allScriptExecutionContextsMap(const Locker<Lock>&)

I think we should revert this since we don't do this in WebCore.
If you landed it, someone will come in and undo it anyway.
No need to cause a churn like that.

> Source/WebCore/dom/ScriptExecutionContext.cpp:261
> +    if (activeDOMObjectsAreStopped())
> +	   return JSC::ScriptExecutionStatus::Stopped;

We should check stopped first since that's more broader state than suspended.
e.g. ScriptExecutionContext can get suspend and then stopped for example.

> LayoutTests/fast/frames/detached-frame-wasm-resolve.html:19
> +	   setTimeout(() => {
> +	       if (subFrameCalled)

I thought we're gonna disable concurrent JIT?