- Revision
- 210398
- Author
- [email protected]
- Date
- 2017-01-05 16:24:11 -0800 (Thu, 05 Jan 2017)
Log Message
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/_javascript_Core:
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:
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (210397 => 210398)
--- trunk/Source/_javascript_Core/ChangeLog 2017-01-05 23:50:00 UTC (rev 210397)
+++ trunk/Source/_javascript_Core/ChangeLog 2017-01-06 00:24:11 UTC (rev 210398)
@@ -1,3 +1,14 @@
+2017-01-05 Filip Pizlo <[email protected]>
+
+ 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:
+
2017-01-05 Per Arne Vollan <[email protected]>
[Win] Compile error.
Modified: trunk/Source/_javascript_Core/dfg/DFGWorklist.cpp (210397 => 210398)
--- trunk/Source/_javascript_Core/dfg/DFGWorklist.cpp 2017-01-05 23:50:00 UTC (rev 210397)
+++ trunk/Source/_javascript_Core/dfg/DFGWorklist.cpp 2017-01-06 00:24:11 UTC (rev 210398)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2013-2014, 2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2013-2017 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -150,8 +150,10 @@
m_longLivedState = std::make_unique<LongLivedState>();
}
- void threadWillStop() override
+ void threadIsStopping(const LockHolder&) override
{
+ // We're holding the Worklist::m_lock, so we should be careful not to deadlock.
+
if (Options::verboseCompilationQueue())
dataLog(m_worklist, ": Thread will stop\n");
Modified: trunk/Source/WTF/ChangeLog (210397 => 210398)
--- 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)
@@ -1,3 +1,27 @@
+2017-01-05 Filip Pizlo <[email protected]>
+
+ 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:
+
2017-01-04 Darin Adler <[email protected]>
Remove PassRefPtr use from the "html" directory, other improvements
Modified: trunk/Source/WTF/wtf/AutomaticThread.cpp (210397 => 210398)
--- 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)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2016-2017 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -147,26 +147,6 @@
m_isRunningCondition.wait(*m_lock);
}
-class AutomaticThread::ThreadScope {
-public:
- ThreadScope(RefPtr<AutomaticThread> thread)
- : m_thread(thread)
- {
- m_thread->threadDidStart();
- }
-
- ~ThreadScope()
- {
- m_thread->threadWillStop();
-
- LockHolder locker(*m_thread->m_lock);
- m_thread->m_hasUnderlyingThread = false;
- }
-
-private:
- RefPtr<AutomaticThread> m_thread;
-};
-
void AutomaticThread::start(const LockHolder&)
{
RELEASE_ASSERT(m_isRunning);
@@ -180,18 +160,30 @@
[=] () {
if (verbose)
dataLog(RawPointer(this), ": Running automatic thread!\n");
- ThreadScope threadScope(preserveThisForThread);
+ RefPtr<AutomaticThread> thread = preserveThisForThread;
+ thread->threadDidStart();
+
if (!ASSERT_DISABLED) {
LockHolder locker(*m_lock);
ASSERT(m_condition->contains(locker, this));
}
- auto stop = [&] (const LockHolder&) {
+ auto stopImpl = [&] (const LockHolder& locker) {
+ thread->threadIsStopping(locker);
+ thread->m_hasUnderlyingThread = false;
+ };
+
+ auto stopPermanently = [&] (const LockHolder& locker) {
m_isRunning = false;
m_isRunningCondition.notifyAll();
+ stopImpl(locker);
};
+ auto stopForTimeout = [&] (const LockHolder& locker) {
+ stopImpl(locker);
+ };
+
for (;;) {
{
LockHolder locker(*m_lock);
@@ -200,7 +192,7 @@
if (result == PollResult::Work)
break;
if (result == PollResult::Stop)
- return stop(locker);
+ return stopPermanently(locker);
RELEASE_ASSERT(result == PollResult::Wait);
// Shut the thread down after one second.
m_isWaiting = true;
@@ -212,7 +204,10 @@
m_isWaiting = false;
if (verbose)
dataLog(RawPointer(this), ": Going to sleep!\n");
- return;
+ // 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);
}
}
}
@@ -220,7 +215,7 @@
WorkResult result = work();
if (result == WorkResult::Stop) {
LockHolder locker(*m_lock);
- return stop(locker);
+ return stopPermanently(locker);
}
RELEASE_ASSERT(result == WorkResult::Continue);
}
@@ -232,7 +227,7 @@
{
}
-void AutomaticThread::threadWillStop()
+void AutomaticThread::threadIsStopping(const LockHolder&)
{
}
Modified: trunk/Source/WTF/wtf/AutomaticThread.h (210397 => 210398)
--- 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)
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2016-2017 Apple Inc. All rights reserved.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions
@@ -168,14 +168,11 @@
// when the thread dies. These methods let you do this. You can override these methods, and you
// can be sure that the default ones don't do anything (so you don't need a super call).
virtual void threadDidStart();
- virtual void threadWillStop();
+ virtual void threadIsStopping(const LockHolder&);
private:
friend class AutomaticThreadCondition;
- class ThreadScope;
- friend class ThreadScope;
-
void start(const LockHolder&);
Box<Lock> m_lock;
