Changeset 248288 in webkit

Timestamp:

Aug 5, 2019, 8:45:40 PM (7 years ago)

Author:

youenn@apple.com

Message:

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.

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):

Location:

trunk/Source

Files:

• WTF/ChangeLog (modified) (1 diff)
• WTF/wtf/Logger.h (modified) (5 diffs)
• WebCore/ChangeLog (modified) (1 diff)
• WebCore/dom/Document.cpp (modified) (1 diff)
• WebCore/platform/mediastream/mac/RealtimeIncomingAudioSourceCocoa.cpp (modified) (1 diff)
• WebCore/platform/mediastream/mac/RealtimeIncomingVideoSourceCocoa.mm (modified) (3 diffs)

trunk/Source/WTF/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WTF/wtf/Logger.h

@@ -26 +26 @@
 #pragma once
 
+#include <wtf/Lock.h>
 #include <wtf/text/StringBuilder.h>
 
@@ -113 +114 @@
     public:
         virtual ~Observer() = default;
+        // Can be called on any thread.
         virtual void didLogMessage(const WTFLogChannel&, WTFLogLevel, Vector<JSONLogValue>&&) = 0;
     };
@@ -217 +219 @@
     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;
@@ -248 +252 @@
             return;
 
+        auto lock = tryHoldLock(observerLock());
+        if (!lock)
+            return;
+
         for (Observer& observer : observers())
             observer.didLogMessage(channel, level, { ConsoleLogValue<Argument>::toValue(arguments)... });
@@ -258 +266 @@
     }
 
+    static Lock& observerLock()
+    {
+        static NeverDestroyed<Lock> observerLock;
+        return observerLock;
+    }
+
+
     bool m_enabled { true };
     const void* m_owner;

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/dom/Document.cpp

@@ -8021 +8021 @@
 }
 
+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;

trunk/Source/WebCore/platform/mediastream/mac/RealtimeIncomingAudioSourceCocoa.cpp

@@ -88 +88 @@
 
 #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
 

trunk/Source/WebCore/platform/mediastream/mac/RealtimeIncomingVideoSourceCocoa.mm

@@ -159 +159 @@
 
 #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;
     }
@@ -184 +178 @@
     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;
     }
@@ -194 +186 @@
     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;
     }