Merge r235261 - JSRunLoopTimer may run part of a member function afte…

…r 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):

Source/JavaScriptCore/ChangeLog

@@ -1,3 +1,135 @@
+2018-08-23  Saam barati  <sbarati@apple.com>
+
+        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.
+
+        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):
+
 2018-08-23  Mark Lam  <mark.lam@apple.com>
 
         Move vmEntryGlobalObject() to VM from CallFrame.

Source/JavaScriptCore/heap/EdenGCActivityCallback.cpp

@@ -36,21 +36,20 @@ EdenGCActivityCallback::EdenGCActivityCallback(Heap* heap)
 {
 }
 
-void EdenGCActivityCallback::doCollection()
+void EdenGCActivityCallback::doCollection(VM& vm)
 {
-    m_vm->heap.collectAsync(CollectionScope::Eden);
+    vm.heap.collectAsync(CollectionScope::Eden);
 }
 
-Seconds EdenGCActivityCallback::lastGCLength()
+Seconds EdenGCActivityCallback::lastGCLength(Heap& heap)
 {
-    return m_vm->heap.lastEdenGCLength();
+    return heap.lastEdenGCLength();
 }
 
-double EdenGCActivityCallback::deathRate()
+double EdenGCActivityCallback::deathRate(Heap& heap)
 {
-    Heap* heap = &m_vm->heap;
-    size_t sizeBefore = heap->sizeBeforeLastEdenCollection();
-    size_t sizeAfter = heap->sizeAfterLastEdenCollection();
+    size_t sizeBefore = heap.sizeBeforeLastEdenCollection();
+    size_t sizeAfter = heap.sizeAfterLastEdenCollection();
     if (!sizeBefore)
         return 1.0;
     if (sizeAfter > sizeBefore) {

Source/JavaScriptCore/heap/EdenGCActivityCallback.h

@@ -33,12 +33,12 @@ class JS_EXPORT_PRIVATE EdenGCActivityCallback : public GCActivityCallback {
 public:
     EdenGCActivityCallback(Heap*);
 
-    void doCollection() override;
+    void doCollection(VM&) override;
 
 protected:
-    Seconds lastGCLength() override;
+    Seconds lastGCLength(Heap&) override;
     double gcTimeSlice(size_t bytes) override;
-    double deathRate() override;
+    double deathRate(Heap&) override;
 };
 
 inline RefPtr<GCActivityCallback> GCActivityCallback::createEdenTimer(Heap* heap)

Source/JavaScriptCore/heap/FullGCActivityCallback.cpp

@@ -39,9 +39,9 @@ FullGCActivityCallback::FullGCActivityCallback(Heap* heap)
 {
 }
 
-void FullGCActivityCallback::doCollection()
+void FullGCActivityCallback::doCollection(VM& vm)
 {
-    Heap& heap = m_vm->heap;
+    Heap& heap = vm.heap;
     m_didGCRecently = false;
 
 #if !PLATFORM(IOS)
@@ -56,16 +56,15 @@ void FullGCActivityCallback::doCollection()
     heap.collectAsync(CollectionScope::Full);
 }
 
-Seconds FullGCActivityCallback::lastGCLength()
+Seconds FullGCActivityCallback::lastGCLength(Heap& heap)
 {
-    return m_vm->heap.lastFullGCLength();
+    return heap.lastFullGCLength();
 }
 
-double FullGCActivityCallback::deathRate()
+double FullGCActivityCallback::deathRate(Heap& heap)
 {
-    Heap* heap = &m_vm->heap;
-    size_t sizeBefore = heap->sizeBeforeLastFullCollection();
-    size_t sizeAfter = heap->sizeAfterLastFullCollection();
+    size_t sizeBefore = heap.sizeBeforeLastFullCollection();
+    size_t sizeAfter = heap.sizeAfterLastFullCollection();
     if (!sizeBefore)
         return 1.0;
     if (sizeAfter > sizeBefore) {

Source/JavaScriptCore/heap/FullGCActivityCallback.h

@@ -33,15 +33,15 @@ class JS_EXPORT_PRIVATE FullGCActivityCallback : public GCActivityCallback {
 public:
     FullGCActivityCallback(Heap*);
 
-    void doCollection() override;
+    void doCollection(VM&) override;
 
     bool didGCRecently() const { return m_didGCRecently; }
     void setDidGCRecently() { m_didGCRecently = true; }
 
 protected:
-    Seconds lastGCLength() override;
+    Seconds lastGCLength(Heap&) override;
     double gcTimeSlice(size_t bytes) override;
-    double deathRate() override;
+    double deathRate(Heap&) override;
 
     bool m_didGCRecently { false };
 };

Source/JavaScriptCore/heap/GCActivityCallback.cpp

@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2010-2017 Apple Inc. All rights reserved.
+ * Copyright (C) 2010-2018 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -45,83 +45,52 @@ GCActivityCallback::GCActivityCallback(Heap* heap)
 {
 }
 
-void GCActivityCallback::doWork()
+void GCActivityCallback::doWork(VM& vm)
 {
-    Heap* heap = &m_vm->heap;
     if (!isEnabled())
         return;
 
-    JSLockHolder locker(m_vm);
-    if (heap->isDeferred()) {
+    ASSERT(vm.currentThreadIsHoldingAPILock());
+    Heap& heap = vm.heap;
+    if (heap.isDeferred()) {
         scheduleTimer(0_s);
         return;
     }
 
-    doCollection();
+    doCollection(vm);
 }
 
-#if USE(CF)
 void GCActivityCallback::scheduleTimer(Seconds newDelay)
 {
     if (newDelay * timerSlop > m_delay)
         return;
     Seconds delta = m_delay - newDelay;
     m_delay = newDelay;
-    CFRunLoopTimerSetNextFireDate(m_timer.get(), CFRunLoopTimerGetNextFireDate(m_timer.get()) - delta.seconds());
+    if (auto timeUntilFire = this->timeUntilFire())
+        setTimeUntilFire(*timeUntilFire - delta);
+    else
+        setTimeUntilFire(newDelay);
 }
 
-void GCActivityCallback::cancelTimer()
-{
-    m_delay = s_decade;
-    CFRunLoopTimerSetNextFireDate(m_timer.get(), CFAbsoluteTimeGetCurrent() + s_decade.seconds());
-}
-
-MonotonicTime GCActivityCallback::nextFireTime()
-{
-    return MonotonicTime::now() + Seconds(CFRunLoopTimerGetNextFireDate(m_timer.get()) - CFAbsoluteTimeGetCurrent());
-}
-#else
-void GCActivityCallback::scheduleTimer(Seconds newDelay)
-{
-    if (newDelay * timerSlop > m_delay)
-        return;
-    Seconds delta = m_delay - newDelay;
-    m_delay = newDelay;
-
-    Seconds secondsUntilFire = m_timer.secondsUntilFire();
-    m_timer.startOneShot(std::max<Seconds>(secondsUntilFire - delta, 0_s));
-}
-
-void GCActivityCallback::cancelTimer()
-{
-    m_delay = s_decade;
-    m_timer.startOneShot(s_decade);
-}
-
-MonotonicTime GCActivityCallback::nextFireTime()
-{
-    return MonotonicTime::now() + m_timer.secondsUntilFire();
-}
-#endif
-
-void GCActivityCallback::didAllocate(size_t bytes)
+void GCActivityCallback::didAllocate(Heap& heap, size_t bytes)
 {
     // The first byte allocated in an allocation cycle will report 0 bytes to didAllocate. 
     // We pretend it's one byte so that we don't ignore this allocation entirely.
     if (!bytes)
         bytes = 1;
-    double bytesExpectedToReclaim = static_cast<double>(bytes) * deathRate();
-    Seconds newDelay = lastGCLength() / gcTimeSlice(bytesExpectedToReclaim);
+    double bytesExpectedToReclaim = static_cast<double>(bytes) * deathRate(heap);
+    Seconds newDelay = lastGCLength(heap) / gcTimeSlice(bytesExpectedToReclaim);
     scheduleTimer(newDelay);
 }
 
 void GCActivityCallback::willCollect()
 {
-    cancelTimer();
+    cancel();
 }
 
 void GCActivityCallback::cancel()
 {
+    m_delay = s_decade;
     cancelTimer();
 }
 

Source/JavaScriptCore/heap/GCActivityCallback.h

@@ -48,24 +48,22 @@ class JS_EXPORT_PRIVATE GCActivityCallback : public JSRunLoopTimer {
 
     GCActivityCallback(Heap*);
 
-    void doWork() override;
+    void doWork(VM&) override;
 
-    virtual void doCollection() = 0;
+    virtual void doCollection(VM&) = 0;
 
-    virtual void didAllocate(size_t);
-    virtual void willCollect();
-    virtual void cancel();
+    void didAllocate(Heap&, size_t);
+    void willCollect();
+    void cancel();
     bool isEnabled() const { return m_enabled; }
     void setEnabled(bool enabled) { m_enabled = enabled; }
 
     static bool s_shouldCreateGCTimer;
 
-    MonotonicTime nextFireTime();
-
 protected:
-    virtual Seconds lastGCLength() = 0;
+    virtual Seconds lastGCLength(Heap&) = 0;
     virtual double gcTimeSlice(size_t bytes) = 0;
-    virtual double deathRate() = 0;
+    virtual double deathRate(Heap&) = 0;
 
     GCActivityCallback(VM* vm)
         : Base(vm)
@@ -77,7 +75,6 @@ class JS_EXPORT_PRIVATE GCActivityCallback : public JSRunLoopTimer {
     bool m_enabled;
 
 protected:
-    void cancelTimer();
     void scheduleTimer(Seconds);
 
 private: