Skip to content

Commit 49a87d7

Browse files
saambaratisaambarati
committed
JSRunLoopTimer may run part of a member function after it's destroyed
https://bugs.webkit.org/show_bug.cgi?id=188426 Reviewed by Mark Lam. Source/JavaScriptCore: When I was reading the JSRunLoopTimer code, I noticed that it is possible to end up running timer code after the class had been destroyed. The issue I spotted was in this function: ``` void JSRunLoopTimer::timerDidFire() { JSLock* apiLock = m_apiLock.get(); if (!apiLock) { // Likely a buggy usage: the timer fired while JSRunLoopTimer was being destroyed. return; } // HERE std::lock_guard<JSLock> lock(*apiLock); RefPtr<VM> vm = apiLock->vm(); if (!vm) { // The VM has been destroyed, so we should just give up. return; } doWork(); } ``` Look at the comment 'HERE'. Let's say that the timer callback thread gets context switched before grabbing the API lock. Then, some other thread destroys the VM. And let's say that the VM owns (perhaps transitively) this timer. Then, the timer would run code and access member variables after it was destroyed. This patch fixes this issue by introducing a new timer manager class. This class manages timers on a per VM basis. When a timer is scheduled, this class refs the timer. It also calls the timer callback while actively maintaining a +1 ref to it. So, it's no longer possible to call the timer callback after the timer has been destroyed. However, calling a timer callback can still race with the VM being destroyed. We continue to detect this case and bail out of the callback early. This patch also removes a lot of duplicate code between GCActivityCallback and JSRunLoopTimer. * heap/EdenGCActivityCallback.cpp: (JSC::EdenGCActivityCallback::doCollection): (JSC::EdenGCActivityCallback::lastGCLength): (JSC::EdenGCActivityCallback::deathRate): * heap/EdenGCActivityCallback.h: * heap/FullGCActivityCallback.cpp: (JSC::FullGCActivityCallback::doCollection): (JSC::FullGCActivityCallback::lastGCLength): (JSC::FullGCActivityCallback::deathRate): * heap/FullGCActivityCallback.h: * heap/GCActivityCallback.cpp: (JSC::GCActivityCallback::doWork): (JSC::GCActivityCallback::scheduleTimer): (JSC::GCActivityCallback::didAllocate): (JSC::GCActivityCallback::willCollect): (JSC::GCActivityCallback::cancel): (JSC::GCActivityCallback::cancelTimer): Deleted. (JSC::GCActivityCallback::nextFireTime): Deleted. * heap/GCActivityCallback.h: * heap/Heap.cpp: (JSC::Heap::reportAbandonedObjectGraph): (JSC::Heap::notifyIncrementalSweeper): (JSC::Heap::updateAllocationLimits): (JSC::Heap::didAllocate): * heap/IncrementalSweeper.cpp: (JSC::IncrementalSweeper::scheduleTimer): (JSC::IncrementalSweeper::doWork): (JSC::IncrementalSweeper::doSweep): (JSC::IncrementalSweeper::sweepNextBlock): (JSC::IncrementalSweeper::startSweeping): (JSC::IncrementalSweeper::stopSweeping): * heap/IncrementalSweeper.h: * heap/StopIfNecessaryTimer.cpp: (JSC::StopIfNecessaryTimer::doWork): (JSC::StopIfNecessaryTimer::scheduleSoon): * heap/StopIfNecessaryTimer.h: * runtime/JSRunLoopTimer.cpp: (JSC::epochTime): (JSC::JSRunLoopTimer::Manager::timerDidFireCallback): (JSC::JSRunLoopTimer::Manager::PerVMData::setRunLoop): (JSC::JSRunLoopTimer::Manager::PerVMData::PerVMData): (JSC::JSRunLoopTimer::Manager::PerVMData::~PerVMData): (JSC::JSRunLoopTimer::Manager::timerDidFire): (JSC::JSRunLoopTimer::Manager::shared): (JSC::JSRunLoopTimer::Manager::registerVM): (JSC::JSRunLoopTimer::Manager::unregisterVM): (JSC::JSRunLoopTimer::Manager::scheduleTimer): (JSC::JSRunLoopTimer::Manager::cancelTimer): (JSC::JSRunLoopTimer::Manager::timeUntilFire): (JSC::JSRunLoopTimer::Manager::didChangeRunLoop): (JSC::JSRunLoopTimer::timerDidFire): (JSC::JSRunLoopTimer::JSRunLoopTimer): (JSC::JSRunLoopTimer::timeUntilFire): (JSC::JSRunLoopTimer::setTimeUntilFire): (JSC::JSRunLoopTimer::cancelTimer): (JSC::JSRunLoopTimer::setRunLoop): Deleted. (JSC::JSRunLoopTimer::timerDidFireCallback): Deleted. (JSC::JSRunLoopTimer::scheduleTimer): Deleted. * runtime/JSRunLoopTimer.h: (JSC::JSRunLoopTimer::Manager::PerVMData::PerVMData): * runtime/PromiseDeferredTimer.cpp: (JSC::PromiseDeferredTimer::doWork): (JSC::PromiseDeferredTimer::runRunLoop): (JSC::PromiseDeferredTimer::addPendingPromise): (JSC::PromiseDeferredTimer::hasPendingPromise): (JSC::PromiseDeferredTimer::hasDependancyInPendingPromise): (JSC::PromiseDeferredTimer::cancelPendingPromise): (JSC::PromiseDeferredTimer::scheduleWorkSoon): * runtime/PromiseDeferredTimer.h: * runtime/VM.cpp: (JSC::VM::VM): (JSC::VM::~VM): (JSC::VM::setRunLoop): (JSC::VM::registerRunLoopTimer): Deleted. (JSC::VM::unregisterRunLoopTimer): Deleted. * runtime/VM.h: (JSC::VM::runLoop const): * wasm/js/WebAssemblyPrototype.cpp: (JSC::webAssemblyModuleValidateAsyncInternal): (JSC::instantiate): (JSC::compileAndInstantiate): (JSC::webAssemblyModuleInstantinateAsyncInternal): (JSC::webAssemblyCompileStreamingInternal): (JSC::webAssemblyInstantiateStreamingInternal): Source/WebCore: * page/cocoa/ResourceUsageThreadCocoa.mm: (WebCore::ResourceUsageThread::platformThreadBody): * page/linux/ResourceUsageThreadLinux.cpp: (WebCore::ResourceUsageThread::platformThreadBody): Canonical link: https://commits.webkit.org/203824@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@235107 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent 8561c67 commit 49a87d7

22 files changed

Lines changed: 559 additions & 246 deletions

‎Source/JavaScriptCore/ChangeLog‎

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,135 @@
1+
2018-08-21 Saam barati <sbarati@apple.com>
2+
3+
JSRunLoopTimer may run part of a member function after it's destroyed
4+
https://bugs.webkit.org/show_bug.cgi?id=188426
5+
6+
Reviewed by Mark Lam.
7+
8+
When I was reading the JSRunLoopTimer code, I noticed that it is possible
9+
to end up running timer code after the class had been destroyed.
10+
11+
The issue I spotted was in this function:
12+
```
13+
void JSRunLoopTimer::timerDidFire()
14+
{
15+
JSLock* apiLock = m_apiLock.get();
16+
if (!apiLock) {
17+
// Likely a buggy usage: the timer fired while JSRunLoopTimer was being destroyed.
18+
return;
19+
}
20+
// HERE
21+
std::lock_guard<JSLock> lock(*apiLock);
22+
RefPtr<VM> vm = apiLock->vm();
23+
if (!vm) {
24+
// The VM has been destroyed, so we should just give up.
25+
return;
26+
}
27+
28+
doWork();
29+
}
30+
```
31+
32+
Look at the comment 'HERE'. Let's say that the timer callback thread gets context
33+
switched before grabbing the API lock. Then, some other thread destroys the VM.
34+
And let's say that the VM owns (perhaps transitively) this timer. Then, the
35+
timer would run code and access member variables after it was destroyed.
36+
37+
This patch fixes this issue by introducing a new timer manager class.
38+
This class manages timers on a per VM basis. When a timer is scheduled,
39+
this class refs the timer. It also calls the timer callback while actively
40+
maintaining a +1 ref to it. So, it's no longer possible to call the timer
41+
callback after the timer has been destroyed. However, calling a timer callback
42+
can still race with the VM being destroyed. We continue to detect this case and
43+
bail out of the callback early.
44+
45+
This patch also removes a lot of duplicate code between GCActivityCallback
46+
and JSRunLoopTimer.
47+
48+
* heap/EdenGCActivityCallback.cpp:
49+
(JSC::EdenGCActivityCallback::doCollection):
50+
(JSC::EdenGCActivityCallback::lastGCLength):
51+
(JSC::EdenGCActivityCallback::deathRate):
52+
* heap/EdenGCActivityCallback.h:
53+
* heap/FullGCActivityCallback.cpp:
54+
(JSC::FullGCActivityCallback::doCollection):
55+
(JSC::FullGCActivityCallback::lastGCLength):
56+
(JSC::FullGCActivityCallback::deathRate):
57+
* heap/FullGCActivityCallback.h:
58+
* heap/GCActivityCallback.cpp:
59+
(JSC::GCActivityCallback::doWork):
60+
(JSC::GCActivityCallback::scheduleTimer):
61+
(JSC::GCActivityCallback::didAllocate):
62+
(JSC::GCActivityCallback::willCollect):
63+
(JSC::GCActivityCallback::cancel):
64+
(JSC::GCActivityCallback::cancelTimer): Deleted.
65+
(JSC::GCActivityCallback::nextFireTime): Deleted.
66+
* heap/GCActivityCallback.h:
67+
* heap/Heap.cpp:
68+
(JSC::Heap::reportAbandonedObjectGraph):
69+
(JSC::Heap::notifyIncrementalSweeper):
70+
(JSC::Heap::updateAllocationLimits):
71+
(JSC::Heap::didAllocate):
72+
* heap/IncrementalSweeper.cpp:
73+
(JSC::IncrementalSweeper::scheduleTimer):
74+
(JSC::IncrementalSweeper::doWork):
75+
(JSC::IncrementalSweeper::doSweep):
76+
(JSC::IncrementalSweeper::sweepNextBlock):
77+
(JSC::IncrementalSweeper::startSweeping):
78+
(JSC::IncrementalSweeper::stopSweeping):
79+
* heap/IncrementalSweeper.h:
80+
* heap/StopIfNecessaryTimer.cpp:
81+
(JSC::StopIfNecessaryTimer::doWork):
82+
(JSC::StopIfNecessaryTimer::scheduleSoon):
83+
* heap/StopIfNecessaryTimer.h:
84+
* runtime/JSRunLoopTimer.cpp:
85+
(JSC::epochTime):
86+
(JSC::JSRunLoopTimer::Manager::timerDidFireCallback):
87+
(JSC::JSRunLoopTimer::Manager::PerVMData::setRunLoop):
88+
(JSC::JSRunLoopTimer::Manager::PerVMData::PerVMData):
89+
(JSC::JSRunLoopTimer::Manager::PerVMData::~PerVMData):
90+
(JSC::JSRunLoopTimer::Manager::timerDidFire):
91+
(JSC::JSRunLoopTimer::Manager::shared):
92+
(JSC::JSRunLoopTimer::Manager::registerVM):
93+
(JSC::JSRunLoopTimer::Manager::unregisterVM):
94+
(JSC::JSRunLoopTimer::Manager::scheduleTimer):
95+
(JSC::JSRunLoopTimer::Manager::cancelTimer):
96+
(JSC::JSRunLoopTimer::Manager::timeUntilFire):
97+
(JSC::JSRunLoopTimer::Manager::didChangeRunLoop):
98+
(JSC::JSRunLoopTimer::timerDidFire):
99+
(JSC::JSRunLoopTimer::JSRunLoopTimer):
100+
(JSC::JSRunLoopTimer::timeUntilFire):
101+
(JSC::JSRunLoopTimer::setTimeUntilFire):
102+
(JSC::JSRunLoopTimer::cancelTimer):
103+
(JSC::JSRunLoopTimer::setRunLoop): Deleted.
104+
(JSC::JSRunLoopTimer::timerDidFireCallback): Deleted.
105+
(JSC::JSRunLoopTimer::scheduleTimer): Deleted.
106+
* runtime/JSRunLoopTimer.h:
107+
(JSC::JSRunLoopTimer::Manager::PerVMData::PerVMData):
108+
* runtime/PromiseDeferredTimer.cpp:
109+
(JSC::PromiseDeferredTimer::doWork):
110+
(JSC::PromiseDeferredTimer::runRunLoop):
111+
(JSC::PromiseDeferredTimer::addPendingPromise):
112+
(JSC::PromiseDeferredTimer::hasPendingPromise):
113+
(JSC::PromiseDeferredTimer::hasDependancyInPendingPromise):
114+
(JSC::PromiseDeferredTimer::cancelPendingPromise):
115+
(JSC::PromiseDeferredTimer::scheduleWorkSoon):
116+
* runtime/PromiseDeferredTimer.h:
117+
* runtime/VM.cpp:
118+
(JSC::VM::VM):
119+
(JSC::VM::~VM):
120+
(JSC::VM::setRunLoop):
121+
(JSC::VM::registerRunLoopTimer): Deleted.
122+
(JSC::VM::unregisterRunLoopTimer): Deleted.
123+
* runtime/VM.h:
124+
(JSC::VM::runLoop const):
125+
* wasm/js/WebAssemblyPrototype.cpp:
126+
(JSC::webAssemblyModuleValidateAsyncInternal):
127+
(JSC::instantiate):
128+
(JSC::compileAndInstantiate):
129+
(JSC::webAssemblyModuleInstantinateAsyncInternal):
130+
(JSC::webAssemblyCompileStreamingInternal):
131+
(JSC::webAssemblyInstantiateStreamingInternal):
132+
1133
2018-08-20 Saam barati <sbarati@apple.com>
2134

3135
Inline DataView accesses into DFG/FTL

‎Source/JavaScriptCore/heap/EdenGCActivityCallback.cpp‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -36,21 +36,20 @@ EdenGCActivityCallback::EdenGCActivityCallback(Heap* heap)
3636
{
3737
}
3838

39-
void EdenGCActivityCallback::doCollection()
39+
void EdenGCActivityCallback::doCollection(VM& vm)
4040
{
41-
m_vm->heap.collectAsync(CollectionScope::Eden);
41+
vm.heap.collectAsync(CollectionScope::Eden);
4242
}
4343

44-
Seconds EdenGCActivityCallback::lastGCLength()
44+
Seconds EdenGCActivityCallback::lastGCLength(Heap& heap)
4545
{
46-
return m_vm->heap.lastEdenGCLength();
46+
return heap.lastEdenGCLength();
4747
}
4848

49-
double EdenGCActivityCallback::deathRate()
49+
double EdenGCActivityCallback::deathRate(Heap& heap)
5050
{
51-
Heap* heap = &m_vm->heap;
52-
size_t sizeBefore = heap->sizeBeforeLastEdenCollection();
53-
size_t sizeAfter = heap->sizeAfterLastEdenCollection();
51+
size_t sizeBefore = heap.sizeBeforeLastEdenCollection();
52+
size_t sizeAfter = heap.sizeAfterLastEdenCollection();
5453
if (!sizeBefore)
5554
return 1.0;
5655
if (sizeAfter > sizeBefore) {

‎Source/JavaScriptCore/heap/EdenGCActivityCallback.h‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,12 +33,12 @@ class JS_EXPORT_PRIVATE EdenGCActivityCallback : public GCActivityCallback {
3333
public:
3434
EdenGCActivityCallback(Heap*);
3535

36-
void doCollection() override;
36+
void doCollection(VM&) override;
3737

3838
protected:
39-
Seconds lastGCLength() override;
39+
Seconds lastGCLength(Heap&) override;
4040
double gcTimeSlice(size_t bytes) override;
41-
double deathRate() override;
41+
double deathRate(Heap&) override;
4242
};
4343

4444
inline RefPtr<GCActivityCallback> GCActivityCallback::createEdenTimer(Heap* heap)

‎Source/JavaScriptCore/heap/FullGCActivityCallback.cpp‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -39,9 +39,9 @@ FullGCActivityCallback::FullGCActivityCallback(Heap* heap)
3939
{
4040
}
4141

42-
void FullGCActivityCallback::doCollection()
42+
void FullGCActivityCallback::doCollection(VM& vm)
4343
{
44-
Heap& heap = m_vm->heap;
44+
Heap& heap = vm.heap;
4545
m_didGCRecently = false;
4646

4747
#if !PLATFORM(IOS)
@@ -56,16 +56,15 @@ void FullGCActivityCallback::doCollection()
5656
heap.collectAsync(CollectionScope::Full);
5757
}
5858

59-
Seconds FullGCActivityCallback::lastGCLength()
59+
Seconds FullGCActivityCallback::lastGCLength(Heap& heap)
6060
{
61-
return m_vm->heap.lastFullGCLength();
61+
return heap.lastFullGCLength();
6262
}
6363

64-
double FullGCActivityCallback::deathRate()
64+
double FullGCActivityCallback::deathRate(Heap& heap)
6565
{
66-
Heap* heap = &m_vm->heap;
67-
size_t sizeBefore = heap->sizeBeforeLastFullCollection();
68-
size_t sizeAfter = heap->sizeAfterLastFullCollection();
66+
size_t sizeBefore = heap.sizeBeforeLastFullCollection();
67+
size_t sizeAfter = heap.sizeAfterLastFullCollection();
6968
if (!sizeBefore)
7069
return 1.0;
7170
if (sizeAfter > sizeBefore) {

‎Source/JavaScriptCore/heap/FullGCActivityCallback.h‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,15 +33,15 @@ class JS_EXPORT_PRIVATE FullGCActivityCallback : public GCActivityCallback {
3333
public:
3434
FullGCActivityCallback(Heap*);
3535

36-
void doCollection() override;
36+
void doCollection(VM&) override;
3737

3838
bool didGCRecently() const { return m_didGCRecently; }
3939
void setDidGCRecently() { m_didGCRecently = true; }
4040

4141
protected:
42-
Seconds lastGCLength() override;
42+
Seconds lastGCLength(Heap&) override;
4343
double gcTimeSlice(size_t bytes) override;
44-
double deathRate() override;
44+
double deathRate(Heap&) override;
4545

4646
bool m_didGCRecently { false };
4747
};

‎Source/JavaScriptCore/heap/GCActivityCallback.cpp‎

Lines changed: 15 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (C) 2010-2017 Apple Inc. All rights reserved.
2+
* Copyright (C) 2010-2018 Apple Inc. All rights reserved.
33
*
44
* Redistribution and use in source and binary forms, with or without
55
* modification, are permitted provided that the following conditions
@@ -45,83 +45,52 @@ GCActivityCallback::GCActivityCallback(Heap* heap)
4545
{
4646
}
4747

48-
void GCActivityCallback::doWork()
48+
void GCActivityCallback::doWork(VM& vm)
4949
{
50-
Heap* heap = &m_vm->heap;
5150
if (!isEnabled())
5251
return;
5352

54-
JSLockHolder locker(m_vm);
55-
if (heap->isDeferred()) {
53+
ASSERT(vm.currentThreadIsHoldingAPILock());
54+
Heap& heap = vm.heap;
55+
if (heap.isDeferred()) {
5656
scheduleTimer(0_s);
5757
return;
5858
}
5959

60-
doCollection();
60+
doCollection(vm);
6161
}
6262

63-
#if USE(CF)
6463
void GCActivityCallback::scheduleTimer(Seconds newDelay)
6564
{
6665
if (newDelay * timerSlop > m_delay)
6766
return;
6867
Seconds delta = m_delay - newDelay;
6968
m_delay = newDelay;
70-
CFRunLoopTimerSetNextFireDate(m_timer.get(), CFRunLoopTimerGetNextFireDate(m_timer.get()) - delta.seconds());
69+
if (auto timeUntilFire = this->timeUntilFire())
70+
setTimeUntilFire(*timeUntilFire - delta);
71+
else
72+
setTimeUntilFire(delta);
7173
}
7274

73-
void GCActivityCallback::cancelTimer()
74-
{
75-
m_delay = s_decade;
76-
CFRunLoopTimerSetNextFireDate(m_timer.get(), CFAbsoluteTimeGetCurrent() + s_decade.seconds());
77-
}
78-
79-
MonotonicTime GCActivityCallback::nextFireTime()
80-
{
81-
return MonotonicTime::now() + Seconds(CFRunLoopTimerGetNextFireDate(m_timer.get()) - CFAbsoluteTimeGetCurrent());
82-
}
83-
#else
84-
void GCActivityCallback::scheduleTimer(Seconds newDelay)
85-
{
86-
if (newDelay * timerSlop > m_delay)
87-
return;
88-
Seconds delta = m_delay - newDelay;
89-
m_delay = newDelay;
90-
91-
Seconds secondsUntilFire = m_timer.secondsUntilFire();
92-
m_timer.startOneShot(std::max<Seconds>(secondsUntilFire - delta, 0_s));
93-
}
94-
95-
void GCActivityCallback::cancelTimer()
96-
{
97-
m_delay = s_decade;
98-
m_timer.startOneShot(s_decade);
99-
}
100-
101-
MonotonicTime GCActivityCallback::nextFireTime()
102-
{
103-
return MonotonicTime::now() + m_timer.secondsUntilFire();
104-
}
105-
#endif
106-
107-
void GCActivityCallback::didAllocate(size_t bytes)
75+
void GCActivityCallback::didAllocate(Heap& heap, size_t bytes)
10876
{
10977
// The first byte allocated in an allocation cycle will report 0 bytes to didAllocate.
11078
// We pretend it's one byte so that we don't ignore this allocation entirely.
11179
if (!bytes)
11280
bytes = 1;
113-
double bytesExpectedToReclaim = static_cast<double>(bytes) * deathRate();
114-
Seconds newDelay = lastGCLength() / gcTimeSlice(bytesExpectedToReclaim);
81+
double bytesExpectedToReclaim = static_cast<double>(bytes) * deathRate(heap);
82+
Seconds newDelay = lastGCLength(heap) / gcTimeSlice(bytesExpectedToReclaim);
11583
scheduleTimer(newDelay);
11684
}
11785

11886
void GCActivityCallback::willCollect()
11987
{
120-
cancelTimer();
88+
cancel();
12189
}
12290

12391
void GCActivityCallback::cancel()
12492
{
93+
m_delay = s_decade;
12594
cancelTimer();
12695
}
12796

‎Source/JavaScriptCore/heap/GCActivityCallback.h‎

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -48,24 +48,22 @@ class JS_EXPORT_PRIVATE GCActivityCallback : public JSRunLoopTimer {
4848

4949
GCActivityCallback(Heap*);
5050

51-
void doWork() override;
51+
void doWork(VM&) override;
5252

53-
virtual void doCollection() = 0;
53+
virtual void doCollection(VM&) = 0;
5454

55-
virtual void didAllocate(size_t);
56-
virtual void willCollect();
57-
virtual void cancel();
55+
void didAllocate(Heap&, size_t);
56+
void willCollect();
57+
void cancel();
5858
bool isEnabled() const { return m_enabled; }
5959
void setEnabled(bool enabled) { m_enabled = enabled; }
6060

6161
static bool s_shouldCreateGCTimer;
6262

63-
MonotonicTime nextFireTime();
64-
6563
protected:
66-
virtual Seconds lastGCLength() = 0;
64+
virtual Seconds lastGCLength(Heap&) = 0;
6765
virtual double gcTimeSlice(size_t bytes) = 0;
68-
virtual double deathRate() = 0;
66+
virtual double deathRate(Heap&) = 0;
6967

7068
GCActivityCallback(VM* vm)
7169
: Base(vm)
@@ -77,7 +75,6 @@ class JS_EXPORT_PRIVATE GCActivityCallback : public JSRunLoopTimer {
7775
bool m_enabled;
7876

7977
protected:
80-
void cancelTimer();
8178
void scheduleTimer(Seconds);
8279

8380
private:

0 commit comments

Comments
 (0)