Changeset 215694 in webkit

Timestamp:

Apr 24, 2017 2:11:44 PM (7 years ago)

Author:

sbarati@apple.com

Message:

ASSERTION FAILED: m_table seen with workers/wasm-hashset LayoutTests
​https://bugs.webkit.org/show_bug.cgi?id=171119
<rdar://problem/31760635>

Reviewed by Keith Miller.

The HashSet of timer set notification callbacks can be accessed
and augmented simultaneously from different threads. e.g, the worker
thread can augment it while the wasm compilation thread will
access it. Therefore, accesses must be guarded by a lock.

• runtime/JSRunLoopTimer.cpp:

(JSC::JSRunLoopTimer::scheduleTimer):
(JSC::JSRunLoopTimer::addTimerSetNotification):
(JSC::JSRunLoopTimer::removeTimerSetNotification):

• runtime/JSRunLoopTimer.h:

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• runtime/JSRunLoopTimer.cpp (modified) (4 diffs)
• runtime/JSRunLoopTimer.h (modified) (2 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2017-04-24  Saam Barati  <sbarati@apple.com>
+
+        ASSERTION FAILED: m_table seen with workers/wasm-hashset LayoutTests
+        https://bugs.webkit.org/show_bug.cgi?id=171119
+        <rdar://problem/31760635>
+
+        Reviewed by Keith Miller.
+
+        The HashSet of timer set notification callbacks can be accessed
+        and augmented simultaneously from different threads. e.g, the worker
+        thread can augment it while the wasm compilation thread will
+        access it. Therefore, accesses must be guarded by a lock.
+
+        * runtime/JSRunLoopTimer.cpp:
+        (JSC::JSRunLoopTimer::scheduleTimer):
+        (JSC::JSRunLoopTimer::addTimerSetNotification):
+        (JSC::JSRunLoopTimer::removeTimerSetNotification):
+        * runtime/JSRunLoopTimer.h:
+
 2017-04-24  Joseph Pecoraro  <pecoraro@apple.com>
 

trunk/Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp

@@ -105 +105 @@
     CFRunLoopTimerSetNextFireDate(m_timer.get(), CFAbsoluteTimeGetCurrent() + intervalInSeconds.seconds());
     m_isScheduled = true;
+    auto locker = holdLock(m_timerCallbacksLock);
     for (auto& task : m_timerSetCallbacks)
         task->run();
@@ -143 +144 @@
     m_timer.startOneShot(intervalInSeconds);
     m_isScheduled = true;
+
+    auto locker = holdLock(m_timerCallbacksLock);
     for (auto& task : m_timerSetCallbacks)
         task->run();
@@ -157 +160 @@
 void JSRunLoopTimer::addTimerSetNotification(TimerNotificationCallback callback)
 {
+    auto locker = holdLock(m_timerCallbacksLock);
     m_timerSetCallbacks.add(callback);
 }
@@ -162 +166 @@
 void JSRunLoopTimer::removeTimerSetNotification(TimerNotificationCallback callback)
 {
+    auto locker = holdLock(m_timerCallbacksLock);
     m_timerSetCallbacks.remove(callback);
 }

trunk/Source/JavaScriptCore/runtime/JSRunLoopTimer.h

@@ -63 +63 @@
     bool isScheduled() const { return m_isScheduled; }
 
+    // Note: The only thing the timer notification callback cannot do is
+    // call scheduleTimer(). This will cause a deadlock. It would not
+    // be hard to make this work, however, there are no clients that need
+    // this behavior. We should implement it only if we find that we need it.
     JS_EXPORT_PRIVATE void addTimerSetNotification(TimerNotificationCallback);
     JS_EXPORT_PRIVATE void removeTimerSetNotification(TimerNotificationCallback);
@@ -88 +92 @@
 #endif
 
+    Lock m_timerCallbacksLock;
     HashSet<TimerNotificationCallback> m_timerSetCallbacks;