Title: [259683] trunk
- Revision
- 259683
- Author
- [email protected]
- Date
- 2020-04-07 16:24:52 -0700 (Tue, 07 Apr 2020)
Log Message
[JSC] Collect-continuously thread should take m_collectContinuouslyLock while it is waking up concurrent collector thread
https://bugs.webkit.org/show_bug.cgi?id=210163
Reviewed by Saam Barati.
JSTests:
* stress/collect-continuously-should-not-wake-concurrent-collector-after-prevent-collection-is-called.js: Added.
(let.theCode):
Source/_javascript_Core:
Collect-Continuously thread has fancy race issue.
In Heap::preventCollection, we first take m_collectContinuouslyLock to ensure collect-continuously thread is not working, and then
we ensure collector thread is stopped by using waitForCollector. However our collect-continuously thread is implemented like this.
while (!m_shouldStopCollectingContinuously) {
{ // (A)
LockHolder locker(*m_threadLock);
if (m_requests.isEmpty()) {
m_requests.append(WTF::nullopt);
m_lastGrantedTicket++;
m_threadCondition->notifyOne(locker); // (B) WAKING UP concurrent collector thread.
}
}
{
LockHolder locker(m_collectContinuouslyLock);
...
while (!hasElapsed(timeToWakeUp) && !m_shouldStopCollectingContinuously)
m_collectContinuouslyCondition.waitUntil(m_collectContinuouslyLock, timeToWakeUp);
}
}
Even if m_collectContinuouslyLock is taken, collect-continuously thread is still able to wake up concurrent collector thread
since (B)'s code is not guarded by m_collectContinuouslyLock. The following sequence can happen,
1. The main thread calls Heap::preventCollection to ensure all collection is stopped.
2. The collect-continuously thread is at (A) point.
3. The main thread takes m_collectContinuouslyLock. This is OK.
4. The main thread calls waitForCollector to ensure that concurrent collector thread is stopped.
5. The collect-continuously thread executes (B). It is allowed since this is not guarded by m_collectContinuouslyLock. So, concurrent collector starts working.
6. While the main thread called Heap::preventCollection, concurrent collector starts collection!
We should guard (A)'s block with m_collectContinuouslyLock too.
* heap/Heap.cpp:
(JSC::Heap::notifyIsSafeToCollect):
Modified Paths
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (259682 => 259683)
--- trunk/JSTests/ChangeLog 2020-04-07 23:12:58 UTC (rev 259682)
+++ trunk/JSTests/ChangeLog 2020-04-07 23:24:52 UTC (rev 259683)
@@ -1,3 +1,13 @@
+2020-04-07 Yusuke Suzuki <[email protected]>
+
+ [JSC] Collect-continuously thread should take m_collectContinuouslyLock while it is waking up concurrent collector thread
+ https://bugs.webkit.org/show_bug.cgi?id=210163
+
+ Reviewed by Saam Barati.
+
+ * stress/collect-continuously-should-not-wake-concurrent-collector-after-prevent-collection-is-called.js: Added.
+ (let.theCode):
+
2020-04-07 Saam Barati <[email protected]>
Delete ICs can't cache dictionaries
Added: trunk/JSTests/stress/collect-continuously-should-not-wake-concurrent-collector-after-prevent-collection-is-called.js (0 => 259683)
--- trunk/JSTests/stress/collect-continuously-should-not-wake-concurrent-collector-after-prevent-collection-is-called.js (rev 0)
+++ trunk/JSTests/stress/collect-continuously-should-not-wake-concurrent-collector-after-prevent-collection-is-called.js 2020-04-07 23:24:52 UTC (rev 259683)
@@ -0,0 +1,22 @@
+//@ runDefault("--maxPerThreadStackUsage=1572864", "--forceMiniVMMode=1", "--stealEmptyBlocksFromOtherAllocators=0", "--collectContinuously=1", "--watchdog=3000", "--watchdog-exception-ok")
+// Reproducing the crash with this test is very hard. You should execute something like this.
+// while true; do for x in {0..4}; do DYLD_FRAMEWORK_PATH=$VM $VM/jsc --maxPerThreadStackUsage=1572864 --forceMiniVMMode=1 --stealEmptyBlocksFromOtherAllocators=0 --collectContinuously=1 collect-continuously-should-not-wake-concurrent-collector-after-prevent-collection-is-called.js --watchdog=3000&; done; wait; sleep 0.1; done
+
+array = [];
+for (var i = 0; i < 800; ++i) {
+ array[i] = new DataView(new ArrayBuffer());
+}
+
+let theCode = `
+for (let j = 0; j < 100; j++) {
+ generateHeapSnapshotForGCDebugging();
+}
+`
+
+for (let i=0; i<5; i++) {
+ $.agent.start(theCode);
+}
+
+for (let i=0; i<3; i++) {
+ runString(theCode);
+}
Modified: trunk/Source/_javascript_Core/ChangeLog (259682 => 259683)
--- trunk/Source/_javascript_Core/ChangeLog 2020-04-07 23:12:58 UTC (rev 259682)
+++ trunk/Source/_javascript_Core/ChangeLog 2020-04-07 23:24:52 UTC (rev 259683)
@@ -1,3 +1,48 @@
+2020-04-07 Yusuke Suzuki <[email protected]>
+
+ [JSC] Collect-continuously thread should take m_collectContinuouslyLock while it is waking up concurrent collector thread
+ https://bugs.webkit.org/show_bug.cgi?id=210163
+
+ Reviewed by Saam Barati.
+
+ Collect-Continuously thread has fancy race issue.
+
+ In Heap::preventCollection, we first take m_collectContinuouslyLock to ensure collect-continuously thread is not working, and then
+ we ensure collector thread is stopped by using waitForCollector. However our collect-continuously thread is implemented like this.
+
+ while (!m_shouldStopCollectingContinuously) {
+ { // (A)
+ LockHolder locker(*m_threadLock);
+ if (m_requests.isEmpty()) {
+ m_requests.append(WTF::nullopt);
+ m_lastGrantedTicket++;
+ m_threadCondition->notifyOne(locker); // (B) WAKING UP concurrent collector thread.
+ }
+ }
+
+ {
+ LockHolder locker(m_collectContinuouslyLock);
+ ...
+ while (!hasElapsed(timeToWakeUp) && !m_shouldStopCollectingContinuously)
+ m_collectContinuouslyCondition.waitUntil(m_collectContinuouslyLock, timeToWakeUp);
+ }
+ }
+
+ Even if m_collectContinuouslyLock is taken, collect-continuously thread is still able to wake up concurrent collector thread
+ since (B)'s code is not guarded by m_collectContinuouslyLock. The following sequence can happen,
+
+ 1. The main thread calls Heap::preventCollection to ensure all collection is stopped.
+ 2. The collect-continuously thread is at (A) point.
+ 3. The main thread takes m_collectContinuouslyLock. This is OK.
+ 4. The main thread calls waitForCollector to ensure that concurrent collector thread is stopped.
+ 5. The collect-continuously thread executes (B). It is allowed since this is not guarded by m_collectContinuouslyLock. So, concurrent collector starts working.
+ 6. While the main thread called Heap::preventCollection, concurrent collector starts collection!
+
+ We should guard (A)'s block with m_collectContinuouslyLock too.
+
+ * heap/Heap.cpp:
+ (JSC::Heap::notifyIsSafeToCollect):
+
2020-04-07 Saam Barati <[email protected]>
Delete ICs can't cache dictionaries
Modified: trunk/Source/_javascript_Core/heap/Heap.cpp (259682 => 259683)
--- trunk/Source/_javascript_Core/heap/Heap.cpp 2020-04-07 23:12:58 UTC (rev 259682)
+++ trunk/Source/_javascript_Core/heap/Heap.cpp 2020-04-07 23:24:52 UTC (rev 259683)
@@ -2899,7 +2899,8 @@
[this] () {
MonotonicTime initialTime = MonotonicTime::now();
Seconds period = Seconds::fromMilliseconds(Options::collectContinuouslyPeriodMS());
- while (!m_shouldStopCollectingContinuously) {
+ while (true) {
+ LockHolder locker(m_collectContinuouslyLock);
{
LockHolder locker(*m_threadLock);
if (m_requests.isEmpty()) {
@@ -2909,17 +2910,16 @@
}
}
- {
- LockHolder locker(m_collectContinuouslyLock);
- Seconds elapsed = MonotonicTime::now() - initialTime;
- Seconds elapsedInPeriod = elapsed % period;
- MonotonicTime timeToWakeUp =
- initialTime + elapsed - elapsedInPeriod + period;
- while (!hasElapsed(timeToWakeUp) && !m_shouldStopCollectingContinuously) {
- m_collectContinuouslyCondition.waitUntil(
- m_collectContinuouslyLock, timeToWakeUp);
- }
+ Seconds elapsed = MonotonicTime::now() - initialTime;
+ Seconds elapsedInPeriod = elapsed % period;
+ MonotonicTime timeToWakeUp =
+ initialTime + elapsed - elapsedInPeriod + period;
+ while (!hasElapsed(timeToWakeUp) && !m_shouldStopCollectingContinuously) {
+ m_collectContinuouslyCondition.waitUntil(
+ m_collectContinuouslyLock, timeToWakeUp);
}
+ if (m_shouldStopCollectingContinuously)
+ break;
}
}, ThreadType::GarbageCollection);
}
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
