Attachment 372331 Details for Bug 198911 – Patch

[patch] Patch

bug-198911-20190618003904.patch (text/plain), 14.79 KB, created by Yusuke Suzuki on 2019-06-18 00:39:04 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2019-06-18 00:39:04 PDT

Size: 14.79 KB

patch

obsolete

>Subversion Revision: 246532
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index fd0e8bfabf8df96a9c4eb9e42fc3af10d2415109..11b1095daedfdf67b976e48342a3f95959568f3b 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -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 NOBODY (OOPS!).
>+
>+        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-17  Justin Michaud  <justin_michaud@apple.com>
> 
>         Validate that table element type is funcref if using an element section
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 61548b320a7c9508f93d4bae785b46e4ae698e61..a7f6cca47b5ed2198747670b0670a580e573dfbf 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -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 NOBODY (OOPS!).
>+
>+        * bindings/js/CommonVM.cpp:
>+        (WebCore::commonVMSlow):
>+
> 2019-06-17  Sihui Liu  <sihui_liu@apple.com>
> 
>         -[WKWebsiteDataStore removeDataOfTypes:modifiedSince:completionHandler:] doesn't delete _WKWebsiteDataTypeCredentials
>diff --git a/Source/JavaScriptCore/API/JSVirtualMachine.mm b/Source/JavaScriptCore/API/JSVirtualMachine.mm
>index 2afc097b2facd931a447e3be30fb37aadee7f850..62eef4d4892c3d4f09b6323a4f0f2ff8c5c946b6 100644
>--- a/Source/JavaScriptCore/API/JSVirtualMachine.mm
>+++ b/Source/JavaScriptCore/API/JSVirtualMachine.mm
>@@ -248,6 +248,11 @@ - (JSC::VM&)vm
>     return *toJS(m_group);
> }
> 
>+- (BOOL)isWebThreadAware
>+{
>+    return [self vm].apiLock().isWebThreadAware();
>+}
>+
> + (void)setCrashOnVMCreation:(BOOL)shouldCrash
> {
>     JSC::VM::setCrashOnVMCreation(shouldCrash);
>diff --git a/Source/JavaScriptCore/API/JSVirtualMachineInternal.h b/Source/JavaScriptCore/API/JSVirtualMachineInternal.h
>index f64ee1ae028a696a81c58e36c36d450e6c0dbc98..ee86b615339ff67cbc6e3cbf3141d915cbec2606 100644
>--- a/Source/JavaScriptCore/API/JSVirtualMachineInternal.h
>+++ b/Source/JavaScriptCore/API/JSVirtualMachineInternal.h
>@@ -46,6 +46,8 @@ JSContextGroupRef getGroupFromVirtualMachine(JSVirtualMachine *);
> 
> - (JSC::VM&)vm;
> 
>+- (BOOL)isWebThreadAware;
>+
> @end
> 
> #endif // defined(__OBJC__)
>diff --git a/Source/JavaScriptCore/runtime/JSLock.cpp b/Source/JavaScriptCore/runtime/JSLock.cpp
>index 7cc3f69d4afc2e7d52ebcb9739c81d297255c52c..daf0f77d56ee1327ffdeaac0d808e457cfe86310 100644
>--- a/Source/JavaScriptCore/runtime/JSLock.cpp
>+++ b/Source/JavaScriptCore/runtime/JSLock.cpp
>@@ -35,6 +35,10 @@
> #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,24 +54,17 @@ 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()) {
>diff --git a/Source/JavaScriptCore/runtime/JSLock.h b/Source/JavaScriptCore/runtime/JSLock.h
>index f461f820f9e3aa5f1ec86a02113da2296f3330e8..0b27ff63fe4815bf263fc66f789710c32b0c2506 100644
>--- a/Source/JavaScriptCore/runtime/JSLock.h
>+++ b/Source/JavaScriptCore/runtime/JSLock.h
>@@ -70,9 +70,8 @@ class JSLockHolder {
>     JS_EXPORT_PRIVATE JSLockHolder(ExecState*);
> 
>     JS_EXPORT_PRIVATE ~JSLockHolder();
>-private:
>-    void init();
> 
>+private:
>     RefPtr<VM> m_vm;
> };
> 
>@@ -119,6 +118,13 @@ class JSLock : public ThreadSafeRefCounted<JSLock> {
>         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 @@ class JSLock : public ThreadSafeRefCounted<JSLock> {
>     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.
>diff --git a/Source/WebCore/bindings/js/CommonVM.cpp b/Source/WebCore/bindings/js/CommonVM.cpp
>index c1e94b8b85726b3d51fe11958992351bf37e1b52..09b784f9ed356ad6bcba84f5e63bf8a6f1ea267d 100644
>--- a/Source/WebCore/bindings/js/CommonVM.cpp
>+++ b/Source/WebCore/bindings/js/CommonVM.cpp
>@@ -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
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index f5f940b90a2aaaceb96325e8ca4b9555cb731255..07f4849f36d4162a7f6fc9a28bcd291d74f6e5d0 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -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 NOBODY (OOPS!).
>+
>+        * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
>+        * TestWebKitAPI/Tests/WebKitLegacy/ios/JSLockTakesWebThreadLock.mm: Added.
>+        (TestWebKitAPI::TEST):
>+
> 2019-06-17  Sihui Liu  <sihui_liu@apple.com>
> 
>         -[WKWebsiteDataStore removeDataOfTypes:modifiedSince:completionHandler:] doesn't delete _WKWebsiteDataTypeCredentials
>diff --git a/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj b/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
>index 864729a99db43355650ba417c99f468d32149750..916186310cd1bd845ef537b10474d4967d07ff1f 100644
>--- a/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
>+++ b/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
>@@ -875,6 +875,7 @@
> 		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 */; };
>@@ -2272,6 +2273,7 @@
> 		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>"; };
>@@ -3807,6 +3809,7 @@
> 			children = (
> 				CDC8E49A1BC728FE00594FEC /* Resources */,
> 				CDC8E4851BC5B19400594FEC /* AudioSessionCategoryIOS.mm */,
>+				E35FC7B122B82A6D00F32F98 /* JSLockTakesWebThreadLock.mm */,
> 				CDC0932A21C872C10030C4B0 /* ScrollingDoesNotPauseMedia.mm */,
> 				0F4FFA9D1ED3AA8500F7111F /* SnapshotViaRenderInContext.mm */,
> 			);
>@@ -4303,6 +4306,7 @@
> 				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
>index 0000000000000000000000000000000000000000..e1039f7768c2b75d3c75f208a7954d7fb806aa17
>--- /dev/null
>+++ b/Tools/TestWebKitAPI/Tests/WebKitLegacy/ios/JSLockTakesWebThreadLock.mm
>@@ -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

Flags:

ggaren: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 198911: 372236 | 372237 | 372261 | 372303 | 372319 | 372329 | 372331