Bug 214508 – Finalizers shouldn't run if events can't fire

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Keith Miller]

Currently, we don't have a mechanism for WebCore to tell JSC which windows are suspended/killed. This means that JSC will continue to run finalizers for iframes that have been detached from the document. Normally, those windows don't trigger and events on the runloop. This is already a bug for Wasm but that's less obvious than finalizers.

[Radar WebKit Bug Importer]

<rdar://problem/66079646>

[Keith Miller]

Created attachment 410261 [details]
WIP

[Keith Miller]

Created attachment 410549 [details]
WIP

[Keith Miller]

Created attachment 410876 [details]
Patch

[Keith Miller]

Created attachment 410880 [details]
Patch

[Keith Miller]

Created attachment 410886 [details]
Patch

[Keith Miller]

Created attachment 410891 [details]
Build-fix

[Ryosuke Niwa]

Comment on attachment 410891 [details]
Build-fix

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

> Source/WebCore/ChangeLog:13
> +        the ticket).  The only exception to this is if the global object

Nit: Two spaces after period. We use single space.

> Source/JavaScriptCore/runtime/DeferredWorkTimer.cpp:62
> +        // We may have already canceled this task or its owner may have been canceled.
> +        if (pendingTicket == m_pendingTickets.end())

It's better to define a descriptive bool in that case like so:
bool ticketOrTickerOwnerHasBeenCanceled = pendingTicket == m_pendingTickets.end();
if (ticketOrTickerOwnerHasBeenCanceled)

> Source/JavaScriptCore/runtime/DeferredWorkTimer.cpp:73
> +        default:
> +            break;

Can we explicitly call out Running so that we'd catch it when a new value is added to enum without updating this code?

> Source/JavaScriptCore/runtime/DeferredWorkTimer.cpp:130
> +    dataLogLnIf(DeferredWorkTimerInternal::verbose, "Adding new pending ticket: ", RawPointer(ticket));

This data log doesn't seem to belong here? Should be inside ensure, right?

> Source/JavaScriptCore/runtime/JSGlobalObject.h:201
> +    Canceled,

Can we call this stopped to match WebCore concepts?
Canceled isn't a conceptually right terminology here.
Script execution isn't canceled. It has been stopped.

> Source/WTF/wtf/Locker.h:91
> -        m_lockable->unlock();
> +        unlock();

Hm... this code will now check m_lockable.
Are you sure that won't have a negative perf impact?

> Source/WTF/wtf/Locker.h:164
> +class DropLockScope : private AbstractLocker {

Hm... DropLockScope sounds oxymoron. Either you've dropped or not.
How about UnlockScope or UnlockForScope? Maybe UnlockerScope or just Unlocker?

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

This blank line seems unnecessary.

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

What is this code change about??

> Source/WebCore/dom/ScriptExecutionContext.cpp:299
> +    vm().deferredWorkTimer->scriptExecutionOwnerWasUnsuspended();

I'd called this scriptExecutionIsResumed or simply resumeScriptExecution.

> LayoutTests/ChangeLog:7
> +

Say that we're adding regression tests?

> LayoutTests/fast/frames/detached-frame-wasm-resolve.html:1
> +<script src="../../resources/js-test.js"></script>

Missing DOCTYPE.

> LayoutTests/fast/frames/detached-frame-wasm-resolve.html:13
> +    promise.then(() => subFrameCalled = true);

Can we make wrap this as an async function so that we can do:
(async () => {
await detachedWindow.WebAssembly.compile(~);
subFrameCalled = true;
})();

> LayoutTests/fast/frames/detached-frame-wasm-resolve.html:15
> +    promise = WebAssembly.compile(new Uint8Array([ 0,97,115,109,1,0,0,0,1,136,128,128,128,0,2,96,0,0,96,0,1,124,3,131,128,128,128,0,2,0,1,7,138,128,128,128,0,1,6,114,117,110,102,54,52,0,1,10,165,128,128,128,0,2,130,128,128,128,0,0,11,152,128,128,128,0,0,3,124,68,0,0,0,0,0,0,0,0,65,0,14,0,1,16,0,11,12,0,0,11 ]));

Ditto. It's confusing to override the same variable here.

> LayoutTests/fast/frames/detached-frame-wasm-resolve.html:25
> +        // Add small timeout to be safe.
> +        }, 100);

100ms isn't nearly enough on bots.
Can we instead schedule a new WASM compilation here and wait for it complete?
Presumably, that new completion should (or likely) complete later than the pervious compilation?

> LayoutTests/fast/history/page-cache-wasm-promise-resolve.html:42
> +window.addEventListener("pagehide", function(event) {

Can we make this async?

> LayoutTests/fast/history/page-cache-wasm-promise-resolve.html:52
> +    promise.then(check);

So that we can have the content of check here instead?
Well, after setting notSuspended = false first.

[Keith Miller]

Comment on attachment 410891 [details]
Build-fix

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

>> Source/WebCore/ChangeLog:13
>> +        the ticket).  The only exception to this is if the global object
> 
> Nit: Two spaces after period. We use single space.

Sure.

>> Source/JavaScriptCore/runtime/DeferredWorkTimer.cpp:62
>> +        if (pendingTicket == m_pendingTickets.end())
> 
> It's better to define a descriptive bool in that case like so:
> bool ticketOrTickerOwnerHasBeenCanceled = pendingTicket == m_pendingTickets.end();
> if (ticketOrTickerOwnerHasBeenCanceled)

Sure.

>> Source/JavaScriptCore/runtime/DeferredWorkTimer.cpp:73
>> +            break;
> 
> Can we explicitly call out Running so that we'd catch it when a new value is added to enum without updating this code?

Sure, although, it's a little hard to imagine what that new case could be. :P

>> Source/JavaScriptCore/runtime/DeferredWorkTimer.cpp:130
>> +    dataLogLnIf(DeferredWorkTimerInternal::verbose, "Adding new pending ticket: ", RawPointer(ticket));
> 
> This data log doesn't seem to belong here? Should be inside ensure, right?

Yeah, I guess I lost track of the other one while refactoring. I'll add this to the ensure and put the old one back.

>> Source/JavaScriptCore/runtime/JSGlobalObject.h:201
>> +    Canceled,
> 
> Can we call this stopped to match WebCore concepts?
> Canceled isn't a conceptually right terminology here.
> Script execution isn't canceled. It has been stopped.

I went with canceled because stopped, to me at least, implies that it can be resumed. But AFAICT that's not true. So I went with Canceled for JSC.

>> Source/WTF/wtf/Locker.h:91
>> +        unlock();
> 
> Hm... this code will now check m_lockable.
> Are you sure that won't have a negative perf impact?

I don't think so, there's only ~5 uses of "unlockEarly()" in the codebase. None of them seem obviously hot and all of them are in the same code as the lock acquire. So my guess is the null check gets eliminated. Also, the atomic instructions of the locking are likely way more expensive than a predicted branch will ever be.

>> Source/WTF/wtf/Locker.h:164
>> +class DropLockScope : private AbstractLocker {
> 
> Hm... DropLockScope sounds oxymoron. Either you've dropped or not.
> How about UnlockScope or UnlockForScope? Maybe UnlockerScope or just Unlocker?

That's the terminology used in JSC for dropping the API lock. So I'd like to keep it consistent. If we think `Unlock` is the right name then we should do a comprehensive rename.

>> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:240
>> +
> 
> This blank line seems unnecessary.

fixed.

>> Source/WebCore/dom/ScriptExecutionContext.cpp:80
>> +static HashMap<ScriptExecutionContextIdentifier, ScriptExecutionContext*>& allScriptExecutionContextsMap(const Locker<Lock>&)
> 
> What is this code change about??

Just API cleanup. If you have code that expects a lock to be held while called you should take the locker to force people to do it.

>> Source/WebCore/dom/ScriptExecutionContext.cpp:299
>> +    vm().deferredWorkTimer->scriptExecutionOwnerWasUnsuspended();
> 
> I'd called this scriptExecutionIsResumed or simply resumeScriptExecution.

How about `scriptExecutionResumed`?

>> LayoutTests/ChangeLog:7
>> +
> 
> Say that we're adding regression tests?

Isn't that implied by the tests added below?

>> LayoutTests/fast/frames/detached-frame-wasm-resolve.html:1
>> +<script src="../../resources/js-test.js"></script>
> 
> Missing DOCTYPE.

Sure.

>> LayoutTests/fast/frames/detached-frame-wasm-resolve.html:13
>> +    promise.then(() => subFrameCalled = true);
> 
> Can we make wrap this as an async function so that we can do:
> (async () => {
> await detachedWindow.WebAssembly.compile(~);
> subFrameCalled = true;
> })();

That seems more complicated?

>> LayoutTests/fast/frames/detached-frame-wasm-resolve.html:15
>> +    promise = WebAssembly.compile(new Uint8Array([ 0,97,115,109,1,0,0,0,1,136,128,128,128,0,2,96,0,0,96,0,1,124,3,131,128,128,128,0,2,0,1,7,138,128,128,128,0,1,6,114,117,110,102,54,52,0,1,10,165,128,128,128,0,2,130,128,128,128,0,0,11,152,128,128,128,0,0,3,124,68,0,0,0,0,0,0,0,0,65,0,14,0,1,16,0,11,12,0,0,11 ]));
> 
> Ditto. It's confusing to override the same variable here.

Sure. I can give a different name.

>> LayoutTests/fast/frames/detached-frame-wasm-resolve.html:25
>> +        }, 100);
> 
> 100ms isn't nearly enough on bots.
> Can we instead schedule a new WASM compilation here and wait for it complete?
> Presumably, that new completion should (or likely) complete later than the pervious compilation?

The timeout shouldn't really be needed. I can probably just get rid of it.

>> LayoutTests/fast/history/page-cache-wasm-promise-resolve.html:52
>> +    promise.then(check);
> 
> So that we can have the content of check here instead?
> Well, after setting notSuspended = false first.

Sure.

[Ryosuke Niwa]

(In reply to Keith Miller from comment #9)
> Comment on attachment 410891 [details]
> Build-fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=410891&action=review
>
> >> Source/JavaScriptCore/runtime/JSGlobalObject.h:201
> >> +    Canceled,
> > 
> > Can we call this stopped to match WebCore concepts?
> > Canceled isn't a conceptually right terminology here.
> > Script execution isn't canceled. It has been stopped.
> 
> I went with canceled because stopped, to me at least, implies that it can be
> resumed. But AFAICT that's not true. So I went with Canceled for JSC.

Given the other value in this enum is suspended, I think stopped would make it clear it's not resumable.
It's better to match terminology across components so that it's easier to navigate between them.

> >> Source/WTF/wtf/Locker.h:164
> >> +class DropLockScope : private AbstractLocker {
> > 
> > Hm... DropLockScope sounds oxymoron. Either you've dropped or not.
> > How about UnlockScope or UnlockForScope? Maybe UnlockerScope or just Unlocker?
> 
> That's the terminology used in JSC for dropping the API lock. So I'd like to
> keep it consistent. If we think `Unlock` is the right name then we should do
> a comprehensive rename.

Then we should probably call this DropLockForScope.

> >> Source/WebCore/dom/ScriptExecutionContext.cpp:80
> >> +static HashMap<ScriptExecutionContextIdentifier, ScriptExecutionContext*>& allScriptExecutionContextsMap(const Locker<Lock>&)
> > 
> > What is this code change about??
> 
> Just API cleanup. If you have code that expects a lock to be held while
> called you should take the locker to force people to do it.

I don't think we do that elsewhere in the codebase so I don't think we should that here either.

> >> Source/WebCore/dom/ScriptExecutionContext.cpp:299
> >> +    vm().deferredWorkTimer->scriptExecutionOwnerWasUnsuspended();
> > 
> > I'd called this scriptExecutionIsResumed or simply resumeScriptExecution.
> 
> How about `scriptExecutionResumed`?

Sure, that's okay although we tend to use "did" so probably more like didResumeScriptExecution or scriptExecutionDidResume.

> >> LayoutTests/ChangeLog:7
> >> +
> > 
> > Say that we're adding regression tests?
> 
> Isn't that implied by the tests added below?

Yeah, but we should still put a description in the change log as to what kind of tests are being added.

[Keith Miller]

(In reply to Ryosuke Niwa from comment #10)
> (In reply to Keith Miller from comment #9)
> > Comment on attachment 410891 [details]
> > Build-fix
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=410891&action=review
> >
> > >> Source/JavaScriptCore/runtime/JSGlobalObject.h:201
> > >> +    Canceled,
> > > 
> > > Can we call this stopped to match WebCore concepts?
> > > Canceled isn't a conceptually right terminology here.
> > > Script execution isn't canceled. It has been stopped.
> > 
> > I went with canceled because stopped, to me at least, implies that it can be
> > resumed. But AFAICT that's not true. So I went with Canceled for JSC.
> 
> Given the other value in this enum is suspended, I think stopped would make
> it clear it's not resumable.
> It's better to match terminology across components so that it's easier to
> navigate between them.

Fair enough, I made the same argument below so I guess I can't complain :P

> 
> > >> Source/WTF/wtf/Locker.h:164
> > >> +class DropLockScope : private AbstractLocker {
> > > 
> > > Hm... DropLockScope sounds oxymoron. Either you've dropped or not.
> > > How about UnlockScope or UnlockForScope? Maybe UnlockerScope or just Unlocker?
> > 
> > That's the terminology used in JSC for dropping the API lock. So I'd like to
> > keep it consistent. If we think `Unlock` is the right name then we should do
> > a comprehensive rename.
> 
> Then we should probably call this DropLockForScope.

Sure.

> 
> > >> Source/WebCore/dom/ScriptExecutionContext.cpp:80
> > >> +static HashMap<ScriptExecutionContextIdentifier, ScriptExecutionContext*>& allScriptExecutionContextsMap(const Locker<Lock>&)
> > > 
> > > What is this code change about??
> > 
> > Just API cleanup. If you have code that expects a lock to be held while
> > called you should take the locker to force people to do it.
> 
> I don't think we do that elsewhere in the codebase so I don't think we
> should that here either.

It's a very common idiom in JSC, at least, that's why we have the NoLockingNecessary enum so you can call things that would normally require a lock without actually taking the lock (when you know it's safe obviously).

> 
> > >> Source/WebCore/dom/ScriptExecutionContext.cpp:299
> > >> +    vm().deferredWorkTimer->scriptExecutionOwnerWasUnsuspended();
> > > 
> > > I'd called this scriptExecutionIsResumed or simply resumeScriptExecution.
> > 
> > How about `scriptExecutionResumed`?
> 
> Sure, that's okay although we tend to use "did" so probably more like
> didResumeScriptExecution or scriptExecutionDidResume.

Sure didResumeScriptExecution sounds good.

> 
> > >> LayoutTests/ChangeLog:7
> > >> +
> > > 
> > > Say that we're adding regression tests?
> > 
> > Isn't that implied by the tests added below?
> 
> Yeah, but we should still put a description in the change log as to what
> kind of tests are being added.

Ok

[Keith Miller]

Created attachment 410910 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 410910 [details]
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?

[Keith Miller]

Comment on attachment 410910 [details]
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.

Sure, I can move it into the switch.

>> 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.

Kinda but it's a bit more precise since it tells you exactly when it's the JSDocument. Perhaps that's unnecessary though.

>> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:240
>> +
> 
> This blank line seems unnecessary?

Sorry forgot to remove before. Will do.

>> Source/WebCore/bindings/js/JSDOMWindowBase.cpp:247
>> +    return document->wrapped().jscScriptExecutionStatus();
> 
> I would have done this in a single line instead.

Ok.

>> Source/WebCore/bindings/js/JSWorkerGlobalScopeBase.cpp:140
>> +    return thisObject->scriptExecutionContext()->jscScriptExecutionStatus();
> 
> Ditto.

Done.

>> Source/WebCore/bindings/js/JSWorkletGlobalScopeBase.cpp:109
>> +    return thisObject->scriptExecutionContext()->jscScriptExecutionStatus();
> 
> Ditto.

Done.

>> Source/WebCore/dom/ScriptExecutionContext.cpp:80
>> +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.

Alright that's fine. I'll revert.

>> Source/WebCore/dom/ScriptExecutionContext.cpp:261
>> +        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.

Gotcha. Wasn't sure if the order mattered. From skimming the code it looks like we stop suspending but I may have missed some cases. I'll flip.

>> LayoutTests/fast/frames/detached-frame-wasm-resolve.html:19
>> +            if (subFrameCalled)
> 
> I thought we're gonna disable concurrent JIT?

Oh for some reason this test got untracked on git. It's not using concurrent JIT anymore. Also, most of the timeouts/promises are now awaited other than the first one since the expectation is that one never gets resolved.

> LayoutTests/platform/win/TestExpectations:704
> +fast/history/page-cache-active-finalization-registry-callback.html [ Skip ]

Whoops this is the wrong test. That's what I get for uploading a patch at 11pm... lol

[Keith Miller]

Created attachment 410947 [details]
Patch for landing

[EWS]

Committed r268271: <https://trac.webkit.org/changeset/268271>

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