Add threading assertions to RefCounted
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 12 Aug 2019 18:10:26 +0000 (18:10 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 12 Aug 2019 18:10:26 +0000 (18:10 +0000)
https://bugs.webkit.org/show_bug.cgi?id=200507

Reviewed by Ryosuke Niwa.

Source/JavaScriptCore:

* dfg/DFGPlan.cpp:
(JSC::DFG::Plan::Plan):
Disable threading assertions for DFG::Plan::m_inlineCallFrames while the JSC team
investigates.

Source/WebKit:

Enable new RefCounted threading assertions for WebKit2
(UIProcess + auxiliary processes).

* Shared/AuxiliaryProcess.cpp:
(WebKit::AuxiliaryProcess::initialize):
* Shared/Cocoa/WebKit2InitializeCocoa.mm:
(WebKit::runInitializationCode):
* Shared/WebKit2Initialize.cpp:
(WebKit::InitializeWebKit2):

Source/WebKitLegacy/mac:

* WebView/WebView.mm:
(+[WebView initialize]):
Enable new RefCounted threading assertions for WebKitLegacy.

Source/WTF:

Add threading assertions to RefCounted to try and catch unsafe concurrent ref'ing / derefing of
RefCounted objects from several threads. If you hit these new assertions, it likely means you either
need to:
1. Have your class subclass ThreadSafeRefCounted instead of RefCounted
or
2. Make sure your objects always gets ref'd / deref'd from the same thread.

These assertions already found several thread safety bugs in our code base, which I fixed via
dependency bugs.

These assertions are currently enabled in WebKit (UIProcess, child processes and
WebKitLegacy), they do not apply other JavascriptCore API clients.

* WTF.xcodeproj/project.pbxproj:
* wtf/CMakeLists.txt:
* wtf/RefCounted.cpp: Added.
* wtf/RefCounted.h:
(WTF::RefCountedBase::ref const):
(WTF::RefCountedBase::disableThreadingChecks):
(WTF::RefCountedBase::enableThreadingChecksGlobally):
(WTF::RefCountedBase::RefCountedBase):
(WTF::RefCountedBase::areThreadingCheckedEnabled const):
(WTF::RefCountedBase::derefBase const):
* wtf/SizeLimits.cpp:

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

16 files changed:
Source/JavaScriptCore/API/JSContextRef.cpp
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/b3/testb3_1.cpp
Source/JavaScriptCore/dfg/DFGPlan.cpp
Source/WTF/ChangeLog
Source/WTF/WTF.xcodeproj/project.pbxproj
Source/WTF/wtf/CMakeLists.txt
Source/WTF/wtf/RefCounted.cpp [new file with mode: 0644]
Source/WTF/wtf/RefCounted.h
Source/WTF/wtf/SizeLimits.cpp
Source/WebKit/ChangeLog
Source/WebKit/Shared/AuxiliaryProcess.cpp
Source/WebKit/Shared/Cocoa/WebKit2InitializeCocoa.mm
Source/WebKit/Shared/WebKit2Initialize.cpp
Source/WebKitLegacy/mac/ChangeLog
Source/WebKitLegacy/mac/WebView/WebView.mm

index 9f5fdaf..771bf52 100644 (file)
@@ -66,6 +66,7 @@ using namespace JSC;
 
 JSContextGroupRef JSContextGroupCreate()
 {
+    WTF::initializeMainThread();
     initializeThreading();
     return toRef(&VM::createContextGroup().leakRef());
 }
@@ -116,6 +117,7 @@ void JSContextGroupClearExecutionTimeLimit(JSContextGroupRef group)
 
 JSGlobalContextRef JSGlobalContextCreate(JSClassRef globalObjectClass)
 {
+    WTF::initializeMainThread();
     initializeThreading();
 
 #if OS(DARWIN)
@@ -131,6 +133,7 @@ JSGlobalContextRef JSGlobalContextCreate(JSClassRef globalObjectClass)
 
 JSGlobalContextRef JSGlobalContextCreateInGroup(JSContextGroupRef group, JSClassRef globalObjectClass)
 {
+    WTF::initializeMainThread();
     initializeThreading();
 
     Ref<VM> vm = group ? Ref<VM>(*toJS(group)) : VM::createContextGroup();
index 4d737fb..a7b4b81 100644 (file)
@@ -1,5 +1,17 @@
 2019-08-12  Chris Dumez  <cdumez@apple.com>
 
+        Add threading assertions to RefCounted
+        https://bugs.webkit.org/show_bug.cgi?id=200507
+
+        Reviewed by Ryosuke Niwa.
+
+        * dfg/DFGPlan.cpp:
+        (JSC::DFG::Plan::Plan):
+        Disable threading assertions for DFG::Plan::m_inlineCallFrames while the JSC team
+        investigates.
+
+2019-08-12  Chris Dumez  <cdumez@apple.com>
+
         Unreviewed, rolling out r248525.
 
         Revert new threading assertions while I work on fixing the
index c7699fc..af2adae 100644 (file)
@@ -902,6 +902,7 @@ int main(int argc, char** argv)
         break;
     }
 
+    WTF::initializeMainThread();
     JSC::initializeThreading();
     
     for (unsigned i = 0; i <= 2; ++i) {
index e413ea4..4dbcee8 100644 (file)
@@ -150,6 +150,7 @@ Plan::Plan(CodeBlock* passedCodeBlock, CodeBlock* profiledDFGCodeBlock,
     , m_stage(Preparing)
 {
     RELEASE_ASSERT(m_codeBlock->alternative()->jitCode());
+    m_inlineCallFrames->disableThreadingChecks();
 }
 
 Plan::~Plan()
index 6ecdd23..ea4a1af 100644 (file)
@@ -1,5 +1,37 @@
 2019-08-12  Chris Dumez  <cdumez@apple.com>
 
+        Add threading assertions to RefCounted
+        https://bugs.webkit.org/show_bug.cgi?id=200507
+
+        Reviewed by Ryosuke Niwa.
+
+        Add threading assertions to RefCounted to try and catch unsafe concurrent ref'ing / derefing of
+        RefCounted objects from several threads. If you hit these new assertions, it likely means you either
+        need to:
+        1. Have your class subclass ThreadSafeRefCounted instead of RefCounted
+        or
+        2. Make sure your objects always gets ref'd / deref'd from the same thread.
+
+        These assertions already found several thread safety bugs in our code base, which I fixed via
+        dependency bugs.
+
+        These assertions are currently enabled in WebKit (UIProcess, child processes and
+        WebKitLegacy), they do not apply other JavascriptCore API clients.
+
+        * WTF.xcodeproj/project.pbxproj:
+        * wtf/CMakeLists.txt:
+        * wtf/RefCounted.cpp: Added.
+        * wtf/RefCounted.h:
+        (WTF::RefCountedBase::ref const):
+        (WTF::RefCountedBase::disableThreadingChecks):
+        (WTF::RefCountedBase::enableThreadingChecksGlobally):
+        (WTF::RefCountedBase::RefCountedBase):
+        (WTF::RefCountedBase::areThreadingCheckedEnabled const):
+        (WTF::RefCountedBase::derefBase const):
+        * wtf/SizeLimits.cpp:
+
+2019-08-12  Chris Dumez  <cdumez@apple.com>
+
         Unreviewed, rolling out r248525.
 
         Revert new threading assertions while I work on fixing the
index 9ef5683..b973150 100644 (file)
@@ -61,6 +61,7 @@
                2CDED0F318115C85004DBA70 /* RunLoop.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 2CDED0F118115C85004DBA70 /* RunLoop.cpp */; };
                3337DB9CE743410FAF076E17 /* StackTrace.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 313EDEC9778E49C9BEA91CFC /* StackTrace.cpp */; };
                4427C5AA21F6D6C300A612A4 /* ASCIICType.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 4427C5A921F6D6C300A612A4 /* ASCIICType.cpp */; };
+               46BEB6EB22FFE24900269867 /* RefCounted.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 46BEB6E922FFDDD500269867 /* RefCounted.cpp */; };
                50DE35F5215BB01500B979C7 /* ExternalStringImpl.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 50DE35F3215BB01500B979C7 /* ExternalStringImpl.cpp */; };
                515F794E1CFC9F4A00CCED93 /* CrossThreadCopier.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 515F794B1CFC9F4A00CCED93 /* CrossThreadCopier.cpp */; };
                517F82D71FD22F3000DA3DEA /* CrossThreadTaskHandler.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 517F82D51FD22F2F00DA3DEA /* CrossThreadTaskHandler.cpp */; };
                430B47871AAAAC1A001223DA /* StringCommon.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = StringCommon.h; sourceTree = "<group>"; };
                4427C5A921F6D6C300A612A4 /* ASCIICType.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = ASCIICType.cpp; sourceTree = "<group>"; };
                46BA9EAB1F4CD61E009A2BBC /* CompletionHandler.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = CompletionHandler.h; sourceTree = "<group>"; };
+               46BEB6E922FFDDD500269867 /* RefCounted.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = RefCounted.cpp; sourceTree = "<group>"; };
                50DE35F3215BB01500B979C7 /* ExternalStringImpl.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = ExternalStringImpl.cpp; sourceTree = "<group>"; };
                50DE35F4215BB01500B979C7 /* ExternalStringImpl.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ExternalStringImpl.h; sourceTree = "<group>"; };
                513E170A1CD7D5BF00E3650B /* LoggingAccumulator.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = LoggingAccumulator.h; sourceTree = "<group>"; };
                                0FDE87F61DFD07CC0064C390 /* RecursiveLockAdapter.h */,
                                A8A472FE151A825B004123FF /* RedBlackTree.h */,
                                26299B6D17A9E5B800ADEBE5 /* Ref.h */,
+                               46BEB6E922FFDDD500269867 /* RefCounted.cpp */,
                                A8A472FF151A825B004123FF /* RefCounted.h */,
                                A8A47300151A825B004123FF /* RefCountedArray.h */,
                                A8A47301151A825B004123FF /* RefCountedLeakCounter.cpp */,
                                A3B725EC987446AD93F1A440 /* RandomDevice.cpp in Sources */,
                                A8A47414151A825B004123FF /* RandomNumber.cpp in Sources */,
                                0FEC3C5E1F368A9700F59B6C /* ReadWriteLock.cpp in Sources */,
+                               46BEB6EB22FFE24900269867 /* RefCounted.cpp in Sources */,
                                A8A4741A151A825B004123FF /* RefCountedLeakCounter.cpp in Sources */,
                                E392FA2722E92BFF00ECDC73 /* ResourceUsageCocoa.cpp in Sources */,
                                2CDED0F318115C85004DBA70 /* RunLoop.cpp in Sources */,
index a32e7e2..a16baa7 100644 (file)
@@ -393,6 +393,7 @@ set(WTF_SOURCES
     RandomDevice.cpp
     RandomNumber.cpp
     ReadWriteLock.cpp
+    RefCounted.cpp
     RefCountedLeakCounter.cpp
     RunLoop.cpp
     SHA1.cpp
diff --git a/Source/WTF/wtf/RefCounted.cpp b/Source/WTF/wtf/RefCounted.cpp
new file mode 100644 (file)
index 0000000..94ee959
--- /dev/null
@@ -0,0 +1,30 @@
+/*
+ * Copyright (C) 2019 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
+ * License as published by the Free Software Foundation; either
+ * version 2 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Library General Public License for more details.
+ *
+ * You should have received a copy of the GNU Library General Public License
+ * along with this library; see the file COPYING.LIB.  If not, write to
+ * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
+ * Boston, MA 02110-1301, USA.
+ *
+ */
+
+#include "config.h"
+#include <wtf/RefCounted.h>
+
+namespace WTF {
+
+#if !ASSERT_DISABLED
+bool RefCountedBase::areThreadingChecksEnabledGlobally { false };
+#endif
+
+}
index ea9782a..cbb8d8b 100644 (file)
@@ -22,6 +22,7 @@
 
 #include <wtf/Assertions.h>
 #include <wtf/FastMalloc.h>
+#include <wtf/MainThread.h>
 #include <wtf/Noncopyable.h>
 
 namespace WTF {
@@ -39,6 +40,16 @@ class RefCountedBase {
 public:
     void ref() const
     {
+#if !ASSERT_DISABLED
+        if (m_isOwnedByMainThread != isMainThread() && hasOneRef())
+            m_isOwnedByMainThread = isMainThread(); // Likely ownership transfer.
+
+        // If you hit this assertion, it means that the RefCounted object was ref'd or deref'd
+        // concurrent from several threads, which is not safe. You should either subclass
+        // ThreadSafeRefCounted instead, or make sure to always ref / deref from the same thread.
+        ASSERT_WITH_MESSAGE(!areThreadingCheckedEnabled() || m_isOwnedByMainThread == isMainThread(), "Should not be ref'd / deref'd concurrently from several threads");
+#endif
+
 #if CHECK_REF_COUNTED_LIFECYCLE
         ASSERT_WITH_SECURITY_IMPLICATION(!m_deletionHasBegun);
         ASSERT(!m_adoptionIsRequired);
@@ -68,9 +79,28 @@ public:
 #endif
     }
 
+    // Please only call this method if you really know that what you're doing is safe (e.g.
+    // locking at call sites).
+    void disableThreadingChecks()
+    {
+#if !ASSERT_DISABLED
+        m_areThreadingChecksEnabled = false;
+#endif
+    }
+
+    static void enableThreadingChecksGlobally()
+    {
+#if !ASSERT_DISABLED
+        areThreadingChecksEnabledGlobally = true;
+#endif
+    }
+
 protected:
     RefCountedBase()
         : m_refCount(1)
+#if !ASSERT_DISABLED
+        , m_isOwnedByMainThread(isMainThread())
+#endif
 #if CHECK_REF_COUNTED_LIFECYCLE
         , m_deletionHasBegun(false)
         , m_adoptionIsRequired(true)
@@ -78,6 +108,13 @@ protected:
     {
     }
 
+#if !ASSERT_DISABLED
+    bool areThreadingCheckedEnabled() const
+    {
+        return areThreadingChecksEnabledGlobally && m_areThreadingChecksEnabled;
+    }
+#endif
+
     ~RefCountedBase()
     {
 #if CHECK_REF_COUNTED_LIFECYCLE
@@ -89,6 +126,16 @@ protected:
     // Returns whether the pointer should be freed or not.
     bool derefBase() const
     {
+#if !ASSERT_DISABLED
+        if (m_isOwnedByMainThread != isMainThread() && hasOneRef())
+            m_isOwnedByMainThread = isMainThread(); // Likely ownership transfer.
+
+        // If you hit this assertion, it means that the RefCounted object was ref'd or deref'd
+        // concurrent from several threads, which is not safe. You should either subclass
+        // ThreadSafeRefCounted instead, or make sure to always ref / deref from the same thread.
+        ASSERT_WITH_MESSAGE(!areThreadingCheckedEnabled() || m_isOwnedByMainThread == isMainThread(), "Should not be ref'd / deref'd concurrently from several threads");
+#endif
+
 #if CHECK_REF_COUNTED_LIFECYCLE
         ASSERT_WITH_SECURITY_IMPLICATION(!m_deletionHasBegun);
         ASSERT(!m_adoptionIsRequired);
@@ -120,6 +167,11 @@ private:
 #endif
 
     mutable unsigned m_refCount;
+#if !ASSERT_DISABLED
+    mutable bool m_isOwnedByMainThread;
+    bool m_areThreadingChecksEnabled { true };
+    WTF_EXPORT_PRIVATE static bool areThreadingChecksEnabledGlobally;
+#endif
 #if CHECK_REF_COUNTED_LIFECYCLE
     mutable bool m_deletionHasBegun;
     mutable bool m_adoptionIsRequired;
index b0ad140..67fa62e 100644 (file)
@@ -45,6 +45,8 @@ struct SameSizeAsRefCounted {
     int a;
     bool b;
     bool c;
+    bool d;
+    bool e;
     // The debug version may get bigger.
 };
 #else
index efc068a..177c51b 100644 (file)
@@ -1,5 +1,22 @@
 2019-08-12  Chris Dumez  <cdumez@apple.com>
 
+        Add threading assertions to RefCounted
+        https://bugs.webkit.org/show_bug.cgi?id=200507
+
+        Reviewed by Ryosuke Niwa.
+
+        Enable new RefCounted threading assertions for WebKit2
+        (UIProcess + auxiliary processes).
+
+        * Shared/AuxiliaryProcess.cpp:
+        (WebKit::AuxiliaryProcess::initialize):
+        * Shared/Cocoa/WebKit2InitializeCocoa.mm:
+        (WebKit::runInitializationCode):
+        * Shared/WebKit2Initialize.cpp:
+        (WebKit::InitializeWebKit2):
+
+2019-08-12  Chris Dumez  <cdumez@apple.com>
+
         Unreviewed, rolling out r248525.
 
         Revert new threading assertions while I work on fixing the
index 067f708..61272e2 100644 (file)
@@ -60,6 +60,8 @@ void AuxiliaryProcess::didClose(IPC::Connection&)
 
 void AuxiliaryProcess::initialize(const AuxiliaryProcessInitializationParameters& parameters)
 {
+    WTF::RefCountedBase::enableThreadingChecksGlobally();
+
     RELEASE_ASSERT_WITH_MESSAGE(parameters.processIdentifier, "Unable to initialize child process without a WebCore process identifier");
     Process::setIdentifier(*parameters.processIdentifier);
 
index f19ea28..1384103 100644 (file)
@@ -31,6 +31,7 @@
 #import <WebCore/LogInitialization.h>
 #import <mutex>
 #import <wtf/MainThread.h>
+#import <wtf/RefCounted.h>
 #import <wtf/RunLoop.h>
 
 #if PLATFORM(IOS_FAMILY)
@@ -50,6 +51,8 @@ static void runInitializationCode(void* = nullptr)
     JSC::initializeThreading();
     RunLoop::initializeMainRunLoop();
 
+    WTF::RefCountedBase::enableThreadingChecksGlobally();
+
 #if !LOG_DISABLED || !RELEASE_LOG_DISABLED
     WebCore::initializeLogChannelsIfNecessary();
     WebKit::initializeLogChannelsIfNecessary();
index c855b1f..083730d 100644 (file)
@@ -30,6 +30,7 @@
 #include <JavaScriptCore/InitializeThreading.h>
 #include <WebCore/LogInitialization.h>
 #include <wtf/MainThread.h>
+#include <wtf/RefCounted.h>
 #include <wtf/RunLoop.h>
 
 namespace WebKit {
@@ -41,6 +42,8 @@ void InitializeWebKit2()
     JSC::initializeThreading();
     RunLoop::initializeMainRunLoop();
 
+    WTF::RefCountedBase::enableThreadingChecksGlobally();
+
 #if !LOG_DISABLED || !RELEASE_LOG_DISABLED
     WebCore::initializeLogChannelsIfNecessary();
     WebKit::initializeLogChannelsIfNecessary();
index 3764dce..6855356 100644 (file)
@@ -1,5 +1,16 @@
 2019-08-12  Chris Dumez  <cdumez@apple.com>
 
+        Add threading assertions to RefCounted
+        https://bugs.webkit.org/show_bug.cgi?id=200507
+
+        Reviewed by Ryosuke Niwa.
+
+        * WebView/WebView.mm:
+        (+[WebView initialize]):
+        Enable new RefCounted threading assertions for WebKitLegacy.
+
+2019-08-12  Chris Dumez  <cdumez@apple.com>
+
         Unreviewed, rolling out r248525.
 
         Revert new threading assertions while I work on fixing the
index 54bcd10..bccc64b 100644 (file)
@@ -5416,6 +5416,8 @@ static Vector<String> toStringVector(NSArray* patterns)
     RunLoop::initializeMainRunLoop();
 #endif
 
+    WTF::RefCountedBase::enableThreadingChecksGlobally();
+
     WTF::setProcessPrivileges(allPrivileges());
     WebCore::NetworkStorageSession::permitProcessToUseCookieAPI(true);