Changeset 248525 in webkit


Ignore:
Timestamp:
Aug 11, 2019 7:00:30 PM (5 years ago)
Author:
Chris Dumez
Message:

Add threading assertions to RefCounted
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

  1. 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:
Location:
trunk/Source
Files:
1 added
13 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/JavaScriptCore/ChangeLog

    r248494 r248525  
     12019-08-11  Chris Dumez  <cdumez@apple.com>
     2
     3        Add threading assertions to RefCounted
     4        https://bugs.webkit.org/show_bug.cgi?id=200507
     5
     6        Reviewed by Ryosuke Niwa.
     7
     8        * dfg/DFGPlan.cpp:
     9        (JSC::DFG::Plan::Plan):
     10        Disable threading assertions for DFG::Plan::m_inlineCallFrames while the JSC team
     11        investigates.
     12
    1132019-08-09  Yusuke Suzuki  <ysuzuki@apple.com>
    214
  • trunk/Source/JavaScriptCore/dfg/DFGPlan.cpp

    r248027 r248525  
    151151{
    152152    RELEASE_ASSERT(m_codeBlock->alternative()->jitCode());
     153    m_inlineCallFrames->disableThreadingChecks();
    153154}
    154155
  • trunk/Source/WTF/ChangeLog

    r248488 r248525  
     12019-08-11  Chris Dumez  <cdumez@apple.com>
     2
     3        Add threading assertions to RefCounted
     4        https://bugs.webkit.org/show_bug.cgi?id=200507
     5
     6        Reviewed by Ryosuke Niwa.
     7
     8        Add threading assertions to RefCounted to try and catch unsafe concurrent ref'ing / derefing of
     9        RefCounted objects from several threads. If you hit these new assertions, it likely means you either
     10        need to:
     11        1. Have your class subclass ThreadSafeRefCounted instead of RefCounted
     12        or
     13        2. Make sure your objects always gets ref'd / deref'd from the same thread.
     14
     15        These assertions already found several thread safety bugs in our code base, which I fixed via
     16        dependency bugs.
     17
     18        These assertions are currently enabled in WebKit (UIProcess, child processes and
     19        WebKitLegacy), they do not apply other JavascriptCore API clients.
     20
     21        * WTF.xcodeproj/project.pbxproj:
     22        * wtf/CMakeLists.txt:
     23        * wtf/RefCounted.cpp: Added.
     24        * wtf/RefCounted.h:
     25        (WTF::RefCountedBase::ref const):
     26        (WTF::RefCountedBase::disableThreadingChecks):
     27        (WTF::RefCountedBase::enableThreadingChecksGlobally):
     28        (WTF::RefCountedBase::RefCountedBase):
     29        (WTF::RefCountedBase::areThreadingCheckedEnabled const):
     30        (WTF::RefCountedBase::derefBase const):
     31        * wtf/SizeLimits.cpp:
     32
    1332019-08-09  Saam Barati  <sbarati@apple.com>
    234
  • trunk/Source/WTF/WTF.xcodeproj/project.pbxproj

    r248441 r248525  
    6262                3337DB9CE743410FAF076E17 /* StackTrace.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 313EDEC9778E49C9BEA91CFC /* StackTrace.cpp */; };
    6363                4427C5AA21F6D6C300A612A4 /* ASCIICType.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 4427C5A921F6D6C300A612A4 /* ASCIICType.cpp */; };
     64                46BEB6EB22FFE24900269867 /* RefCounted.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 46BEB6E922FFDDD500269867 /* RefCounted.cpp */; };
    6465                50DE35F5215BB01500B979C7 /* ExternalStringImpl.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 50DE35F3215BB01500B979C7 /* ExternalStringImpl.cpp */; };
    6566                515F794E1CFC9F4A00CCED93 /* CrossThreadCopier.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 515F794B1CFC9F4A00CCED93 /* CrossThreadCopier.cpp */; };
     
    345346                4427C5A921F6D6C300A612A4 /* ASCIICType.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = ASCIICType.cpp; sourceTree = "<group>"; };
    346347                46BA9EAB1F4CD61E009A2BBC /* CompletionHandler.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = CompletionHandler.h; sourceTree = "<group>"; };
     348                46BEB6E922FFDDD500269867 /* RefCounted.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = RefCounted.cpp; sourceTree = "<group>"; };
    347349                50DE35F3215BB01500B979C7 /* ExternalStringImpl.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = ExternalStringImpl.cpp; sourceTree = "<group>"; };
    348350                50DE35F4215BB01500B979C7 /* ExternalStringImpl.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = ExternalStringImpl.h; sourceTree = "<group>"; };
     
    11081110                                A8A472FE151A825B004123FF /* RedBlackTree.h */,
    11091111                                26299B6D17A9E5B800ADEBE5 /* Ref.h */,
     1112                                46BEB6E922FFDDD500269867 /* RefCounted.cpp */,
    11101113                                A8A472FF151A825B004123FF /* RefCounted.h */,
    11111114                                A8A47300151A825B004123FF /* RefCountedArray.h */,
     
    16011604                                A8A47414151A825B004123FF /* RandomNumber.cpp in Sources */,
    16021605                                0FEC3C5E1F368A9700F59B6C /* ReadWriteLock.cpp in Sources */,
     1606                                46BEB6EB22FFE24900269867 /* RefCounted.cpp in Sources */,
    16031607                                A8A4741A151A825B004123FF /* RefCountedLeakCounter.cpp in Sources */,
    16041608                                E392FA2722E92BFF00ECDC73 /* ResourceUsageCocoa.cpp in Sources */,
  • trunk/Source/WTF/wtf/CMakeLists.txt

    r247815 r248525  
    394394    RandomNumber.cpp
    395395    ReadWriteLock.cpp
     396    RefCounted.cpp
    396397    RefCountedLeakCounter.cpp
    397398    RunLoop.cpp
  • trunk/Source/WTF/wtf/RefCounted.h

    r248488 r248525  
    2323#include <wtf/Assertions.h>
    2424#include <wtf/FastMalloc.h>
     25#include <wtf/MainThread.h>
    2526#include <wtf/Noncopyable.h>
    2627
     
    4041    void ref() const
    4142    {
     43#if !ASSERT_DISABLED
     44        if (m_isOwnedByMainThread != isMainThread() && hasOneRef())
     45            m_isOwnedByMainThread = isMainThread(); // Likely ownership transfer.
     46
     47        // If you hit this assertion, it means that the RefCounted object was ref'd or deref'd
     48        // concurrent from several threads, which is not safe. You should either subclass
     49        // ThreadSafeRefCounted instead, or make sure to always ref / deref from the same thread.
     50        ASSERT_WITH_MESSAGE(!areThreadingCheckedEnabled() || m_isOwnedByMainThread == isMainThread(), "Should not be ref'd / deref'd concurrently from several threads");
     51#endif
     52
    4253#if CHECK_REF_COUNTED_LIFECYCLE
    4354        ASSERT_WITH_SECURITY_IMPLICATION(!m_deletionHasBegun);
     
    6980    }
    7081
     82    // Please only call this method if you really know that what you're doing is safe (e.g.
     83    // locking at call sites).
     84    void disableThreadingChecks()
     85    {
     86#if !ASSERT_DISABLED
     87        m_areThreadingChecksEnabled = false;
     88#endif
     89    }
     90
     91    static void enableThreadingChecksGlobally()
     92    {
     93#if !ASSERT_DISABLED
     94        areThreadingChecksEnabledGlobally = true;
     95#endif
     96    }
     97
    7198protected:
    7299    RefCountedBase()
    73100        : m_refCount(1)
     101#if !ASSERT_DISABLED
     102        , m_isOwnedByMainThread(isMainThread())
     103#endif
    74104#if CHECK_REF_COUNTED_LIFECYCLE
    75105        , m_deletionHasBegun(false)
     
    79109    }
    80110
     111#if !ASSERT_DISABLED
     112    bool areThreadingCheckedEnabled() const
     113    {
     114        return areThreadingChecksEnabledGlobally && m_areThreadingChecksEnabled;
     115    }
     116#endif
     117
    81118    ~RefCountedBase()
    82119    {
     
    90127    bool derefBase() const
    91128    {
     129#if !ASSERT_DISABLED
     130        if (m_isOwnedByMainThread != isMainThread() && hasOneRef())
     131            m_isOwnedByMainThread = isMainThread(); // Likely ownership transfer.
     132
     133        // If you hit this assertion, it means that the RefCounted object was ref'd or deref'd
     134        // concurrent from several threads, which is not safe. You should either subclass
     135        // ThreadSafeRefCounted instead, or make sure to always ref / deref from the same thread.
     136        ASSERT_WITH_MESSAGE(!areThreadingCheckedEnabled() || m_isOwnedByMainThread == isMainThread(), "Should not be ref'd / deref'd concurrently from several threads");
     137#endif
     138
    92139#if CHECK_REF_COUNTED_LIFECYCLE
    93140        ASSERT_WITH_SECURITY_IMPLICATION(!m_deletionHasBegun);
     
    121168
    122169    mutable unsigned m_refCount;
     170#if !ASSERT_DISABLED
     171    mutable bool m_isOwnedByMainThread;
     172    bool m_areThreadingChecksEnabled { true };
     173    WTF_EXPORT_PRIVATE static bool areThreadingChecksEnabledGlobally;
     174#endif
    123175#if CHECK_REF_COUNTED_LIFECYCLE
    124176    mutable bool m_deletionHasBegun;
  • trunk/Source/WTF/wtf/SizeLimits.cpp

    r230130 r248525  
    4646    bool b;
    4747    bool c;
     48    bool d;
     49    bool e;
    4850    // The debug version may get bigger.
    4951};
  • trunk/Source/WebKit/ChangeLog

    r248522 r248525  
     12019-08-11  Chris Dumez  <cdumez@apple.com>
     2
     3        Add threading assertions to RefCounted
     4        https://bugs.webkit.org/show_bug.cgi?id=200507
     5
     6        Reviewed by Ryosuke Niwa.
     7
     8        Enable new RefCounted threading assertions for WebKit2
     9        (UIProcess + auxiliary processes).
     10
     11        * Shared/AuxiliaryProcess.cpp:
     12        (WebKit::AuxiliaryProcess::initialize):
     13        * Shared/Cocoa/WebKit2InitializeCocoa.mm:
     14        (WebKit::runInitializationCode):
     15        * Shared/WebKit2Initialize.cpp:
     16        (WebKit::InitializeWebKit2):
     17
    1182019-08-11  Wenson Hsieh  <wenson_hsieh@apple.com>
    219
  • trunk/Source/WebKit/Shared/AuxiliaryProcess.cpp

    r243528 r248525  
    6161void AuxiliaryProcess::initialize(const AuxiliaryProcessInitializationParameters& parameters)
    6262{
     63    WTF::RefCountedBase::enableThreadingChecksGlobally();
     64
    6365    RELEASE_ASSERT_WITH_MESSAGE(parameters.processIdentifier, "Unable to initialize child process without a WebCore process identifier");
    6466    Process::setIdentifier(*parameters.processIdentifier);
  • trunk/Source/WebKit/Shared/Cocoa/WebKit2InitializeCocoa.mm

    r237266 r248525  
    3232#import <mutex>
    3333#import <wtf/MainThread.h>
     34#import <wtf/RefCounted.h>
    3435#import <wtf/RunLoop.h>
    3536
     
    5051    JSC::initializeThreading();
    5152    RunLoop::initializeMainRunLoop();
     53
     54    WTF::RefCountedBase::enableThreadingChecksGlobally();
    5255
    5356#if !LOG_DISABLED || !RELEASE_LOG_DISABLED
  • trunk/Source/WebKit/Shared/WebKit2Initialize.cpp

    r228218 r248525  
    3131#include <WebCore/LogInitialization.h>
    3232#include <wtf/MainThread.h>
     33#include <wtf/RefCounted.h>
    3334#include <wtf/RunLoop.h>
    3435
     
    4243    RunLoop::initializeMainRunLoop();
    4344
     45    WTF::RefCountedBase::enableThreadingChecksGlobally();
     46
    4447#if !LOG_DISABLED || !RELEASE_LOG_DISABLED
    4548    WebCore::initializeLogChannelsIfNecessary();
  • trunk/Source/WebKitLegacy/mac/ChangeLog

    r248498 r248525  
     12019-08-11  Chris Dumez  <cdumez@apple.com>
     2
     3        Add threading assertions to RefCounted
     4        https://bugs.webkit.org/show_bug.cgi?id=200507
     5
     6        Reviewed by Ryosuke Niwa.
     7
     8        * WebView/WebView.mm:
     9        (+[WebView initialize]):
     10        Enable new RefCounted threading assertions for WebKitLegacy.
     11
    1122019-08-10  Tim Horton  <timothy_horton@apple.com>
    213
  • trunk/Source/WebKitLegacy/mac/WebView/WebView.mm

    r248498 r248525  
    54175417#endif
    54185418
     5419    WTF::RefCountedBase::enableThreadingChecksGlobally();
     5420
    54195421    WTF::setProcessPrivileges(allPrivileges());
    54205422    WebCore::NetworkStorageSession::permitProcessToUseCookieAPI(true);
Note: See TracChangeset for help on using the changeset viewer.