Diff
Modified: branches/safari-603-branch/Source/_javascript_Core/ChangeLog (210632 => 210633)
--- branches/safari-603-branch/Source/_javascript_Core/ChangeLog 2017-01-12 16:07:02 UTC (rev 210632)
+++ branches/safari-603-branch/Source/_javascript_Core/ChangeLog 2017-01-12 16:44:36 UTC (rev 210633)
@@ -1,3 +1,18 @@
+2017-01-11 Matthew Hanson <[email protected]>
+
+ Merge r210398. rdar://problem/29229439
+
+ 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-10 Matthew Hanson <[email protected]>
Rollout r210336. rdar://problem/29912353
Modified: branches/safari-603-branch/Source/_javascript_Core/dfg/DFGWorklist.cpp (210632 => 210633)
--- branches/safari-603-branch/Source/_javascript_Core/dfg/DFGWorklist.cpp 2017-01-12 16:07:02 UTC (rev 210632)
+++ branches/safari-603-branch/Source/_javascript_Core/dfg/DFGWorklist.cpp 2017-01-12 16:44:36 UTC (rev 210633)
@@ -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: branches/safari-603-branch/Source/WTF/ChangeLog (210632 => 210633)
--- branches/safari-603-branch/Source/WTF/ChangeLog 2017-01-12 16:07:02 UTC (rev 210632)
+++ branches/safari-603-branch/Source/WTF/ChangeLog 2017-01-12 16:44:36 UTC (rev 210633)
@@ -1,3 +1,31 @@
+2017-01-11 Matthew Hanson <[email protected]>
+
+ Merge r210398. rdar://problem/29229439
+
+ 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:
+
2016-12-19 Babak Shafiei <[email protected]>
Merge r210010.
Modified: branches/safari-603-branch/Source/WTF/wtf/AutomaticThread.cpp (210632 => 210633)
--- branches/safari-603-branch/Source/WTF/wtf/AutomaticThread.cpp 2017-01-12 16:07:02 UTC (rev 210632)
+++ branches/safari-603-branch/Source/WTF/wtf/AutomaticThread.cpp 2017-01-12 16:44:36 UTC (rev 210633)
@@ -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: branches/safari-603-branch/Source/WTF/wtf/AutomaticThread.h (210632 => 210633)
--- branches/safari-603-branch/Source/WTF/wtf/AutomaticThread.h 2017-01-12 16:07:02 UTC (rev 210632)
+++ branches/safari-603-branch/Source/WTF/wtf/AutomaticThread.h 2017-01-12 16:44:36 UTC (rev 210633)
@@ -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;
