- Revision
- 278734
- Author
- keith_mil...@apple.com
- Date
- 2021-06-10 14:54:36 -0700 (Thu, 10 Jun 2021)
Log Message
Shouldn't drain the micro task queue when calling out to ObjC
https://bugs.webkit.org/show_bug.cgi?id=161942
Reviewed by Saam Barati.
It looks like the issue is that we aren't checking for the
presence of dropped locks when deciding to drain microtasks during
JSLock::unlock. This meant that when we drop all locks when
calling out to API clients we would drain our microtasks at that
point. An alternative would be to pass an extra parameter to the
unlock function that says not to drain microtasks. I chose not to
do that since it seemed a bit less robust.
This patch is very likely to break existing API users. So I'm adding
a linked on or after check to protect existing Apps.
Lastly, change our Poker Bros check to use applicationSDKVersion too
so others trying to add a linked on or after check don't use
the dyld function directly too.
* API/tests/testapi.cpp:
(TestAPI::promiseDrainDoesNotEatExceptions):
(testCAPIViaCpp):
* API/tests/testapi.mm:
(testMicrotaskWithFunction):
(testObjectiveCAPI):
* runtime/JSLock.cpp:
(JSC::JSLock::willReleaseLock):
* runtime/ObjectPrototype.cpp:
(JSC::isPokerBros):
* runtime/VM.cpp:
(JSC::VM::didExhaustMicrotaskQueue):
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/API/tests/testapi.cpp (278733 => 278734)
--- trunk/Source/_javascript_Core/API/tests/testapi.cpp 2021-06-10 21:19:59 UTC (rev 278733)
+++ trunk/Source/_javascript_Core/API/tests/testapi.cpp 2021-06-10 21:54:36 UTC (rev 278734)
@@ -38,6 +38,10 @@
#include <wtf/Vector.h>
#include <wtf/text/StringCommon.h>
+#if PLATFORM(COCOA)
+#include <wtf/cocoa/RuntimeApplicationChecksCocoa.h>
+#endif
+
extern "C" void configureJSCForTesting();
extern "C" int testCAPIViaCpp(const char* filter);
extern "C" void JSSynchronousGarbageCollectForDebugging(JSContextRef);
@@ -147,6 +151,7 @@
void promiseUnhandledRejection();
void promiseUnhandledRejectionFromUnhandledRejectionCallback();
void promiseEarlyHandledRejections();
+ void promiseDrainDoesNotEatExceptions();
void topCallFrameAccess();
void markedJSValueArrayAndGC();
void classDefinitionWithJSSubclass();
@@ -609,6 +614,27 @@
check(!callbackCalled, "unhandled rejection callback should not be called for asynchronous early-handled rejection");
}
+void TestAPI::promiseDrainDoesNotEatExceptions()
+{
+#if PLATFORM(COCOA)
+ bool useLegacyDrain = false;
+#if PLATFORM(MAC)
+ useLegacyDrain = applicationSDKVersion() < DYLD_MACOSX_VERSION_12_00;
+#elif PLATFORM(WATCH)
+ // Don't check, JSC isn't API on watch anyway.
+#elif PLATFORM(IOS_FAMILY)
+ useLegacyDrain = applicationSDKVersion() < DYLD_IOS_VERSION_15_0;
+#else
+#error "Unsupported Cocoa Platform"
+#endif
+ if (useLegacyDrain)
+ return;
+#endif
+ ScriptResult result = callFunction("(function() { Promise.resolve().then(() => { throw 2; }); throw 1; })");
+ check(!result, "function should throw an error");
+ check(JSValueIsNumber(context, result.error()) && JSValueToNumber(context, result.error(), nullptr) == 1, "exception payload should have been 1");
+}
+
void TestAPI::topCallFrameAccess()
{
{
@@ -760,6 +786,7 @@
RUN(promiseRejectTrue());
RUN(promiseUnhandledRejection());
RUN(promiseUnhandledRejectionFromUnhandledRejectionCallback());
+ RUN(promiseDrainDoesNotEatExceptions());
RUN(promiseEarlyHandledRejections());
RUN(markedJSValueArrayAndGC());
RUN(classDefinitionWithJSSubclass());
@@ -766,10 +793,8 @@
RUN(proxyReturnedWithJSSubclassing());
RUN(testJSObjectSetOnGlobalObjectSubclassDefinition());
- if (tasks.isEmpty()) {
- dataLogLn("Filtered all tests: ERROR");
- return 1;
- }
+ if (tasks.isEmpty())
+ return 0;
Lock lock;
Modified: trunk/Source/_javascript_Core/API/tests/testapi.mm (278733 => 278734)
--- trunk/Source/_javascript_Core/API/tests/testapi.mm 2021-06-10 21:19:59 UTC (rev 278733)
+++ trunk/Source/_javascript_Core/API/tests/testapi.mm 2021-06-10 21:54:36 UTC (rev 278734)
@@ -2730,6 +2730,25 @@
}
}
+static void testMicrotaskWithFunction()
+{
+ @autoreleasepool {
+ JSContext *context = [[JSContext alloc] init];
+
+ JSValue *globalObject = context.globalObject;
+
+ auto block = ^() {
+ return 1+1;
+ };
+
+ [globalObject setValue:block forProperty:@"setTimeout"];
+ JSValue *arr = [context evaluateScript:@"var arr = []; (async () => { await 1; arr.push(3); })(); arr.push(1); setTimeout(); arr.push(2); arr;"];
+ checkResult(@"arr[0] should be 1", [arr[@0] toInt32] == 1);
+ checkResult(@"arr[1] should be 2", [arr[@1] toInt32] == 2);
+ checkResult(@"arr[2] should be 3", [arr[@2] toInt32] == 3);
+ }
+}
+
@protocol ToString <JSExport>
- (NSString *)toString;
@end
@@ -2848,6 +2867,8 @@
RUN(promiseCreateRejected());
RUN(parallelPromiseResolveTest());
+ RUN(testMicrotaskWithFunction());
+
if (!filter)
testObjectiveCAPIMain();
}
Modified: trunk/Source/_javascript_Core/ChangeLog (278733 => 278734)
--- trunk/Source/_javascript_Core/ChangeLog 2021-06-10 21:19:59 UTC (rev 278733)
+++ trunk/Source/_javascript_Core/ChangeLog 2021-06-10 21:54:36 UTC (rev 278734)
@@ -1,3 +1,38 @@
+2021-06-10 Keith Miller <keith_mil...@apple.com>
+
+ Shouldn't drain the micro task queue when calling out to ObjC
+ https://bugs.webkit.org/show_bug.cgi?id=161942
+
+ Reviewed by Saam Barati.
+
+ It looks like the issue is that we aren't checking for the
+ presence of dropped locks when deciding to drain microtasks during
+ JSLock::unlock. This meant that when we drop all locks when
+ calling out to API clients we would drain our microtasks at that
+ point. An alternative would be to pass an extra parameter to the
+ unlock function that says not to drain microtasks. I chose not to
+ do that since it seemed a bit less robust.
+
+ This patch is very likely to break existing API users. So I'm adding
+ a linked on or after check to protect existing Apps.
+
+ Lastly, change our Poker Bros check to use applicationSDKVersion too
+ so others trying to add a linked on or after check don't use
+ the dyld function directly too.
+
+ * API/tests/testapi.cpp:
+ (TestAPI::promiseDrainDoesNotEatExceptions):
+ (testCAPIViaCpp):
+ * API/tests/testapi.mm:
+ (testMicrotaskWithFunction):
+ (testObjectiveCAPI):
+ * runtime/JSLock.cpp:
+ (JSC::JSLock::willReleaseLock):
+ * runtime/ObjectPrototype.cpp:
+ (JSC::isPokerBros):
+ * runtime/VM.cpp:
+ (JSC::VM::didExhaustMicrotaskQueue):
+
2021-06-10 Mark Lam <mark....@apple.com>
Another speculative build fix for Win32.
Modified: trunk/Source/_javascript_Core/runtime/JSLock.cpp (278733 => 278734)
--- trunk/Source/_javascript_Core/runtime/JSLock.cpp 2021-06-10 21:19:59 UTC (rev 278733)
+++ trunk/Source/_javascript_Core/runtime/JSLock.cpp 2021-06-10 21:54:36 UTC (rev 278734)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2005-2018 Apple Inc. All rights reserved.
+ * Copyright (C) 2005-2021 Apple Inc. All rights reserved.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Library General Public
@@ -33,6 +33,10 @@
#include <wtf/ios/WebCoreThread.h>
#endif
+#if PLATFORM(COCOA)
+#include <wtf/cocoa/RuntimeApplicationChecksCocoa.h>
+#endif
+
namespace JSC {
Lock GlobalJSLock::s_sharedInstanceMutex;
@@ -197,8 +201,25 @@
{
RefPtr<VM> vm = m_vm;
if (vm) {
- vm->drainMicrotasks();
+ static bool useLegacyDrain = false;
+#if PLATFORM(COCOA)
+ static std::once_flag once;
+ std::call_once(once, [] {
+#if PLATFORM(MAC)
+ useLegacyDrain = applicationSDKVersion() < DYLD_MACOSX_VERSION_12_00;
+#elif PLATFORM(WATCH)
+ // Don't check, JSC isn't API on watch anyway.
+#elif PLATFORM(IOS_FAMILY)
+ useLegacyDrain = applicationSDKVersion() < DYLD_IOS_VERSION_15_0;
+#else
+#error "Unsupported Cocoa Platform"
+#endif
+ });
+#endif
+ if (!m_lockDropDepth || useLegacyDrain)
+ vm->drainMicrotasks();
+
if (!vm->topCallFrame)
vm->clearLastException();
Modified: trunk/Source/_javascript_Core/runtime/ObjectPrototype.cpp (278733 => 278734)
--- trunk/Source/_javascript_Core/runtime/ObjectPrototype.cpp 2021-06-10 21:19:59 UTC (rev 278733)
+++ trunk/Source/_javascript_Core/runtime/ObjectPrototype.cpp 2021-06-10 21:54:36 UTC (rev 278734)
@@ -29,7 +29,7 @@
#include "PropertySlot.h"
#if PLATFORM(IOS)
-#include <wtf/spi/darwin/dyldSPI.h>
+#include <wtf/cocoa/RuntimeApplicationChecksCocoa.h>
#endif
namespace JSC {
@@ -322,7 +322,7 @@
auto bundleID = CFBundleGetIdentifier(CFBundleGetMainBundle());
return bundleID
&& CFEqual(bundleID, CFSTR("com.kpgame.PokerBros"))
- && dyld_get_program_sdk_version() < DYLD_IOS_VERSION_14_0;
+ && applicationSDKVersion() < DYLD_IOS_VERSION_14_0;
}
#endif
Modified: trunk/Source/_javascript_Core/runtime/VM.cpp (278733 => 278734)
--- trunk/Source/_javascript_Core/runtime/VM.cpp 2021-06-10 21:19:59 UTC (rev 278733)
+++ trunk/Source/_javascript_Core/runtime/VM.cpp 2021-06-10 21:54:36 UTC (rev 278734)
@@ -1368,13 +1368,15 @@
void VM::didExhaustMicrotaskQueue()
{
- auto unhandledRejections = WTFMove(m_aboutToBeNotifiedRejectedPromises);
- for (auto& promise : unhandledRejections) {
- if (promise->isHandled(*this))
- continue;
+ do {
+ auto unhandledRejections = WTFMove(m_aboutToBeNotifiedRejectedPromises);
+ for (auto& promise : unhandledRejections) {
+ if (promise->isHandled(*this))
+ continue;
- callPromiseRejectionCallback(promise);
- }
+ callPromiseRejectionCallback(promise);
+ }
+ } while (!m_aboutToBeNotifiedRejectedPromises.isEmpty());
}
void VM::promiseRejected(JSPromise* promise)
