Bug 161942 – Shouldn't drain the micro task queue when calling out to ObjC

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Saboff]

This is from https://bugs.webkit.org/show_bug.cgi?id=161929#c3

    It occurs to me that it is also a bug that we drain the micro task queue at all when calling out to ObjC. That should only happen when we exit the VM.

Currently we drain micro tasks in willReleaseLock():

void JSLock::willReleaseLock()
{
    RefPtr<VM> vm = m_vm;
    if (vm) {
        vm->drainMicrotasks();

        vm->heap.releaseDelayedReleasedObjects();
        vm->setStackPointerAtVMEntry(nullptr);
    }

    if (m_entryAtomicStringTable) {
        wtfThreadData().setCurrentAtomicStringTable(m_entryAtomicStringTable);
        m_entryAtomicStringTable = nullptr;
    }
}

[chen]

after 3 years，this bug haven't fix ....


same issue https://bugs.webkit.org/show_bug.cgi?id=197828

@Michael Saboff  did you have solution?

[Yusuke Suzuki]

*** Bug 197828 has been marked as a duplicate of this bug. ***

[Radar WebKit Bug Importer]

<rdar://problem/76884264>

[chyizheng]

any solution?

[chen]

use V8

[chen]

(In reply to chyizheng from comment #4)
> any solution?

use V8

[Keith Miller]

*** Bug 197828 has been marked as a duplicate of this bug. ***

[Keith Miller]

Created attachment 430801 [details]
Patch

[Keith Miller]

Created attachment 430884 [details]
Patch

[Saam Barati]

Comment on attachment 430884 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSLock.cpp:205
> +#if USE(APPLE_INTERNAL_SDK)

why only APPLE_INTERNAL_SDK?

> Source/JavaScriptCore/runtime/VM.cpp:1363
>  void VM::didExhaustMicrotaskQueue()

Why this change?

Also why is this function named like it's gonna return a bool?

[Saam Barati]

Comment on attachment 430884 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:17
> +        a linked on or after check to protect existing APPs.

"APPs" => "Apps"

[Saam Barati]

Comment on attachment 430884 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSLock.cpp:218
> +        if (!vm->topCallFrame || useLegacyDrain)

why this "!vm->topCallFrame" check? Is that your "I'm leaving the VM" check? If so, let's just move this to ~VMEntryScope, and keep the legacy stuff here under "useLegacyDrain"

[Tim Horton]

Comment on attachment 430884 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSLock.cpp:205
>> +#if USE(APPLE_INTERNAL_SDK)
> 
> why only APPLE_INTERNAL_SDK?

Also likely should be using applicationSDKVersion() like the rest of WebKit.

[Keith Miller]

Comment on attachment 430884 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:17
>> +        a linked on or after check to protect existing APPs.
> 
> "APPs" => "Apps"

Fixed.

>>> Source/JavaScriptCore/runtime/JSLock.cpp:205
>>> +#if USE(APPLE_INTERNAL_SDK)
>> 
>> why only APPLE_INTERNAL_SDK?
> 
> Also likely should be using applicationSDKVersion() like the rest of WebKit.

dyld_get_program_sdk_version() is SPI. Per offline discussion, it sounds like the current code is fine?

>> Source/JavaScriptCore/runtime/JSLock.cpp:218
>> +        if (!vm->topCallFrame || useLegacyDrain)
> 
> why this "!vm->topCallFrame" check? Is that your "I'm leaving the VM" check? If so, let's just move this to ~VMEntryScope, and keep the legacy stuff here under "useLegacyDrain"

Yeah, that's the I'm leaving the VM check. We can't do it in ~VMEntryScope since the API can only collect the exception right before releasing the JSLock. If we moved it to ~VMEntryScope, the exception could get overriden by an exception in a microtask. We'd have to restructure a ton of code to make it work in ~VMEntryScope.

>> Source/JavaScriptCore/runtime/VM.cpp:1363
>>  void VM::didExhaustMicrotaskQueue()
> 
> Why this change?
> 
> Also why is this function named like it's gonna return a bool?

It makes it so we handle unhandled rejections that were created while handling other unhandled rejections. See promiseUnhandledRejectionFromUnhandledRejectionCallback() in testapi.cpp

I think it's meant as you call this when you've exhausted the microtask queue. I agree the name's weird.

[Saam Barati]

Comment on attachment 430884 [details]
Patch

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

>>>> Source/JavaScriptCore/runtime/JSLock.cpp:205
>>>> +#if USE(APPLE_INTERNAL_SDK)
>>> 
>>> why only APPLE_INTERNAL_SDK?
>> 
>> Also likely should be using applicationSDKVersion() like the rest of WebKit.
> 
> dyld_get_program_sdk_version() is SPI. Per offline discussion, it sounds like the current code is fine?

Just because it's SPI doesn't mean it shouldn't be compiled into open source builds. That's why we have forwarding headers.

Not sure what the difference is between dyld_get_program_sdk_version and applicationSDKVersion. I'd ask Tim.

>>> Source/JavaScriptCore/runtime/JSLock.cpp:218
>>> +        if (!vm->topCallFrame || useLegacyDrain)
>> 
>> why this "!vm->topCallFrame" check? Is that your "I'm leaving the VM" check? If so, let's just move this to ~VMEntryScope, and keep the legacy stuff here under "useLegacyDrain"
> 
> Yeah, that's the I'm leaving the VM check. We can't do it in ~VMEntryScope since the API can only collect the exception right before releasing the JSLock. If we moved it to ~VMEntryScope, the exception could get overriden by an exception in a microtask. We'd have to restructure a ton of code to make it work in ~VMEntryScope.

I feel like "!vm->topCallFrame" is hacky. Why not check if we're fully releasing the lock?

[Keith Miller]

(In reply to Saam Barati from comment #15)
> Comment on attachment 430884 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=430884&action=review
> 
> >>>> Source/JavaScriptCore/runtime/JSLock.cpp:205
> >>>> +#if USE(APPLE_INTERNAL_SDK)
> >>> 
> >>> why only APPLE_INTERNAL_SDK?
> >> 
> >> Also likely should be using applicationSDKVersion() like the rest of WebKit.
> > 
> > dyld_get_program_sdk_version() is SPI. Per offline discussion, it sounds like the current code is fine?
> 
> Just because it's SPI doesn't mean it shouldn't be compiled into open source
> builds. That's why we have forwarding headers.
> 
> Not sure what the difference is between dyld_get_program_sdk_version and
> applicationSDKVersion. I'd ask Tim.

I guess I'll switch to applicationSDKVersion since it seems more robust.

> 
> >>> Source/JavaScriptCore/runtime/JSLock.cpp:218
> >>> +        if (!vm->topCallFrame || useLegacyDrain)
> >> 
> >> why this "!vm->topCallFrame" check? Is that your "I'm leaving the VM" check? If so, let's just move this to ~VMEntryScope, and keep the legacy stuff here under "useLegacyDrain"
> > 
> > Yeah, that's the I'm leaving the VM check. We can't do it in ~VMEntryScope since the API can only collect the exception right before releasing the JSLock. If we moved it to ~VMEntryScope, the exception could get overriden by an exception in a microtask. We'd have to restructure a ton of code to make it work in ~VMEntryScope.
> 
> I feel like "!vm->topCallFrame" is hacky. Why not check if we're fully
> releasing the lock?

Sure, I can switch back to that.

[Keith Miller]

Created attachment 431093 [details]
Patch

[Saam Barati]

Comment on attachment 431093 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSLock.cpp:218
> +        if (!m_lockDropDepth || useLegacyDrain)

!m_lockDropDepth is not the right check here, since it doesn't account for recursively grabbing the lock.

Can you add a test for the new behavior that would catch this?

[Saam Barati]

Comment on attachment 431093 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSLock.cpp:218
>> +        if (!m_lockDropDepth || useLegacyDrain)
> 
> !m_lockDropDepth is not the right check here, since it doesn't account for recursively grabbing the lock.
> 
> Can you add a test for the new behavior that would catch this?

Oh wait, ignore me! I misunderstood. Looks good.

[Saam Barati]

Comment on attachment 431093 [details]
Patch

r=me

[Keith Miller]

Created attachment 431120 [details]
Patch for landing

[EWS]

Committed r278734 (238697@main): <https://commits.webkit.org/238697@main>

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

[Aakash Jain]

(In reply to EWS from comment #22)
> Committed r278734 (238697@main): <https://commits.webkit.org/238697@main>
Seems like it broke testapi

History: https://results.webkit.org/?suite=javascriptcore-tests&test=testapi

[WebKit Commit Bot]

Re-opened since this is blocked by bug 226973

[Keith Miller]

Created attachment 431342 [details]
Patch

[Keith Miller]

Created attachment 431449 [details]
Patch

[Keith Miller]

Created attachment 431451 [details]
Patch

[Keith Miller]

Comment on attachment 431451 [details]
Patch

Ugh, the testapi issue was that I forgot to put the linked on or after checks on one of my tests...

[EWS]

Committed r278888 (238830@main): <https://commits.webkit.org/238830@main>

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