LayoutTest workers/bomb.html is a Crash
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 4 Jul 2017 05:18:15 +0000 (05:18 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 4 Jul 2017 05:18:15 +0000 (05:18 +0000)
https://bugs.webkit.org/show_bug.cgi?id=167757
<rdar://problem/33086462>

Reviewed by Keith Miller.

Source/JavaScriptCore:

VMTraps::SignalSender was accessing VM fields even after
the VM was destroyed. This happened when the SignalSender
thread was in the middle of its work() function while VMTraps
was notified that the VM was shutting down. The VM would proceed
to run its destructor even after the SignalSender thread finished
doing its work. This means that the SignalSender thread was accessing
VM field eve after VM was destructed (including itself, since it is
transitively owned by the VM). The VM must wait for the SignalSender
thread to shutdown before it can continue to destruct itself.

* runtime/VMTraps.cpp:
(JSC::VMTraps::willDestroyVM):

Source/WTF:

* wtf/AutomaticThread.cpp:
(WTF::AutomaticThreadCondition::waitFor):
* wtf/AutomaticThread.h:

LayoutTests:

* platform/mac-wk2/TestExpectations:

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

LayoutTests/ChangeLog
LayoutTests/platform/mac-wk2/TestExpectations
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/runtime/VMTraps.cpp
Source/WTF/ChangeLog
Source/WTF/wtf/AutomaticThread.cpp
Source/WTF/wtf/AutomaticThread.h

index e7e8d38..1c651cc 100644 (file)
@@ -1,3 +1,13 @@
+2017-07-03  Saam Barati  <sbarati@apple.com>
+
+        LayoutTest workers/bomb.html is a Crash
+        https://bugs.webkit.org/show_bug.cgi?id=167757
+        <rdar://problem/33086462>
+
+        Reviewed by Keith Miller.
+
+        * platform/mac-wk2/TestExpectations:
+
 2017-07-03  Matt Lewis  <jlewis3@apple.com>
 
         Removed expectations and skipped workers/bomb.html on mac.
index cd2c5be..d1ef3a7 100644 (file)
@@ -706,5 +706,5 @@ webkit.org/b/172201 webaudio/silent-audio-interrupted-in-background.html [ Pass
 
 webkit.org/b/173608 webrtc/video-replace-muted-track.html [ Pass Failure ]
 
-webkit.org/b/167757 workers/bomb.html [ Skip ]
+webkit.org/b/167757 workers/bomb.html [ Pass Timeout ]
 
index 9c47a95..929f8c8 100644 (file)
@@ -1,5 +1,26 @@
 2017-07-03  Saam Barati  <sbarati@apple.com>
 
+        LayoutTest workers/bomb.html is a Crash
+        https://bugs.webkit.org/show_bug.cgi?id=167757
+        <rdar://problem/33086462>
+
+        Reviewed by Keith Miller.
+
+        VMTraps::SignalSender was accessing VM fields even after
+        the VM was destroyed. This happened when the SignalSender
+        thread was in the middle of its work() function while VMTraps
+        was notified that the VM was shutting down. The VM would proceed
+        to run its destructor even after the SignalSender thread finished
+        doing its work. This means that the SignalSender thread was accessing
+        VM field eve after VM was destructed (including itself, since it is
+        transitively owned by the VM). The VM must wait for the SignalSender
+        thread to shutdown before it can continue to destruct itself.
+
+        * runtime/VMTraps.cpp:
+        (JSC::VMTraps::willDestroyVM):
+
+2017-07-03  Saam Barati  <sbarati@apple.com>
+
         DFGBytecodeParser op_to_this does not access the correct instruction offset for to this status
         https://bugs.webkit.org/show_bug.cgi?id=174110
 
index 02fa4fd..2479918 100644 (file)
@@ -254,8 +254,6 @@ protected:
 
     WorkResult work() override
     {
-
-        // We need a nested scope so that we'll release the lock before we sleep below.
         VM& vm = m_vm;
 
         auto optionalOwnerThread = vm.ownerThread();
@@ -291,7 +289,12 @@ protected:
             });
         }
 
-        sleepMS(1);
+        {
+            auto locker = holdLock(*traps().m_lock);
+            if (traps().m_isShuttingDown)
+                return WorkResult::Stop;
+            traps().m_trapSet->waitFor(*traps().m_lock, 1_ms);
+        }
         return WorkResult::Continue;
     }
     
@@ -305,7 +308,6 @@ private:
 void VMTraps::willDestroyVM()
 {
     m_isShuttingDown = true;
-    WTF::storeStoreFence();
 #if ENABLE(SIGNAL_BASED_VM_TRAPS)
     if (m_signalSender) {
         {
@@ -313,8 +315,7 @@ void VMTraps::willDestroyVM()
             if (!m_signalSender->tryStop(locker))
                 m_trapSet->notifyAll(locker);
         }
-        if (!ASSERT_DISABLED)
-            m_signalSender->join();
+        m_signalSender->join();
         m_signalSender = nullptr;
     }
 #endif
index d93683e..99d5400 100644 (file)
@@ -1,3 +1,15 @@
+2017-07-03  Saam Barati  <sbarati@apple.com>
+
+        LayoutTest workers/bomb.html is a Crash
+        https://bugs.webkit.org/show_bug.cgi?id=167757
+        <rdar://problem/33086462>
+
+        Reviewed by Keith Miller.
+
+        * wtf/AutomaticThread.cpp:
+        (WTF::AutomaticThreadCondition::waitFor):
+        * wtf/AutomaticThread.h:
+
 2017-07-03  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r219060.
index 28b45ee..8111876 100644 (file)
@@ -81,6 +81,11 @@ void AutomaticThreadCondition::wait(Lock& lock)
     m_condition.wait(lock);
 }
 
+bool AutomaticThreadCondition::waitFor(Lock& lock, Seconds time)
+{
+    return m_condition.waitFor(lock, time);
+}
+
 void AutomaticThreadCondition::add(const AbstractLocker&, AutomaticThread* thread)
 {
     ASSERT(!m_threads.contains(thread));
index 6eea239..1b90454 100644 (file)
@@ -82,6 +82,7 @@ public:
     // threads. In such cases, the thread doing the notifyAll() can wake up at most one thread -
     // its partner.
     WTF_EXPORT_PRIVATE void wait(Lock&);
+    WTF_EXPORT_PRIVATE bool waitFor(Lock&, Seconds);
     
 private:
     friend class AutomaticThread;