<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"

"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">

<html xmlns="http://www.w3.org/1999/xhtml">

<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />

<title>[210398] trunk/Source</title>

</head>

<body>

<style type="text/css"><!--

#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }

#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }

#msg dt:after { content:':';}

#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt;  }

#msg dl a { font-weight: bold}

#msg dl a:link    { color:#fc3; }

#msg dl a:active  { color:#ff0; }

#msg dl a:visited { color:#cc6; }

h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }

#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }

#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }

#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }

#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }

#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }

#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }

#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }

#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }

#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }

#logmsg pre { background: #eee; padding: 1em; }

#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}

#logmsg dl { margin: 0; }

#logmsg dt { font-weight: bold; }

#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }

#logmsg dd:before { content:'\00bb';}

#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }

#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }

#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }

#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }

#logmsg table th.Corner { text-align: left; }

#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }

#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }

#patch { width: 100%; }

#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}

#patch .propset h4, #patch .binary h4 {margin:0;}

#patch pre {padding:0;line-height:1.2em;margin:0;}

#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}

#patch .propset .diff, #patch .binary .diff  {padding:10px 0;}

#patch span {display:block;padding:0 10px;}

#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}

#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}

#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}

#patch .lines, .info {color:#888;background:#fff;}

--></style>

<div id="msg">

<dl class="meta">

<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/210398">210398</a></dd>

<dt>Author</dt> <dd>fpizlo@apple.com</dd>

<dt>Date</dt> <dd>2017-01-05 16:24:11 -0800 (Thu, 05 Jan 2017)</dd>

</dl>

<h3>Log Message</h3>

<pre>AutomaticThread timeout shutdown leaves a small window where notify() would think that the thread is still running

https://bugs.webkit.org/show_bug.cgi?id=166742

Reviewed by Geoffrey Garen.

Source/JavaScriptCore:

Update to new AutomaticThread API.

* dfg/DFGWorklist.cpp:

Source/WTF:

Remove the use of the RAII ThreadScope, since the use of RAII helped make this bug possible:

we'd do ~ThreadScope after we had done ~LockHolder, so in between when we decided to shut

down a thread and when it reported itself as being shut down, there was a window where a

notify() call would get confused.

Now, we run all thread shutdown stuff while the lock is held. We release the lock last. One

API implication is that threadWillStop becomes threadIsStopping and it's called while the

lock is held. This seems benign.

* wtf/AutomaticThread.cpp:

(WTF::AutomaticThread::start):

(WTF::AutomaticThread::threadIsStopping):

(WTF::AutomaticThread::ThreadScope::ThreadScope): Deleted.

(WTF::AutomaticThread::ThreadScope::~ThreadScope): Deleted.

(WTF::AutomaticThread::threadWillStop): Deleted.

* wtf/AutomaticThread.h:</pre>

<h3>Modified Paths</h3>

<ul>

<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>

<li><a href="#trunkSourceJavaScriptCoredfgDFGWorklistcpp">trunk/Source/JavaScriptCore/dfg/DFGWorklist.cpp</a></li>

<li><a href="#trunkSourceWTFChangeLog">trunk/Source/WTF/ChangeLog</a></li>

<li><a href="#trunkSourceWTFwtfAutomaticThreadcpp">trunk/Source/WTF/wtf/AutomaticThread.cpp</a></li>

<li><a href="#trunkSourceWTFwtfAutomaticThreadh">trunk/Source/WTF/wtf/AutomaticThread.h</a></li>

</ul>

</div>

<div id="patch">

<h3>Diff</h3>

<a id="trunkSourceJavaScriptCoreChangeLog"></a>

<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (210397 => 210398)</h4>

<pre class="diff"><span>

<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog        2017-01-05 23:50:00 UTC (rev 210397)

+++ trunk/Source/JavaScriptCore/ChangeLog        2017-01-06 00:24:11 UTC (rev 210398)

</span><span class="lines">@@ -1,3 +1,14 @@

</span><ins>+2017-01-05  Filip Pizlo  &lt;fpizlo@apple.com&gt;

+

+        AutomaticThread timeout shutdown leaves a small window where notify() would think that the thread is still running

+        https://bugs.webkit.org/show_bug.cgi?id=166742

+

+        Reviewed by Geoffrey Garen.

+        

+        Update to new AutomaticThread API.

+

+        * dfg/DFGWorklist.cpp:

+

</ins><span class="cx"> 2017-01-05  Per Arne Vollan  &lt;pvollan@apple.com&gt;

</span><span class="cx"> 

</span><span class="cx">         [Win] Compile error.

</span></span></pre></div>

<a id="trunkSourceJavaScriptCoredfgDFGWorklistcpp"></a>

<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/dfg/DFGWorklist.cpp (210397 => 210398)</h4>

<pre class="diff"><span>

<span class="info">--- trunk/Source/JavaScriptCore/dfg/DFGWorklist.cpp        2017-01-05 23:50:00 UTC (rev 210397)

+++ trunk/Source/JavaScriptCore/dfg/DFGWorklist.cpp        2017-01-06 00:24:11 UTC (rev 210398)

</span><span class="lines">@@ -1,5 +1,5 @@

</span><span class="cx"> /*

</span><del>- * Copyright (C) 2013-2014, 2016 Apple Inc. All rights reserved.

</del><ins>+ * Copyright (C) 2013-2017 Apple Inc. All rights reserved.

</ins><span class="cx">  *

</span><span class="cx">  * Redistribution and use in source and binary forms, with or without

</span><span class="cx">  * modification, are permitted provided that the following conditions

</span><span class="lines">@@ -150,8 +150,10 @@

</span><span class="cx">         m_longLivedState = std::make_unique&lt;LongLivedState&gt;();

</span><span class="cx">     }

</span><span class="cx">     

</span><del>-    void threadWillStop() override

</del><ins>+    void threadIsStopping(const LockHolder&amp;) override

</ins><span class="cx">     {

</span><ins>+        // We're holding the Worklist::m_lock, so we should be careful not to deadlock.

+        

</ins><span class="cx">         if (Options::verboseCompilationQueue())

</span><span class="cx">             dataLog(m_worklist, &quot;: Thread will stop\n&quot;);

</span><span class="cx">         

</span></span></pre></div>

<a id="trunkSourceWTFChangeLog"></a>

<div class="modfile"><h4>Modified: trunk/Source/WTF/ChangeLog (210397 => 210398)</h4>

<pre class="diff"><span>

<span class="info">--- trunk/Source/WTF/ChangeLog        2017-01-05 23:50:00 UTC (rev 210397)

+++ trunk/Source/WTF/ChangeLog        2017-01-06 00:24:11 UTC (rev 210398)

</span><span class="lines">@@ -1,3 +1,27 @@

</span><ins>+2017-01-05  Filip Pizlo  &lt;fpizlo@apple.com&gt;

+

+        AutomaticThread timeout shutdown leaves a small window where notify() would think that the thread is still running

+        https://bugs.webkit.org/show_bug.cgi?id=166742

+

+        Reviewed by Geoffrey Garen.

+        

+        Remove the use of the RAII ThreadScope, since the use of RAII helped make this bug possible:

+        we'd do ~ThreadScope after we had done ~LockHolder, so in between when we decided to shut

+        down a thread and when it reported itself as being shut down, there was a window where a

+        notify() call would get confused.

+        

+        Now, we run all thread shutdown stuff while the lock is held. We release the lock last. One

+        API implication is that threadWillStop becomes threadIsStopping and it's called while the

+        lock is held. This seems benign.

+

+        * wtf/AutomaticThread.cpp:

+        (WTF::AutomaticThread::start):

+        (WTF::AutomaticThread::threadIsStopping):

+        (WTF::AutomaticThread::ThreadScope::ThreadScope): Deleted.

+        (WTF::AutomaticThread::ThreadScope::~ThreadScope): Deleted.

+        (WTF::AutomaticThread::threadWillStop): Deleted.

+        * wtf/AutomaticThread.h:

+

</ins><span class="cx"> 2017-01-04  Darin Adler  &lt;darin@apple.com&gt;

</span><span class="cx"> 

</span><span class="cx">         Remove PassRefPtr use from the &quot;html&quot; directory, other improvements

</span></span></pre></div>

<a id="trunkSourceWTFwtfAutomaticThreadcpp"></a>

<div class="modfile"><h4>Modified: trunk/Source/WTF/wtf/AutomaticThread.cpp (210397 => 210398)</h4>

<pre class="diff"><span>

<span class="info">--- trunk/Source/WTF/wtf/AutomaticThread.cpp        2017-01-05 23:50:00 UTC (rev 210397)

+++ trunk/Source/WTF/wtf/AutomaticThread.cpp        2017-01-06 00:24:11 UTC (rev 210398)

</span><span class="lines">@@ -1,5 +1,5 @@

</span><span class="cx"> /*

</del><ins>+ * Copyright (C) 2016-2017 Apple Inc. All rights reserved.

</ins><span class="cx">  *

</span><span class="cx">  * Redistribution and use in source and binary forms, with or without

</span><span class="cx">  * modification, are permitted provided that the following conditions

</span><span class="lines">@@ -147,26 +147,6 @@

</span><span class="cx">         m_isRunningCondition.wait(*m_lock);

</span><span class="cx"> }

</span><span class="cx"> 

</span><del>-class AutomaticThread::ThreadScope {

-public:

-    ThreadScope(RefPtr&lt;AutomaticThread&gt; thread)

-        : m_thread(thread)

-    {

-        m_thread-&gt;threadDidStart();

-    }

-    

-    ~ThreadScope()

-    {

-        m_thread-&gt;threadWillStop();

-        

-        LockHolder locker(*m_thread-&gt;m_lock);

-        m_thread-&gt;m_hasUnderlyingThread = false;

-    }

-

-private:

-    RefPtr&lt;AutomaticThread&gt; m_thread;

-};

-

</del><span class="cx"> void AutomaticThread::start(const LockHolder&amp;)

</span><span class="cx"> {

</span><span class="cx">     RELEASE_ASSERT(m_isRunning);

</span><span class="lines">@@ -180,18 +160,30 @@

</span><span class="cx">         [=] () {

</span><span class="cx">             if (verbose)

</span><span class="cx">                 dataLog(RawPointer(this), &quot;: Running automatic thread!\n&quot;);

</span><del>-            ThreadScope threadScope(preserveThisForThread);

</del><span class="cx">             

</span><ins>+            RefPtr&lt;AutomaticThread&gt; thread = preserveThisForThread;

+            thread-&gt;threadDidStart();

+            

</ins><span class="cx">             if (!ASSERT_DISABLED) {

</span><span class="cx">                 LockHolder locker(*m_lock);

</span><span class="cx">                 ASSERT(m_condition-&gt;contains(locker, this));

</span><span class="cx">             }

</span><span class="cx">             

</span><del>-            auto stop = [&amp;] (const LockHolder&amp;) {

</del><ins>+            auto stopImpl = [&amp;] (const LockHolder&amp; locker) {

+                thread-&gt;threadIsStopping(locker);

+                thread-&gt;m_hasUnderlyingThread = false;

+            };

+            

+            auto stopPermanently = [&amp;] (const LockHolder&amp; locker) {

</ins><span class="cx">                 m_isRunning = false;

</span><span class="cx">                 m_isRunningCondition.notifyAll();

</span><ins>+                stopImpl(locker);

</ins><span class="cx">             };

</span><span class="cx">             

</span><ins>+            auto stopForTimeout = [&amp;] (const LockHolder&amp; locker) {

+                stopImpl(locker);

+            };

+            

</ins><span class="cx">             for (;;) {

</span><span class="cx">                 {

</span><span class="cx">                     LockHolder locker(*m_lock);

</span><span class="lines">@@ -200,7 +192,7 @@

</span><span class="cx">                         if (result == PollResult::Work)

</span><span class="cx">                             break;

</span><span class="cx">                         if (result == PollResult::Stop)

</span><del>-                            return stop(locker);

</del><ins>+                            return stopPermanently(locker);

</ins><span class="cx">                         RELEASE_ASSERT(result == PollResult::Wait);

</span><span class="cx">                         // Shut the thread down after one second.

</span><span class="cx">                         m_isWaiting = true;

</span><span class="lines">@@ -212,7 +204,10 @@

</span><span class="cx">                             m_isWaiting = false;

</span><span class="cx">                             if (verbose)

</span><span class="cx">                                 dataLog(RawPointer(this), &quot;: Going to sleep!\n&quot;);

</span><del>-                            return;

</del><ins>+                            // It's important that we don't release the lock until we have completely

+                            // indicated that the thread is kaput. Otherwise we'll have a a notify

+                            // race that manifests as a deadlock on VM shutdown.

+                            return stopForTimeout(locker);

</ins><span class="cx">                         }

</span><span class="cx">                     }

</span><span class="cx">                 }

</span><span class="lines">@@ -220,7 +215,7 @@

</span><span class="cx">                 WorkResult result = work();

</span><span class="cx">                 if (result == WorkResult::Stop) {

</span><span class="cx">                     LockHolder locker(*m_lock);

</span><del>-                    return stop(locker);

</del><ins>+                    return stopPermanently(locker);

</ins><span class="cx">                 }

</span><span class="cx">                 RELEASE_ASSERT(result == WorkResult::Continue);

</span><span class="cx">             }

</span><span class="lines">@@ -232,7 +227,7 @@

</span><span class="cx"> {

</span><span class="cx"> }

</span><span class="cx"> 

</span><del>-void AutomaticThread::threadWillStop()

</del><ins>+void AutomaticThread::threadIsStopping(const LockHolder&amp;)

</ins><span class="cx"> {

</span><span class="cx"> }

</span><span class="cx"> 

</span></span></pre></div>

<a id="trunkSourceWTFwtfAutomaticThreadh"></a>

<div class="modfile"><h4>Modified: trunk/Source/WTF/wtf/AutomaticThread.h (210397 => 210398)</h4>

<pre class="diff"><span>

<span class="info">--- trunk/Source/WTF/wtf/AutomaticThread.h        2017-01-05 23:50:00 UTC (rev 210397)

+++ trunk/Source/WTF/wtf/AutomaticThread.h        2017-01-06 00:24:11 UTC (rev 210398)

</span><span class="lines">@@ -1,5 +1,5 @@

</span><span class="cx"> /*

</del><ins>+ * Copyright (C) 2016-2017 Apple Inc. All rights reserved.

</ins><span class="cx">  *

</span><span class="cx">  * Redistribution and use in source and binary forms, with or without

</span><span class="cx">  * modification, are permitted provided that the following conditions

</span><span class="lines">@@ -168,14 +168,11 @@

</span><span class="cx">     // when the thread dies. These methods let you do this. You can override these methods, and you

</span><span class="cx">     // can be sure that the default ones don't do anything (so you don't need a super call).

</span><span class="cx">     virtual void threadDidStart();

</span><del>-    virtual void threadWillStop();

</del><ins>+    virtual void threadIsStopping(const LockHolder&amp;);

</ins><span class="cx">     

</span><span class="cx"> private:

</span><span class="cx">     friend class AutomaticThreadCondition;

</span><span class="cx">     

</span><del>-    class ThreadScope;

-    friend class ThreadScope;

-    

</del><span class="cx">     void start(const LockHolder&amp;);

</span><span class="cx">     

</span><span class="cx">     Box&lt;Lock&gt; m_lock;

</span></span></pre>

</div>

</div>

</body>

</html>