⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 276691 in webkit


Ignore:
Timestamp:
Apr 27, 2021, 11:26:12 PM (5 years ago)
Author:
commit-queue@webkit.org
Message:

Add a Condition type that supports thread safety analysis
​https://bugs.webkit.org/show_bug.cgi?id=224970

Patch by Kimmo Kinnunen <​kkinnunen@apple.com> on 2021-04-27
Reviewed by Darin Adler.

Source/WebKit:

Use CheckedCondition and CheckedLock as an example of the
added implementations.

  • Platform/IPC/Connection.cpp:

(IPC::Connection::waitForMessage):
(IPC::Connection::processIncomingMessage):
(IPC::Connection::connectionDidClose):

  • Platform/IPC/Connection.h:

(IPC::Connection::WTF_GUARDED_BY_LOCK):
Use CheckedCondition (as an example).

Mark up variables protected by
IPC::Connection::m_waitForMessageMutex
to use thread safety analysis.

  • Shared/mac/MediaFormatReader/MediaTrackReader.cpp:

(WebKit::MediaTrackReader::greatestPresentationTime const):
Fix unlocked access.

(WebKit::MediaTrackReader::addSample):
(WebKit::MediaTrackReader::waitForSample const):
(WebKit::MediaTrackReader::finishParsing):
(WebKit::MediaTrackReader::copyProperty):
(WebKit::MediaTrackReader::finalize):

  • Shared/mac/MediaFormatReader/MediaTrackReader.h:

Use CheckedCondition (as an example).

Mark up variables protected by
MediaTrackReader::m_sampleStorageLock
to use thread safety analysis.

Source/WTF:

Add CheckedCondition, a condition variable to be used with CheckedLock.
Use thread safety analysis annotations for CheckedCondition.

  • WTF.xcodeproj/project.pbxproj:
  • wtf/CMakeLists.txt:
  • wtf/CheckedCondition.h: Added.
  • wtf/CheckedLock.h:

Tools:

A simple test for CheckedCondition to make sure
it compiles.

  • TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
  • TestWebKitAPI/Tests/WTF/CheckedConditionTest.cpp: Copied from Tools/TestWebKitAPI/Tests/WTF/CheckedLockTest.cpp.

(TestWebKitAPI::TEST):

  • TestWebKitAPI/Tests/WTF/CheckedLockTest.cpp:
Location:
trunk
Files:
1 added
12 edited
1 copied

Legend:

Unmodified
Added
Removed

  • trunk/Source/WTF/ChangeLog

    r276682 r276691  
     12021-04-27  Kimmo Kinnunen  <kkinnunen@apple.com>
     2
     3        Add a Condition type that supports thread safety analysis
     4        https://bugs.webkit.org/show_bug.cgi?id=224970
     5
     6        Reviewed by Darin Adler.
     7
     8        Add CheckedCondition, a condition variable to be used with CheckedLock.
     9        Use thread safety analysis annotations for CheckedCondition.
     10
     11        * WTF.xcodeproj/project.pbxproj:
     12        * wtf/CMakeLists.txt:
     13        * wtf/CheckedCondition.h: Added.
     14        * wtf/CheckedLock.h:
     15
    1162021-04-27  Ben Nham  <nham@apple.com>
    217

  • trunk/Source/WTF/WTF.xcodeproj/project.pbxproj

    r276303 r276691  
    457457                7B2739DC2624DAAA0040F182 /* ThreadSafetyAnalysis.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = ThreadSafetyAnalysis.h; sourceTree = "<group>"; };
    458458                7B2739DD2624DAC30040F182 /* CheckedLock.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = CheckedLock.h; sourceTree = "<group>"; };
     459                7B2739F0263179C30040F182 /* CheckedCondition.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = CheckedCondition.h; sourceTree = "<group>"; };
    459460                7C137941222326C700D7A824 /* AUTHORS */ = {isa = PBXFileReference; lastKnownFileType = text; path = AUTHORS; sourceTree = "<group>"; };
    460461                7C137942222326D500D7A824 /* ieee.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = ieee.h; sourceTree = "<group>"; };
    … …  
    982983                                A8A4726A151A825A004123FF /* CheckedArithmetic.h */,
    983984                                A8A4726B151A825A004123FF /* CheckedBoolean.h */,
     985                                7B2739F0263179C30040F182 /* CheckedCondition.h */,
    984986                                7B2739DD2624DAC30040F182 /* CheckedLock.h */,
    985987                                0F66B2801DC97BAB004A1D3F /* ClockType.cpp */,

  • trunk/Source/WTF/wtf/CMakeLists.txt

    r276303 r276691  
    2929    CheckedArithmetic.h
    3030    CheckedBoolean.h
     31    CheckedCondition.h
    3132    CheckedLock.h
    3233    ClockType.h

  • trunk/Source/WTF/wtf/CheckedLock.h

    r276247 r276691  
    6868    bool isHeld() const { return Lock::isHeld(); }
    6969    bool isLocked() const { return Lock::isLocked(); }
     70    friend class CheckedCondition;
    7071};
    7172

  • trunk/Source/WebKit/ChangeLog

    r276689 r276691  
     12021-04-27  Kimmo Kinnunen  <kkinnunen@apple.com>
     2
     3        Add a Condition type that supports thread safety analysis
     4        https://bugs.webkit.org/show_bug.cgi?id=224970
     5
     6        Reviewed by Darin Adler.
     7
     8        Use CheckedCondition and CheckedLock as an example of the
     9        added implementations.
     10
     11        * Platform/IPC/Connection.cpp:
     12        (IPC::Connection::waitForMessage):
     13        (IPC::Connection::processIncomingMessage):
     14        (IPC::Connection::connectionDidClose):
     15        * Platform/IPC/Connection.h:
     16        (IPC::Connection::WTF_GUARDED_BY_LOCK):
     17        Use CheckedCondition (as an example).
     18
     19        Mark up variables protected by
     20        IPC::Connection::m_waitForMessageMutex
     21        to use thread safety analysis.
     22
     23        * Shared/mac/MediaFormatReader/MediaTrackReader.cpp:
     24        (WebKit::MediaTrackReader::greatestPresentationTime const):
     25        Fix unlocked access.
     26
     27        (WebKit::MediaTrackReader::addSample):
     28        (WebKit::MediaTrackReader::waitForSample const):
     29        (WebKit::MediaTrackReader::finishParsing):
     30        (WebKit::MediaTrackReader::copyProperty):
     31        (WebKit::MediaTrackReader::finalize):
     32        * Shared/mac/MediaFormatReader/MediaTrackReader.h:
     33        Use CheckedCondition (as an example).
     34
     35        Mark up variables protected by
     36        MediaTrackReader::m_sampleStorageLock
     37        to use thread safety analysis.
     38
    1392021-04-27  Chris Dumez  <cdumez@apple.com>
    240

  • trunk/Source/WebKit/Platform/IPC/Connection.cpp

    r276678 r276691  
    521521
    522522    {
    523         auto locker = holdLock(m_waitForMessageMutex);
     523        Locker locker { m_waitForMessageMutex };
    524524
    525525        // We don't support having multiple clients waiting for messages.
    … …  
    570570        SyncMessageState::singleton().dispatchMessages();
    571571
    572         std::unique_lock<Lock> lock(m_waitForMessageMutex);
     572        Locker lock { m_waitForMessageMutex };
    573573
    574574        if (m_waitingForMessage->decoder) {
    … …  
    579579
    580580        // Now we wait.
    581         bool didTimeout = !m_waitForMessageCondition.waitUntil(lock, timeout.deadline());
     581        bool didTimeout = !m_waitForMessageCondition.waitUntil(m_waitForMessageMutex, timeout.deadline());
    582582        // We timed out, lost our connection, or a sync message came in with InterruptWaitingIfSyncMessageArrives, so stop waiting.
    583583        if (didTimeout || m_waitingForMessage->messageWaitingInterrupted) {
    … …  
    749749
    750750    // FIXME: These are practically the same mutex, so maybe they could be merged.
    751     auto waitForMessagesLocker = holdLock(m_waitForMessageMutex);
     751    Locker waitForMessagesLocker { m_waitForMessageMutex };
    752752
    753753    auto incomingMessagesLocker = holdLock(m_incomingMessagesMutex);
    … …  
    867867
    868868    {
    869         auto locker = holdLock(m_waitForMessageMutex);
     869        Locker locker { m_waitForMessageMutex };
    870870
    871871        ASSERT(m_shouldWaitForMessages);

  • trunk/Source/WebKit/Platform/IPC/Connection.h

    r276678 r276691  
    3434#include "MessageReceiver.h"
    3535#include "Timeout.h"
     36#include <wtf/CheckedCondition.h>
     37#include <wtf/CheckedLock.h>
    3638#include <wtf/CompletionHandler.h>
    37 #include <wtf/Condition.h>
    3839#include <wtf/Deque.h>
    3940#include <wtf/Forward.h>
    … …  
    412413    Lock m_outgoingMessagesMutex;
    413414    Deque<UniqueRef<Encoder>> m_outgoingMessages;
    414    
    415     Condition m_waitForMessageCondition;
    416     Lock m_waitForMessageMutex;
     415
     416    CheckedCondition m_waitForMessageCondition;
     417    CheckedLock m_waitForMessageMutex;
    417418
    418419    struct WaitForMessageState;
    419     WaitForMessageState* m_waitingForMessage { nullptr };
     420    WaitForMessageState* m_waitingForMessage WTF_GUARDED_BY_LOCK(m_waitForMessageMutex) { nullptr }; // NOLINT
    420421
    421422    class SyncMessageState;
    … …  
    423424    Lock m_syncReplyStateMutex;
    424425    bool m_shouldWaitForSyncReplies;
    425     bool m_shouldWaitForMessages;
     426    bool m_shouldWaitForMessages WTF_GUARDED_BY_LOCK(m_waitForMessageMutex);
    426427    struct PendingSyncReply;
    427428    Vector<PendingSyncReply> m_pendingSyncReplies;

  • trunk/Source/WebKit/Shared/mac/MediaFormatReader/MediaTrackReader.cpp

    r274378 r276691  
    8888MediaTime MediaTrackReader::greatestPresentationTime() const
    8989{
     90    Locker locker { m_sampleStorageLock };
    9091    auto& sampleMap = m_sampleStorage->sampleMap;
    9192    if (sampleMap.empty())
    … …  
    99100{
    100101    ASSERT(!isMainRunLoop());
    101     auto locker = holdLock(m_sampleStorageLock);
     102    Locker locker { m_sampleStorageLock };
    102103    if (!m_sampleStorage)
    103104        m_sampleStorage = makeUnique<SampleStorage>();
    … …  
    117118void MediaTrackReader::waitForSample(Function<bool(SampleMap&, bool)>&& predicate) const
    118119{
    119     auto locker = holdLock(m_sampleStorageLock);
     120    Locker locker { m_sampleStorageLock };
    120121    if (!m_sampleStorage)
    121122        m_sampleStorage = makeUnique<SampleStorage>();
    122123    m_sampleStorageCondition.wait(m_sampleStorageLock, [predicate = WTFMove(predicate), this] {
     124        assertIsHeld(m_sampleStorageLock);
    123125        return predicate(m_sampleStorage->sampleMap, m_sampleStorage->hasAllSamples);
    124126    });
    … …  
    130132
    131133    ALWAYS_LOG(LOGIDENTIFIER);
    132     auto locker = holdLock(m_sampleStorageLock);
     134    Locker locker { m_sampleStorageLock };
    133135    if (!m_sampleStorage)
    134136        m_sampleStorage = makeUnique<SampleStorage>();
    … …  
    166168    }
    167169
    168     auto locker = holdLock(m_sampleStorageLock);
     170    Locker locker { m_sampleStorageLock };
    169171    m_sampleStorageCondition.wait(m_sampleStorageLock, [&] {
     172        assertIsHeld(m_sampleStorageLock);
    170173        return !!m_sampleStorage;
    171174    });
    … …  
    199202void MediaTrackReader::finalize()
    200203{
    201     auto locker = holdLock(m_sampleStorageLock);
     204    Locker locker { m_sampleStorageLock };
    202205    storageQueue().dispatch([sampleStorage = std::exchange(m_sampleStorage, nullptr)]() mutable {
    203206        sampleStorage = nullptr;

  • trunk/Source/WebKit/Shared/mac/MediaFormatReader/MediaTrackReader.h

    r274174 r276691  
    3030#include "CoreMediaWrapped.h"
    3131#include <WebCore/SampleMap.h>
    32 #include <wtf/Condition.h>
     32#include <wtf/CheckedCondition.h>
     33#include <wtf/CheckedLock.h>
    3334
    3435DECLARE_CORE_MEDIA_TRAITS(TrackReader);
    … …  
    106107    const MediaTime m_duration;
    107108    std::atomic<Enabled> m_isEnabled { Enabled::Unknown };
    108     mutable Condition m_sampleStorageCondition;
    109     mutable Lock m_sampleStorageLock;
    110     mutable std::unique_ptr<SampleStorage> m_sampleStorage;
     109    mutable CheckedCondition m_sampleStorageCondition;
     110    mutable CheckedLock m_sampleStorageLock;
     111    mutable std::unique_ptr<SampleStorage> m_sampleStorage WTF_GUARDED_BY_LOCK(m_sampleStorageLock);
    111112    Ref<const Logger> m_logger;
    112113    const void* m_logIdentifier;

  • trunk/Tools/ChangeLog

    r276670 r276691  
     12021-04-27  Kimmo Kinnunen  <kkinnunen@apple.com>
     2
     3        Add a Condition type that supports thread safety analysis
     4        https://bugs.webkit.org/show_bug.cgi?id=224970
     5
     6        Reviewed by Darin Adler.
     7
     8        A simple test for CheckedCondition to make sure
     9        it compiles.
     10
     11        * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
     12        * TestWebKitAPI/Tests/WTF/CheckedConditionTest.cpp: Copied from Tools/TestWebKitAPI/Tests/WTF/CheckedLockTest.cpp.
     13        (TestWebKitAPI::TEST):
     14        * TestWebKitAPI/Tests/WTF/CheckedLockTest.cpp:
     15
    1162021-04-27  Sam Sneddon  <gsnedders@apple.com>
    217

  • trunk/Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj

    r276620 r276691  
    584584                7AEAD4811E20122700416EFE /* CrossPartitionFileSchemeAccess.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 7AEAD47D1E20114E00416EFE /* CrossPartitionFileSchemeAccess.html */; };
    585585                7B2739E0262571CC0040F182 /* CheckedLockTest.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 7B2739DF262571CC0040F182 /* CheckedLockTest.cpp */; };
     586                7B2739EF26315E7E0040F182 /* CheckedConditionTest.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 7B2739EE26315E7D0040F182 /* CheckedConditionTest.cpp */; };
    586587                7B7D096A2519F8F90017A078 /* WebGLNoCrashOnOtherThreadAccess.mm in Sources */ = {isa = PBXBuildFile; fileRef = 7B7D09692519F8F90017A078 /* WebGLNoCrashOnOtherThreadAccess.mm */; };
    587588                7C1AF7951E8DCBAB002645B9 /* PrepareForMoveToWindow.mm in Sources */ = {isa = PBXBuildFile; fileRef = 7C1AF7931E8DCBAB002645B9 /* PrepareForMoveToWindow.mm */; };
    … …  
    24332434                7AEAD47D1E20114E00416EFE /* CrossPartitionFileSchemeAccess.html */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = text.html; name = CrossPartitionFileSchemeAccess.html; path = Tests/mac/CrossPartitionFileSchemeAccess.html; sourceTree = SOURCE_ROOT; };
    24342435                7B2739DF262571CC0040F182 /* CheckedLockTest.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = CheckedLockTest.cpp; sourceTree = "<group>"; };
     2436                7B2739EE26315E7D0040F182 /* CheckedConditionTest.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = CheckedConditionTest.cpp; sourceTree = "<group>"; };
    24352437                7B7D09692519F8F90017A078 /* WebGLNoCrashOnOtherThreadAccess.mm */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.objcpp; path = WebGLNoCrashOnOtherThreadAccess.mm; sourceTree = "<group>"; };
    24362438                7C1AF7931E8DCBAB002645B9 /* PrepareForMoveToWindow.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = PrepareForMoveToWindow.mm; sourceTree = "<group>"; };
    … …  
    43774379                                0451A5A6235E438E009DF945 /* BumpPointerAllocator.cpp */,
    43784380                                A7A966DA140ECCC8005EF9B4 /* CheckedArithmeticOperations.cpp */,
     4381                                7B2739EE26315E7D0040F182 /* CheckedConditionTest.cpp */,
    43794382                                7B2739DF262571CC0040F182 /* CheckedLockTest.cpp */,
    43804383                                E302BDA92404B92300865277 /* CompactRefPtrTuple.cpp */,
    … …  
    51335136                                04DB2396235E43EC00328F17 /* BumpPointerAllocator.cpp in Sources */,
    51345137                                7C83DEA01D0A590C00FEBCF3 /* CheckedArithmeticOperations.cpp in Sources */,
     5138                                7B2739EF26315E7E0040F182 /* CheckedConditionTest.cpp in Sources */,
    51355139                                7B2739E0262571CC0040F182 /* CheckedLockTest.cpp in Sources */,
    51365140                                E302BDAA2404B92400865277 /* CompactRefPtrTuple.cpp in Sources */,

  • trunk/Tools/TestWebKitAPI/Tests/WTF/CheckedConditionTest.cpp

    r276690 r276691  
    2525
    2626#include "config.h"
    27 #include <wtf/CheckedLock.h>
    28 
    29 #include <wtf/StdLibExtras.h>
     27#include <wtf/CheckedCondition.h>
    3028
    3129namespace TestWebKitAPI {
    3230
    33 namespace {
    34 class MyValue {
    35 public:
    36     void setValue(int value)
    37     {
    38         Locker holdLock { m_lock };
    39         m_value = value;
    40     }
    41     void maybeSetOtherValue(int value)
    42     {
    43         if (!m_otherLock.tryLock())
    44             return;
    45         Locker holdLock { AdoptLockTag { }, m_otherLock };
    46         m_otherValue = value;
    47     }
    48     // This function can be used to manually check that compile fails.
    49     template<typename T> void shouldFailCompile(T t)
    50     {
    51         m_value = t;
    52     }
    53     private:
    54     CheckedLock m_lock;
    55     int m_value WTF_GUARDED_BY_LOCK(m_lock) { 77 };
    56     CheckedLock m_otherLock;
    57     int m_otherValue WTF_GUARDED_BY_LOCK(m_otherLock) { 88 };
    58 };
    59 
    60 }
    61 
    62 TEST(WTF_CheckedLock, CheckedLockCompiles)
     31TEST(WTF_CheckedLock, CheckedConditionCompiles)
    6332{
    64     MyValue v;
    65     v.setValue(7);
    66     v.maybeSetOtherValue(34);
     33    CheckedLock lock;
     34    CheckedCondition condition;
     35    Locker locker { lock }; // Comment this to ensure that thread safety analysis creates a compile error.
     36    bool result = condition.waitFor(lock, 0_s);
     37    EXPECT_FALSE(result);
    6738}
    6839

  • trunk/Tools/TestWebKitAPI/Tests/WTF/CheckedLockTest.cpp

    r276247 r276691  
    2626#include "config.h"
    2727#include <wtf/CheckedLock.h>
    28 
    29 #include <wtf/StdLibExtras.h>
    3028
    3129namespace TestWebKitAPI {
Note: See TracChangeset for help on using the changeset viewer.