[JSC] JSLock should be WebThread aware
authorysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 19 Jun 2019 01:19:41 +0000 (01:19 +0000)
committerysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 19 Jun 2019 01:19:41 +0000 (01:19 +0000)
https://bugs.webkit.org/show_bug.cgi?id=198911

Reviewed by Geoffrey Garen.

Source/JavaScriptCore:

Since WebKitLegacy content rendering is done in WebThread instead of the main thread in iOS, user of WebKitLegacy (e.g. UIWebView) needs
to grab the WebThread lock (which is a recursive lock) in the main thread when touching the WebKitLegacy content.
But, WebKitLegacy can expose JSContext for the web view. And we can interact with the JS content through JavaScriptCore APIs. However,
since WebThread is a concept in WebCore, JavaScriptCore APIs do not grab the WebThread lock. As a result, WebKitLegacy web content can be
modified from the main thread without grabbing the WebThread lock through JavaScriptCore APIs.

This patch makes JSC aware of WebThread: JSLock grabs the WebThread lock before grabbing JS's lock. While this seems layering violation,
we already have many USE(WEB_THREAD) and WebThread aware code in WTF. Eventually, we should move WebThread code from WebCore to WTF since
JSC and WTF need to be aware of WebThread. But, for now, we just use the function pointer exposed by WebCore.

Since both JSLock and the WebThread lock are recursive locks, nested locking is totally OK. The possible problem is the order of locking.
We ensure that we always grab locks in (1) the WebThread lock and (2) JSLock order.

In JSLock, we take the WebThread lock, but we do not unlock it. This is how we use the WebThread lock: the WebThread lock is released
automatically when RunLoop finishes the current cycle, and in WebKitLegacy, we do not call unlocking function of the WebThread lock except
for some edge cases.

* API/JSVirtualMachine.mm:
(-[JSVirtualMachine isWebThreadAware]):
* API/JSVirtualMachineInternal.h:
* runtime/JSLock.cpp:
(JSC::JSLockHolder::JSLockHolder):
(JSC::JSLock::lock):
(JSC::JSLockHolder::init): Deleted.
* runtime/JSLock.h:
(JSC::JSLock::makeWebThreadAware):
(JSC::JSLock::isWebThreadAware const):

Source/WebCore:

* bindings/js/CommonVM.cpp:
(WebCore::commonVMSlow):

Tools:

* TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
* TestWebKitAPI/Tests/WebKitLegacy/ios/JSLockTakesWebThreadLock.mm: Added.
(TestWebKitAPI::TEST):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@246578 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/API/JSVirtualMachine.mm
Source/JavaScriptCore/API/JSVirtualMachineInternal.h
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/runtime/JSLock.cpp
Source/JavaScriptCore/runtime/JSLock.h
Source/WebCore/ChangeLog
Source/WebCore/bindings/js/CommonVM.cpp
Tools/ChangeLog
Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
Tools/TestWebKitAPI/Tests/WebKitLegacy/ios/JSLockTakesWebThreadLock.mm [new file with mode: 0644]

index 2afc097..62eef4d 100644 (file)
@@ -248,6 +248,11 @@ JSContextGroupRef getGroupFromVirtualMachine(JSVirtualMachine *virtualMachine)
     return *toJS(m_group);
 }
 
+- (BOOL)isWebThreadAware
+{
+    return [self vm].apiLock().isWebThreadAware();
+}
+
 + (void)setCrashOnVMCreation:(BOOL)shouldCrash
 {
     JSC::VM::setCrashOnVMCreation(shouldCrash);
index f64ee1a..ee86b61 100644 (file)
@@ -46,6 +46,8 @@ JSContextGroupRef getGroupFromVirtualMachine(JSVirtualMachine *);
 
 - (JSC::VM&)vm;
 
+- (BOOL)isWebThreadAware;
+
 @end
 
 #endif // defined(__OBJC__)
index 0272dfd..b73d4e4 100644 (file)
@@ -1,3 +1,38 @@
+2019-06-18  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC] JSLock should be WebThread aware
+        https://bugs.webkit.org/show_bug.cgi?id=198911
+
+        Reviewed by Geoffrey Garen.
+
+        Since WebKitLegacy content rendering is done in WebThread instead of the main thread in iOS, user of WebKitLegacy (e.g. UIWebView) needs
+        to grab the WebThread lock (which is a recursive lock) in the main thread when touching the WebKitLegacy content.
+        But, WebKitLegacy can expose JSContext for the web view. And we can interact with the JS content through JavaScriptCore APIs. However,
+        since WebThread is a concept in WebCore, JavaScriptCore APIs do not grab the WebThread lock. As a result, WebKitLegacy web content can be
+        modified from the main thread without grabbing the WebThread lock through JavaScriptCore APIs.
+
+        This patch makes JSC aware of WebThread: JSLock grabs the WebThread lock before grabbing JS's lock. While this seems layering violation,
+        we already have many USE(WEB_THREAD) and WebThread aware code in WTF. Eventually, we should move WebThread code from WebCore to WTF since
+        JSC and WTF need to be aware of WebThread. But, for now, we just use the function pointer exposed by WebCore.
+
+        Since both JSLock and the WebThread lock are recursive locks, nested locking is totally OK. The possible problem is the order of locking.
+        We ensure that we always grab locks in (1) the WebThread lock and (2) JSLock order.
+
+        In JSLock, we take the WebThread lock, but we do not unlock it. This is how we use the WebThread lock: the WebThread lock is released
+        automatically when RunLoop finishes the current cycle, and in WebKitLegacy, we do not call unlocking function of the WebThread lock except
+        for some edge cases.
+
+        * API/JSVirtualMachine.mm:
+        (-[JSVirtualMachine isWebThreadAware]):
+        * API/JSVirtualMachineInternal.h:
+        * runtime/JSLock.cpp:
+        (JSC::JSLockHolder::JSLockHolder):
+        (JSC::JSLock::lock):
+        (JSC::JSLockHolder::init): Deleted.
+        * runtime/JSLock.h:
+        (JSC::JSLock::makeWebThreadAware):
+        (JSC::JSLock::isWebThreadAware const):
+
 2019-06-18  Justin Michaud  <justin_michaud@apple.com>
 
         [WASM-References] Add support for Table.size, grow and fill instructions
index 7cc3f69..daf0f77 100644 (file)
 #include <wtf/Threading.h>
 #include <wtf/threads/Signals.h>
 
+#if USE(WEB_THREAD)
+#include <wtf/ios/WebCoreThread.h>
+#endif
+
 namespace JSC {
 
 Lock GlobalJSLock::s_sharedInstanceMutex;
@@ -50,25 +54,18 @@ GlobalJSLock::~GlobalJSLock()
 }
 
 JSLockHolder::JSLockHolder(ExecState* exec)
-    : m_vm(&exec->vm())
+    : JSLockHolder(exec->vm())
 {
-    init();
 }
 
 JSLockHolder::JSLockHolder(VM* vm)
-    : m_vm(vm)
+    : JSLockHolder(*vm)
 {
-    init();
 }
 
 JSLockHolder::JSLockHolder(VM& vm)
     : m_vm(&vm)
 {
-    init();
-}
-
-void JSLockHolder::init()
-{
     m_vm->apiLock().lock();
 }
 
@@ -105,6 +102,13 @@ void JSLock::lock()
 void JSLock::lock(intptr_t lockCount)
 {
     ASSERT(lockCount > 0);
+#if USE(WEB_THREAD)
+    if (m_isWebThreadAware) {
+        ASSERT(WebCoreWebThreadIsEnabled && WebCoreWebThreadIsEnabled());
+        WebCoreWebThreadLock();
+    }
+#endif
+
     bool success = m_lock.tryLock();
     if (UNLIKELY(!success)) {
         if (currentThreadIsHoldingLock()) {
index f461f82..0b27ff6 100644 (file)
@@ -70,9 +70,8 @@ public:
     JS_EXPORT_PRIVATE JSLockHolder(ExecState*);
 
     JS_EXPORT_PRIVATE ~JSLockHolder();
-private:
-    void init();
 
+private:
     RefPtr<VM> m_vm;
 };
 
@@ -119,6 +118,13 @@ public:
         unsigned m_dropDepth;
     };
 
+    void makeWebThreadAware()
+    {
+        m_isWebThreadAware = true;
+    }
+
+    bool isWebThreadAware() const { return m_isWebThreadAware; }
+
 private:
     void lock(intptr_t lockCount);
     void unlock(intptr_t unlockCount);
@@ -130,6 +136,7 @@ private:
     void grabAllLocks(DropAllLocks*, unsigned lockCount);
 
     Lock m_lock;
+    bool m_isWebThreadAware { false };
     // We cannot make m_ownerThread an optional (instead of pairing it with an explicit
     // m_hasOwnerThread) because currentThreadIsHoldingLock() may be called from a
     // different thread, and an optional is vulnerable to races.
index 4a7b7b7..dcfefd4 100644 (file)
@@ -1,3 +1,13 @@
+2019-06-18  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC] JSLock should be WebThread aware
+        https://bugs.webkit.org/show_bug.cgi?id=198911
+
+        Reviewed by Geoffrey Garen.
+
+        * bindings/js/CommonVM.cpp:
+        (WebCore::commonVMSlow):
+
 2019-06-18  Joseph Pecoraro  <pecoraro@apple.com>
 
         WebSocketDeflater uses an unnecessarily constrained compression memory level
index c1e94b8..09b784f 100644 (file)
@@ -59,6 +59,8 @@ JSC::VM& commonVMSlow()
     vm.heap.acquireAccess(); // At any time, we may do things that affect the GC.
 
 #if PLATFORM(IOS_FAMILY)
+    if (WebThreadIsEnabled())
+        vm.apiLock().makeWebThreadAware();
     vm.setRunLoop(WebThreadRunLoop());
     vm.heap.machineThreads().addCurrentThread();
 #endif
index 1a6b867..6c8bb2f 100644 (file)
@@ -1,3 +1,14 @@
+2019-06-18  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC] JSLock should be WebThread aware
+        https://bugs.webkit.org/show_bug.cgi?id=198911
+
+        Reviewed by Geoffrey Garen.
+
+        * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
+        * TestWebKitAPI/Tests/WebKitLegacy/ios/JSLockTakesWebThreadLock.mm: Added.
+        (TestWebKitAPI::TEST):
+
 2019-06-18  Keith Miller  <keith_miller@apple.com>
 
         webkit-patch should allow for a bugzilla url not just bugzilla id
index 864729a..9161863 100644 (file)
                E194E1BD177E53C7009C4D4E /* StopLoadingFromDidReceiveResponse.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = E194E1BC177E534A009C4D4E /* StopLoadingFromDidReceiveResponse.html */; };
                E324A6F02041C82000A76593 /* UniqueArray.cpp in Sources */ = {isa = PBXBuildFile; fileRef = E398BC0F2041C76300387136 /* UniqueArray.cpp */; };
                E32B549222810AC4008AD702 /* Packed.cpp in Sources */ = {isa = PBXBuildFile; fileRef = E32B549122810AC0008AD702 /* Packed.cpp */; };
+               E35FC7B222B82A7300F32F98 /* JSLockTakesWebThreadLock.mm in Sources */ = {isa = PBXBuildFile; fileRef = E35FC7B122B82A6D00F32F98 /* JSLockTakesWebThreadLock.mm */; };
                E373D7911F2CF35200C6FAAF /* Signals.cpp in Sources */ = {isa = PBXBuildFile; fileRef = E3953F951F2CF32100A76A2E /* Signals.cpp */; };
                E38A0D351FD50CC300E98C8B /* Threading.cpp in Sources */ = {isa = PBXBuildFile; fileRef = E38A0D341FD50CBC00E98C8B /* Threading.cpp */; };
                E3A1E77F21B25B39008C6007 /* URLParserTextEncoding.cpp in Sources */ = {isa = PBXBuildFile; fileRef = E3A1E77E21B25B39008C6007 /* URLParserTextEncoding.cpp */; };
                E194E1BC177E534A009C4D4E /* StopLoadingFromDidReceiveResponse.html */ = {isa = PBXFileReference; lastKnownFileType = text.html; path = StopLoadingFromDidReceiveResponse.html; sourceTree = "<group>"; };
                E19DB9781B32137C00DB38D4 /* NavigatorLanguage.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = NavigatorLanguage.mm; sourceTree = "<group>"; };
                E32B549122810AC0008AD702 /* Packed.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = Packed.cpp; sourceTree = "<group>"; };
+               E35FC7B122B82A6D00F32F98 /* JSLockTakesWebThreadLock.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = JSLockTakesWebThreadLock.mm; sourceTree = "<group>"; };
                E388887020C9098100E632BC /* WorkerPool.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = WorkerPool.cpp; sourceTree = "<group>"; };
                E38A0D341FD50CBC00E98C8B /* Threading.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = Threading.cpp; sourceTree = "<group>"; };
                E3953F951F2CF32100A76A2E /* Signals.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = Signals.cpp; sourceTree = "<group>"; };
                        children = (
                                CDC8E49A1BC728FE00594FEC /* Resources */,
                                CDC8E4851BC5B19400594FEC /* AudioSessionCategoryIOS.mm */,
+                               E35FC7B122B82A6D00F32F98 /* JSLockTakesWebThreadLock.mm */,
                                CDC0932A21C872C10030C4B0 /* ScrollingDoesNotPauseMedia.mm */,
                                0F4FFA9D1ED3AA8500F7111F /* SnapshotViaRenderInContext.mm */,
                        );
                                7CCE7EAD1A411A3400447C4C /* JavaScriptTest.cpp in Sources */,
                                7CCE7EA51A411A0800447C4C /* JavaScriptTestMac.mm in Sources */,
                                5C0160C121A132460077FA32 /* JITEnabled.mm in Sources */,
+                               E35FC7B222B82A7300F32F98 /* JSLockTakesWebThreadLock.mm in Sources */,
                                7CCE7EC41A411A7E00447C4C /* JSWrapperForNodeInWebFrame.mm in Sources */,
                                F45E15732112CE2900307E82 /* KeyboardInputTestsIOS.mm in Sources */,
                                7CCE7F061A411AE600447C4C /* LayoutMilestonesWithAllContentInFrame.cpp in Sources */,
diff --git a/Tools/TestWebKitAPI/Tests/WebKitLegacy/ios/JSLockTakesWebThreadLock.mm b/Tools/TestWebKitAPI/Tests/WebKitLegacy/ios/JSLockTakesWebThreadLock.mm
new file mode 100644 (file)
index 0000000..e1039f7
--- /dev/null
@@ -0,0 +1,64 @@
+/*
+ * Copyright (C) 2019 Apple Inc. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY
+ * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL APPLE COMPUTER, INC. OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
+ * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
+ * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
+ * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
+ * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#import "config.h"
+
+#if PLATFORM(IOS_FAMILY)
+
+#import "PlatformUtilities.h"
+#import <JavaScriptCore/JSVirtualMachine.h>
+#import <JavaScriptCore/JSVirtualMachineInternal.h>
+#import <UIKit/UIKit.h>
+#import <WebCore/WebCoreThread.h>
+#import <stdlib.h>
+#import <wtf/DataLog.h>
+#import <wtf/RetainPtr.h>
+
+namespace TestWebKitAPI {
+
+TEST(WebKitLegacy, JSLockTakesWebThreadLock)
+{
+    RetainPtr<UIWindow> uiWindow = adoptNS([[UIWindow alloc] initWithFrame:NSMakeRect(0, 0, 800, 600)]);
+    RetainPtr<UIWebView> uiWebView = adoptNS([[UIWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600)]);
+    [uiWindow addSubview:uiWebView.get()];
+    [uiWebView loadHTMLString:@"<p>WebThreadLock and JSLock</p>" baseURL:nil];
+    RetainPtr<JSContext> jsContext = [uiWebView valueForKeyPath:@"documentView.webView.mainFrame.javaScriptContext"];
+
+    EXPECT_TRUE(!!jsContext.get());
+
+    RetainPtr<JSVirtualMachine> jsVM = [jsContext virtualMachine];
+    EXPECT_TRUE([jsVM isWebThreadAware]);
+
+    // Release WebThread lock.
+    Util::spinRunLoop(1);
+
+    EXPECT_FALSE(WebThreadIsLocked());
+    // XMLHttpRequest has Timer, which has thread afinity.
+    [jsContext evaluateScript:@"for (var i = 0; i < 1e3; ++i) { var request = new XMLHttpRequest; request.property = new Array(10000); }"];
+}
+
+}
+
+#endif