Make Logger::log thread safe so that it can be used from background threads
authoryouenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 6 Aug 2019 03:45:40 +0000 (03:45 +0000)
committeryouenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 6 Aug 2019 03:45:40 +0000 (03:45 +0000)
https://bugs.webkit.org/show_bug.cgi?id=200448

Reviewed by Eric Carlson.

Source/WebCore:

No change of behavior.

* dom/Document.cpp:
(WebCore::crossThreadCopy):
(WebCore::Document::didLogMessage):
Make sure to hop to the main thread if needed.
* platform/mediastream/mac/RealtimeIncomingAudioSourceCocoa.cpp:
(WebCore::RealtimeIncomingAudioSourceCocoa::OnData):
Remove hopping to the main thread.
* platform/mediastream/mac/RealtimeIncomingVideoSourceCocoa.mm:
(WebCore::RealtimeIncomingVideoSourceCocoa::OnFrame):
Remove hopping to the main thread.

Source/WTF:

Add a lock to ensure calling log is thread-safe.

* wtf/Logger.h:
(WTF::Logger::addObserver):
(WTF::Logger::removeObserver):
(WTF::Logger::log):
(WTF::Logger::observerLock):

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

Source/WTF/ChangeLog
Source/WTF/wtf/Logger.h
Source/WebCore/ChangeLog
Source/WebCore/dom/Document.cpp
Source/WebCore/platform/mediastream/mac/RealtimeIncomingAudioSourceCocoa.cpp
Source/WebCore/platform/mediastream/mac/RealtimeIncomingVideoSourceCocoa.mm

index 1ac7f9b..75f2a9f 100644 (file)
@@ -1,3 +1,18 @@
+2019-08-05  Youenn Fablet  <youenn@apple.com>
+
+        Make Logger::log thread safe so that it can be used from background threads
+        https://bugs.webkit.org/show_bug.cgi?id=200448
+
+        Reviewed by Eric Carlson.
+
+        Add a lock to ensure calling log is thread-safe.
+
+        * wtf/Logger.h:
+        (WTF::Logger::addObserver):
+        (WTF::Logger::removeObserver):
+        (WTF::Logger::log):
+        (WTF::Logger::observerLock):
+
 2019-08-05  Takashi Komori  <Takashi.Komori@sony.com>
 
         [Curl] implement CertificateInfo::summaryInfo
index 0368ab0..b81b0f9 100644 (file)
@@ -25,6 +25,7 @@
 
 #pragma once
 
+#include <wtf/Lock.h>
 #include <wtf/text/StringBuilder.h>
 
 namespace WTF {
@@ -112,6 +113,7 @@ public:
     class Observer {
     public:
         virtual ~Observer() = default;
+        // Can be called on any thread.
         virtual void didLogMessage(const WTFLogChannel&, WTFLogLevel, Vector<JSONLogValue>&&) = 0;
     };
 
@@ -216,10 +218,12 @@ public:
 
     static inline void addObserver(Observer& observer)
     {
+        auto lock = holdLock(observerLock());
         observers().append(observer);
     }
     static inline void removeObserver(Observer& observer)
     {
+        auto lock = holdLock(observerLock());
         observers().removeFirstMatching([&observer](auto anObserver) {
             return &anObserver.get() == &observer;
         });
@@ -247,6 +251,10 @@ private:
         if (channel.state == WTFLogChannelState::Off || level > channel.level)
             return;
 
+        auto lock = tryHoldLock(observerLock());
+        if (!lock)
+            return;
+
         for (Observer& observer : observers())
             observer.didLogMessage(channel, level, { ConsoleLogValue<Argument>::toValue(arguments)... });
     }
@@ -257,6 +265,13 @@ private:
         return observers;
     }
 
+    static Lock& observerLock()
+    {
+        static NeverDestroyed<Lock> observerLock;
+        return observerLock;
+    }
+
+
     bool m_enabled { true };
     const void* m_owner;
 };
index c8181c4..3114e39 100644 (file)
@@ -1,3 +1,23 @@
+2019-08-05  Youenn Fablet  <youenn@apple.com>
+
+        Make Logger::log thread safe so that it can be used from background threads
+        https://bugs.webkit.org/show_bug.cgi?id=200448
+
+        Reviewed by Eric Carlson.
+
+        No change of behavior.
+
+        * dom/Document.cpp:
+        (WebCore::crossThreadCopy):
+        (WebCore::Document::didLogMessage):
+        Make sure to hop to the main thread if needed.
+        * platform/mediastream/mac/RealtimeIncomingAudioSourceCocoa.cpp:
+        (WebCore::RealtimeIncomingAudioSourceCocoa::OnData):
+        Remove hopping to the main thread.
+        * platform/mediastream/mac/RealtimeIncomingVideoSourceCocoa.mm:
+        (WebCore::RealtimeIncomingVideoSourceCocoa::OnFrame):
+        Remove hopping to the main thread.
+
 2019-08-05  Devin Rousso  <drousso@apple.com>
 
         Can't use $0, $1 etc when inspecting Google Docs pages because the content uses these for function names
index 0abffa8..ed8d934 100644 (file)
@@ -8020,8 +8020,22 @@ static MessageLevel messageLevelFromWTFLogLevel(WTFLogLevel level)
     return MessageLevel::Log;
 }
 
+static inline Vector<JSONLogValue> crossThreadCopy(Vector<JSONLogValue>&& source)
+{
+    auto values = WTFMove(source);
+    for (auto& value : values)
+        value.value = crossThreadCopy(WTFMove(value.value));
+    return values;
+}
+
 void Document::didLogMessage(const WTFLogChannel& channel, WTFLogLevel level, Vector<JSONLogValue>&& logMessages)
 {
+    if (!isMainThread()) {
+        postTask([this, channel, level, logMessages = crossThreadCopy(WTFMove(logMessages))](auto&) mutable {
+            didLogMessage(channel, level, WTFMove(logMessages));
+        });
+        return;
+    }
     if (!page())
         return;
 
index 2df9c48..8f12c1d 100644 (file)
@@ -87,11 +87,7 @@ void RealtimeIncomingAudioSourceCocoa::OnData(const void* audioData, int bitsPer
         memcpy(audioBufferList.buffer(0)->mData, audioData, audioBufferList.buffer(0)->mDataByteSize);
 
 #if !RELEASE_LOG_DISABLED
-    if (!(++m_chunksReceived % 200)) {
-        callOnMainThread([this, protectedThis = makeRef(*this)] {
-            ALWAYS_LOG_IF(loggerPtr(), LOGIDENTIFIER, "chunk ", m_chunksReceived);
-        });
-    }
+    ALWAYS_LOG_IF(loggerPtr() && !(++m_chunksReceived % 200), LOGIDENTIFIER, "chunk ", m_chunksReceived);
 #endif
 
     audioSamplesAvailable(mediaTime, audioBufferList, CAAudioStreamDescription(newDescription), numberOfFrames);
index d230659..c66f185 100644 (file)
@@ -158,18 +158,12 @@ void RealtimeIncomingVideoSourceCocoa::OnFrame(const webrtc::VideoFrame& frame)
         return;
 
 #if !RELEASE_LOG_DISABLED
-    if (!(++m_numberOfFrames % 60)) {
-        callOnMainThread([this, protectedThis = makeRef(*this), numberOfFrames = m_numberOfFrames] {
-            ALWAYS_LOG_IF(loggerPtr(), LOGIDENTIFIER, "frame ", numberOfFrames);
-        });
-    }
+    ALWAYS_LOG_IF(loggerPtr() && !(++m_numberOfFrames % 60), LOGIDENTIFIER, "frame ", m_numberOfFrames);
 #endif
 
     auto pixelBuffer = pixelBufferFromVideoFrame(frame);
     if (!pixelBuffer) {
-        callOnMainThread([this, protectedThis = makeRef(*this)] {
-            ERROR_LOG_IF(loggerPtr(), LOGIDENTIFIER, "Failed to get a pixel buffer from a frame");
-        });
+        ERROR_LOG_IF(loggerPtr(), LOGIDENTIFIER, "Failed to get a pixel buffer from a frame");
         return;
     }
 
@@ -183,9 +177,7 @@ void RealtimeIncomingVideoSourceCocoa::OnFrame(const webrtc::VideoFrame& frame)
     CMVideoFormatDescriptionRef formatDescription;
     OSStatus ostatus = CMVideoFormatDescriptionCreateForImageBuffer(kCFAllocatorDefault, (CVImageBufferRef)pixelBuffer, &formatDescription);
     if (ostatus != noErr) {
-        callOnMainThread([this, protectedThis = makeRef(*this), ostatus] {
-            ERROR_LOG_IF(loggerPtr(), LOGIDENTIFIER, "Failed to initialize CMVideoFormatDescription with error ", static_cast<int>(ostatus));
-        });
+        ERROR_LOG_IF(loggerPtr(), LOGIDENTIFIER, "Failed to initialize CMVideoFormatDescription with error ", static_cast<int>(ostatus));
         return;
     }
 
@@ -193,9 +185,7 @@ void RealtimeIncomingVideoSourceCocoa::OnFrame(const webrtc::VideoFrame& frame)
     ostatus = CMSampleBufferCreateReadyWithImageBuffer(kCFAllocatorDefault, (CVImageBufferRef)pixelBuffer, formatDescription, &timingInfo, &sampleBuffer);
     CFRelease(formatDescription);
     if (ostatus != noErr) {
-        callOnMainThread([this, protectedThis = makeRef(*this), ostatus] {
-            ERROR_LOG_IF(loggerPtr(), LOGIDENTIFIER, "Failed to create the sample buffer with error ", static_cast<int>(ostatus));
-        });
+        ERROR_LOG_IF(loggerPtr(), LOGIDENTIFIER, "Failed to create the sample buffer with error ", static_cast<int>(ostatus));
         return;
     }