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; } }
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?
*** Bug 197828 has been marked as a duplicate of this bug. ***
<rdar://problem/76884264>
any solution?
use V8
(In reply to chyizheng from comment #4) > any solution? use V8
Created attachment 430801 [details] Patch
Created attachment 430884 [details] Patch
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 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 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 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 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 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?
(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.
Created attachment 431093 [details] Patch
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 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 on attachment 431093 [details] Patch r=me
Created attachment 431120 [details] Patch for landing
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].
(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
Re-opened since this is blocked by bug 226973
Created attachment 431342 [details] Patch
Created attachment 431449 [details] Patch
Created attachment 431451 [details] Patch
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...
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].