Bug 161942 - Shouldn't drain the micro task queue when calling out to ObjC
Summary: Shouldn't drain the micro task queue when calling out to ObjC
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords: InRadar
: 197828 (view as bug list)
Depends on: 226973
Blocks:
  Show dependency treegraph
 
Reported: 2016-09-13 19:29 PDT by Michael Saboff
Modified: 2022-05-23 15:33 PDT (History)
14 users (show)

See Also:


Attachments
Patch (5.65 KB, patch)
2021-06-07 19:36 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (7.29 KB, patch)
2021-06-08 13:29 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (8.36 KB, patch)
2021-06-10 11:05 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (9.20 KB, patch)
2021-06-10 13:43 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (8.36 KB, patch)
2021-06-14 10:26 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch (8.84 KB, patch)
2021-06-15 09:38 PDT, Keith Miller
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (9.06 KB, patch)
2021-06-15 09:53 PDT, Keith Miller
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2016-09-13 19:29:54 PDT
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;
    }
}
Comment 1 chen 2019-05-15 23:01:21 PDT
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?
Comment 2 Yusuke Suzuki 2021-04-20 00:15:42 PDT
*** Bug 197828 has been marked as a duplicate of this bug. ***
Comment 3 Radar WebKit Bug Importer 2021-04-20 00:16:13 PDT
<rdar://problem/76884264>
Comment 4 chyizheng 2021-04-20 01:13:41 PDT
any solution?
Comment 5 chen 2021-04-21 00:55:16 PDT Comment hidden (spam)
Comment 6 chen 2021-04-21 00:55:36 PDT Comment hidden (spam)
Comment 7 Keith Miller 2021-06-07 19:35:04 PDT
*** Bug 197828 has been marked as a duplicate of this bug. ***
Comment 8 Keith Miller 2021-06-07 19:36:39 PDT
Created attachment 430801 [details]
Patch
Comment 9 Keith Miller 2021-06-08 13:29:44 PDT
Created attachment 430884 [details]
Patch
Comment 10 Saam Barati 2021-06-09 17:24:48 PDT
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?
Comment 11 Saam Barati 2021-06-09 17:26:24 PDT
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"
Comment 12 Saam Barati 2021-06-09 17:31:12 PDT
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"
Comment 13 Tim Horton 2021-06-09 18:11:18 PDT
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.
Comment 14 Keith Miller 2021-06-10 09:52:36 PDT
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.
Comment 15 Saam Barati 2021-06-10 10:39:34 PDT
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?
Comment 16 Keith Miller 2021-06-10 10:56:16 PDT
(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.
Comment 17 Keith Miller 2021-06-10 11:05:38 PDT
Created attachment 431093 [details]
Patch
Comment 18 Saam Barati 2021-06-10 11:09:08 PDT
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?
Comment 19 Saam Barati 2021-06-10 11:09:26 PDT
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.
Comment 20 Saam Barati 2021-06-10 12:44:31 PDT
Comment on attachment 431093 [details]
Patch

r=me
Comment 21 Keith Miller 2021-06-10 13:43:08 PDT
Created attachment 431120 [details]
Patch for landing
Comment 22 EWS 2021-06-10 14:54:43 PDT
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].
Comment 23 Aakash Jain 2021-06-14 07:44:57 PDT
(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
Comment 24 WebKit Commit Bot 2021-06-14 07:56:18 PDT
Re-opened since this is blocked by bug 226973
Comment 25 Keith Miller 2021-06-14 10:26:16 PDT
Created attachment 431342 [details]
Patch
Comment 26 Keith Miller 2021-06-15 09:38:47 PDT
Created attachment 431449 [details]
Patch
Comment 27 Keith Miller 2021-06-15 09:53:29 PDT
Created attachment 431451 [details]
Patch
Comment 28 Keith Miller 2021-06-15 10:16:18 PDT
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...
Comment 29 EWS 2021-06-15 11:39:35 PDT
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].