Attachment 346929 Details for Bug 188426 – patch

[patch] patch

a-backup.diff (text/plain), 26.28 KB, created by Saam Barati on 2018-08-10 14:40:54 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Saam Barati

Created: 2018-08-10 14:40:54 PDT

Size: 26.28 KB

patch

obsolete

>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 234769)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,87 @@
>+2018-08-10  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 NOBODY (OOPS!).
>+
>+        When I was reading the JSRunLoopTimer code, I noticed that it would be possible
>+        that we 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 making it so that we ref() the timer when
>+        setting a timer and deref()ing in the timer callback code. This gets super
>+        tricky since it's not obvious what to do when cancelling or rescheduling
>+        a timer. For example, one thread may be cancelling a timer right before
>+        a timer callback is about to be called imminently. This patch takes this
>+        approach:
>+        - Any time we schedule or cancel the timer, we go ahead and attempt to do
>+        the necessary work involved. If we notice that after doing the necessary work,
>+        the timer fire time is still past the current time, we declare success.
>+        (A timer won't fire before its target fire date.)
>+        - If we notice that the time after doing the work is after the fire date,
>+        we force the timer to fire immediately, deref itself, and then replay
>+        whatever work we needed to do. That work is either cancel the timer
>+        or reschedule a time.
>+        
>+        This patch also removes a lot of duplication between GCActivityCallback
>+        and JSRunLoopTimer.
>+
>+        * heap/GCActivityCallback.cpp:
>+        (JSC::GCActivityCallback::scheduleTimer):
>+        (JSC::GCActivityCallback::willCollect):
>+        (JSC::GCActivityCallback::cancel):
>+        (JSC::GCActivityCallback::cancelTimer): Deleted.
>+        (JSC::GCActivityCallback::nextFireTime): Deleted.
>+        * heap/GCActivityCallback.h:
>+        * heap/IncrementalSweeper.cpp:
>+        (JSC::IncrementalSweeper::scheduleTimer):
>+        * heap/StopIfNecessaryTimer.cpp:
>+        (JSC::StopIfNecessaryTimer::scheduleSoon):
>+        * runtime/JSRunLoopTimer.cpp:
>+        (JSC::JSRunLoopTimer::timerDidFire):
>+        (JSC::JSRunLoopTimer::setRunLoop):
>+        (JSC::JSRunLoopTimer::~JSRunLoopTimer):
>+        (JSC::JSRunLoopTimer::timeUntilFire):
>+        (JSC::JSRunLoopTimer::didWorkBeforeTimerFire):
>+        (JSC::JSRunLoopTimer::setTimeUntilFire):
>+        (JSC::JSRunLoopTimer::setTimeUntilFireImpl):
>+        (JSC::JSRunLoopTimer::cancelTimer):
>+        (JSC::JSRunLoopTimer::setNeedsBalancedDeref):
>+        (JSC::JSRunLoopTimer::willDestroyVM):
>+        (JSC::JSRunLoopTimer::scheduleTimer): Deleted.
>+        * runtime/JSRunLoopTimer.h:
>+        * runtime/PromiseDeferredTimer.cpp:
>+        (JSC::PromiseDeferredTimer::runRunLoop):
>+        (JSC::PromiseDeferredTimer::scheduleWorkSoon):
>+        * runtime/VM.cpp:
>+        (JSC::VM::~VM):
>+
> 2018-08-10  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
> 
>         Date.UTC should not return NaN with only Year param
>Index: Source/JavaScriptCore/heap/GCActivityCallback.cpp
>===================================================================
>--- Source/JavaScriptCore/heap/GCActivityCallback.cpp	(revision 234769)
>+++ Source/JavaScriptCore/heap/GCActivityCallback.cpp	(working copy)
>@@ -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
>@@ -60,50 +60,15 @@ void GCActivityCallback::doWork()
>     doCollection();
> }
> 
>-#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());
>+    setTimeUntilFire(timeUntilFire() - delta);
> }
> 
>-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)
> {
>     // The first byte allocated in an allocation cycle will report 0 bytes to didAllocate. 
>@@ -117,11 +82,12 @@ void GCActivityCallback::didAllocate(siz
> 
> void GCActivityCallback::willCollect()
> {
>-    cancelTimer();
>+    cancel();
> }
> 
> void GCActivityCallback::cancel()
> {
>+    m_delay = s_decade;
>     cancelTimer();
> }
> 
>Index: Source/JavaScriptCore/heap/GCActivityCallback.h
>===================================================================
>--- Source/JavaScriptCore/heap/GCActivityCallback.h	(revision 234769)
>+++ Source/JavaScriptCore/heap/GCActivityCallback.h	(working copy)
>@@ -52,16 +52,14 @@ public:
> 
>     virtual void doCollection() = 0;
> 
>-    virtual void didAllocate(size_t);
>-    virtual void willCollect();
>-    virtual void cancel();
>+    void didAllocate(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 double gcTimeSlice(size_t bytes) = 0;
>@@ -77,7 +75,6 @@ protected:
>     bool m_enabled;
> 
> protected:
>-    void cancelTimer();
>     void scheduleTimer(Seconds);
> 
> private:
>Index: Source/JavaScriptCore/heap/IncrementalSweeper.cpp
>===================================================================
>--- Source/JavaScriptCore/heap/IncrementalSweeper.cpp	(revision 234769)
>+++ Source/JavaScriptCore/heap/IncrementalSweeper.cpp	(working copy)
>@@ -1,5 +1,5 @@
> /*
>- * Copyright (C) 2012, 2016 Apple Inc. All rights reserved.
>+ * Copyright (C) 2012-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
>@@ -40,7 +40,7 @@ static const double sweepTimeMultiplier 
> 
> void IncrementalSweeper::scheduleTimer()
> {
>-    Base::scheduleTimer(sweepTimeSlice * sweepTimeMultiplier);
>+    setTimeUntilFire(sweepTimeSlice * sweepTimeMultiplier);
> }
> 
> IncrementalSweeper::IncrementalSweeper(Heap* heap)
>Index: Source/JavaScriptCore/heap/StopIfNecessaryTimer.cpp
>===================================================================
>--- Source/JavaScriptCore/heap/StopIfNecessaryTimer.cpp	(revision 234769)
>+++ Source/JavaScriptCore/heap/StopIfNecessaryTimer.cpp	(working copy)
>@@ -1,5 +1,5 @@
> /*
>- * Copyright (C) 2016 Apple Inc. All rights reserved.
>+ * Copyright (C) 2016-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
>@@ -48,7 +48,7 @@ void StopIfNecessaryTimer::scheduleSoon(
>         WTF::loadLoadFence();
>         return;
>     }
>-    scheduleTimer(0_s);
>+    setTimeUntilFire(0_s);
> }
> 
> } // namespace JSC
>Index: Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp
>===================================================================
>--- Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp	(revision 234769)
>+++ Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp	(working copy)
>@@ -1,5 +1,5 @@
> /*
>- * Copyright (C) 2012-2017 Apple Inc. All rights reserved.
>+ * Copyright (C) 2012-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
>@@ -26,13 +26,13 @@
> #include "config.h"
> #include "JSRunLoopTimer.h"
> 
>-#include "GCActivityCallback.h"
> #include "IncrementalSweeper.h"
> #include "JSCInlines.h"
> #include "JSObject.h"
> #include "JSString.h"
> 
> #include <wtf/MainThread.h>
>+#include <wtf/NoTailCalls.h>
> #include <wtf/Threading.h>
> 
> #if USE(GLIB_EVENT_LOOP)
>@@ -48,13 +48,52 @@ const Seconds JSRunLoopTimer::s_decade {
> 
> void JSRunLoopTimer::timerDidFire()
> {
>-    JSLock* apiLock = m_apiLock.get();
>-    if (!apiLock) {
>-        // Likely a buggy usage: the timer fired while JSRunLoopTimer was being destroyed.
>-        return;
>+    NO_TAIL_CALLS();
>+
>+    RefPtr<JSLock> apiLock;
>+
>+    // Don't let the deref below prematurely kill us if we're the last thing keeping ourselves alive.
>+    Ref<JSRunLoopTimer> protectedThis(*this); 
>+
>+    {
>+        auto locker = holdLock(m_lock);
>+        
>+        apiLock = m_apiLock.get();
>+        if (!apiLock) {
>+            // The VM has been destroyed, so we should just give up.
>+            return;
>+        }
>+
>+#if USE(CF)
>+        if (CFRunLoopGetCurrent() != m_runLoop.get())
>+            return;
>+#endif
>+
>+        ASSERT(m_isScheduled);
>+        m_isScheduled = false;
>+        this->deref();
>+
>+        if (m_isTimerFiringForBalancedDeref) {
>+            m_isTimerFiringForBalancedDeref = false;
>+            if (m_timeToSetOnTimerFire) {
>+                Seconds nextFireTime = *m_timeToSetOnTimerFire - MonotonicTime::now();
>+                m_timeToSetOnTimerFire = std::nullopt;
>+                setTimeUntilFireImpl(locker, nextFireTime);
>+            } else {
>+                // Make sure there isn't a secondary pending zero second timer. If we don't
>+                // do this there is a chance we'd deref() an extra time (if there was a timer
>+                // pending to fire immediately after this).
>+#if USE(CF)
>+                CFRunLoopTimerSetNextFireDate(m_timer.get(), CFAbsoluteTimeGetCurrent() + s_decade.seconds());
>+#else
>+                m_timer.startOneShot(s_decade);
>+#endif
>+            }
>+            return;
>+        }
>     }
> 
>-    std::lock_guard<JSLock> lock(*apiLock);
>+    std::lock_guard<JSLock> lock(*apiLock.get());
>     RefPtr<VM> vm = apiLock->vm();
>     if (!vm) {
>         // The VM has been destroyed, so we should just give up.
>@@ -75,28 +114,38 @@ JSRunLoopTimer::JSRunLoopTimer(VM* vm)
> 
> void JSRunLoopTimer::setRunLoop(CFRunLoopRef runLoop)
> {
>+    auto locker = holdLock(m_lock);
>+
>     if (m_runLoop) {
>         CFRunLoopRemoveTimer(m_runLoop.get(), m_timer.get(), kCFRunLoopCommonModes);
>         CFRunLoopTimerInvalidate(m_timer.get());
>         m_runLoop.clear();
>         m_timer.clear();
>+
>+        if (m_isScheduled)
>+            this->deref();
>+        m_isScheduled = false;
>+        m_isTimerFiringForBalancedDeref = false;
>+        m_timeToSetOnTimerFire = std::nullopt;
>     }
> 
>     m_runLoop = runLoop;
>+
>     if (runLoop) {
>         memset(&m_context, 0, sizeof(CFRunLoopTimerContext));
>         m_context.info = this;
>-        m_timer = adoptCF(CFRunLoopTimerCreate(kCFAllocatorDefault, CFAbsoluteTimeGetCurrent() + s_decade.seconds(), s_decade.seconds(), 0, 0, JSRunLoopTimer::timerDidFireCallback, &m_context));
>+        m_timer = adoptCF(CFRunLoopTimerCreate(kCFAllocatorDefault, CFAbsoluteTimeGetCurrent() + s_decade.seconds(), CFAbsoluteTimeGetCurrent() + s_decade.seconds(), 0, 0, JSRunLoopTimer::timerDidFireCallback, &m_context));
>         CFRunLoopAddTimer(m_runLoop.get(), m_timer.get(), kCFRunLoopCommonModes);
>     }
> }
> 
> JSRunLoopTimer::~JSRunLoopTimer()
> {
>-    JSLock* apiLock = m_apiLock.get();
>-    std::lock_guard<JSLock> lock(*apiLock);
>-    m_vm->unregisterRunLoopTimer(this);
>-    m_apiLock = nullptr;
>+    if (JSLock* apiLock = m_apiLock.get()) {
>+        std::lock_guard<JSLock> lock(*apiLock);
>+        m_vm->unregisterRunLoopTimer(this);
>+        m_apiLock = nullptr;
>+    }
> }
> 
> void JSRunLoopTimer::timerDidFireCallback(CFRunLoopTimerRef, void* contextPtr)
>@@ -104,19 +153,9 @@ void JSRunLoopTimer::timerDidFireCallbac
>     static_cast<JSRunLoopTimer*>(contextPtr)->timerDidFire();
> }
> 
>-void JSRunLoopTimer::scheduleTimer(Seconds intervalInSeconds)
>-{
>-    CFRunLoopTimerSetNextFireDate(m_timer.get(), CFAbsoluteTimeGetCurrent() + intervalInSeconds.seconds());
>-    m_isScheduled = true;
>-    auto locker = holdLock(m_timerCallbacksLock);
>-    for (auto& task : m_timerSetCallbacks)
>-        task->run();
>-}
>-
>-void JSRunLoopTimer::cancelTimer()
>+Seconds JSRunLoopTimer::timeUntilFire()
> {
>-    CFRunLoopTimerSetNextFireDate(m_timer.get(), CFAbsoluteTimeGetCurrent() + s_decade.seconds());
>-    m_isScheduled = false;
>+    return Seconds(CFRunLoopTimerGetNextFireDate(m_timer.get()) - CFAbsoluteTimeGetCurrent());
> }
> 
> #else
>@@ -143,10 +182,77 @@ void JSRunLoopTimer::timerDidFireCallbac
>     timerDidFire();
> }
> 
>-void JSRunLoopTimer::scheduleTimer(Seconds intervalInSeconds)
>+Seconds JSRunLoopTimer::timeUntilFire()
> {
>-    m_timer.startOneShot(intervalInSeconds);
>+    return m_timer.secondsUntilFire();
>+}
>+
>+#endif
>+
>+template <typename Function>
>+bool JSRunLoopTimer::didWorkBeforeTimerFire(const LockHolder&, const Function& function)
>+{
>+#if USE(CF)
>+    Seconds fireTimeFromEpoch { CFRunLoopTimerGetNextFireDate(m_timer.get()) };
>+#else
>+    Seconds fireTimeFromEpoch = MonotonicTime::now().secondsSinceEpoch() + timeUntilFire();
>+#endif
>+
>+    function();
>+
>+#if USE(CF)
>+    Seconds now { CFAbsoluteTimeGetCurrent() };
>+#else
>+    Seconds now = MonotonicTime::now().secondsSinceEpoch();
>+#endif
>+
>+    return now < fireTimeFromEpoch;
>+}
>+
>+void JSRunLoopTimer::setTimeUntilFire(Seconds intervalInSeconds)
>+{
>+    auto locker = holdLock(m_lock);
>+    setTimeUntilFireImpl(locker, intervalInSeconds);
>+}
>+
>+void JSRunLoopTimer::setTimeUntilFireImpl(const LockHolder& scheduleLocker, Seconds intervalInSeconds)
>+{
>+    intervalInSeconds = std::max<Seconds>(intervalInSeconds, 0_s);
>+    auto targetFireTime = MonotonicTime::now() + intervalInSeconds;
>+
>+    if (m_isTimerFiringForBalancedDeref) {
>+        ASSERT(m_isScheduled);
>+        m_timeToSetOnTimerFire = targetFireTime;
>+        return;
>+    }
>+
>+    if (!m_isScheduled)
>+        this->ref();
>+
>+    bool completedInTime = didWorkBeforeTimerFire(scheduleLocker, [&] {
>+#if USE(CF)
>+        CFRunLoopTimerSetNextFireDate(m_timer.get(), CFAbsoluteTimeGetCurrent() + intervalInSeconds.seconds());
>+#else
>+        m_timer.startOneShot(intervalInSeconds);
>+#endif
>+    });
>+
>+    if (!completedInTime && m_isScheduled) {
>+        // This is where things get tricky. If we're scheduled, it means we have an extra +1 in refcount.
>+        // If the above call succeeded in rescheduling the timer, there still may be a timer callback
>+        // on the stack, waiting to be called imminently. That would mean that the imminent callstack would
>+        // deref, but then the rescheduled above timer would also deref. That would be bad, since the two
>+        // derefs are not balanced with the single ref we did when we first scheduled the timer. So, what
>+        // we do here is force the timer to run immediately. Once it does, it'll deref itself and call back
>+        // into here to set the timer for the correct delay.
>+        m_timeToSetOnTimerFire = targetFireTime;
>+        setNeedsBalancedDeref(scheduleLocker);
>+        return;
>+    }
>+
>     m_isScheduled = true;
>+    m_isTimerFiringForBalancedDeref = false;
>+    m_timeToSetOnTimerFire = std::nullopt;
> 
>     auto locker = holdLock(m_timerCallbacksLock);
>     for (auto& task : m_timerSetCallbacks)
>@@ -155,11 +261,67 @@ void JSRunLoopTimer::scheduleTimer(Secon
> 
> void JSRunLoopTimer::cancelTimer()
> {
>-    m_timer.startOneShot(s_decade);
>-    m_isScheduled = false;
>+    auto locker = holdLock(m_lock);
>+
>+    m_timeToSetOnTimerFire = std::nullopt;
>+
>+    if (m_isTimerFiringForBalancedDeref) {
>+        ASSERT(m_isScheduled);
>+        return;
>+    }
>+
>+    if (!m_isScheduled)
>+        return;
>+
>+    // This is where things get tricky. If we're scheduled, it means we have an extra +1 in refcount.
>+    // However, we need to do a dance to ensure that we don't run the timer as we're cancelling it,
>+    // otherwise, we have a race to where we may be using dead memory. So, to prevent such a race,
>+    // we optimistically deref ourselves. We then check if we rescheduled the timer for a decade
>+    // before the previous timer was supposed to fire. If we succeed in doing all this before the
>+    // previous timer was supposed to fire, we didn't race, and we can let the deref stand. Otherwise,
>+    // we re-ref() ourselves, make the timer fire immediately, and let it deref itself.
>+
>+    // This deref can't kill us because the timer callback also grabs the lock before it derefs.
>+    RELEASE_ASSERT(this->refCount() >= 2);
>+    this->deref();
>+
>+    bool completedInTime = didWorkBeforeTimerFire(locker, [&] {
>+#if USE(CF)
>+        CFRunLoopTimerSetNextFireDate(m_timer.get(), CFAbsoluteTimeGetCurrent() + s_decade.seconds());
>+#else
>+        m_timer.startOneShot(s_decade);
>+#endif
>+    });
>+
>+    if (completedInTime) {
>+        // The timer could not have fired, so we're good to go.
>+        m_isTimerFiringForBalancedDeref = false;
>+        m_isScheduled = false;
>+        return;
>+    }
>+
>+    ASSERT(m_isScheduled);
>+
>+    // We raced with the timer, it could have fired. We'll let it deref itself.
>+    // We ref here to cancel out our above deref.
>+    this->ref();
>+    setNeedsBalancedDeref(locker);
> }
> 
>+void JSRunLoopTimer::setNeedsBalancedDeref(const LockHolder&)
>+{
>+    m_isTimerFiringForBalancedDeref = true;
>+
>+    // We do this because we don't want to leak memory. We force the timer
>+    // to run now so we can update our state. That either means we update
>+    // to cancel the timer, or we update to a new time.
>+
>+#if USE(CF)
>+    CFRunLoopTimerSetNextFireDate(m_timer.get(), CFAbsoluteTimeGetCurrent() + 0.0);
>+#else
>+    m_timer.startOneShot(0_s);
> #endif
>+}
> 
> void JSRunLoopTimer::addTimerSetNotification(TimerNotificationCallback callback)
> {
>@@ -173,4 +335,11 @@ void JSRunLoopTimer::removeTimerSetNotif
>     m_timerSetCallbacks.remove(callback);
> }
> 
>+void JSRunLoopTimer::willDestroyVM()
>+{
>+    auto locker = holdLock(m_lock);
>+    m_vm = nullptr;
>+    m_apiLock = nullptr;
>+}
>+
> } // namespace JSC
>Index: Source/JavaScriptCore/runtime/JSRunLoopTimer.h
>===================================================================
>--- Source/JavaScriptCore/runtime/JSRunLoopTimer.h	(revision 234769)
>+++ Source/JavaScriptCore/runtime/JSRunLoopTimer.h	(working copy)
>@@ -57,12 +57,12 @@ public:
>     JS_EXPORT_PRIVATE virtual ~JSRunLoopTimer();
>     virtual void doWork() = 0;
> 
>-    void scheduleTimer(Seconds intervalInSeconds);
>+    void setTimeUntilFire(Seconds intervalInSeconds);
>     void cancelTimer();
>     bool isScheduled() const { return m_isScheduled; }
> 
>     // Note: The only thing the timer notification callback cannot do is
>-    // call scheduleTimer(). This will cause a deadlock. It would not
>+    // call setTimeUntilFire(). This will cause a deadlock. It would not
>     // be hard to make this work, however, there are no clients that need
>     // this behavior. We should implement it only if we find that we need it.
>     JS_EXPORT_PRIVATE void addTimerSetNotification(TimerNotificationCallback);
>@@ -72,29 +72,39 @@ public:
>     JS_EXPORT_PRIVATE void setRunLoop(CFRunLoopRef);
> #endif // USE(CF)
> 
>-protected:
>-    VM* m_vm;
>+    JS_EXPORT_PRIVATE Seconds timeUntilFire();
> 
>+    void willDestroyVM();
>+
>+protected:
>     static const Seconds s_decade;
> 
>-    RefPtr<JSLock> m_apiLock;
>     bool m_isScheduled { false };
> #if USE(CF)
>     RetainPtr<CFRunLoopTimerRef> m_timer;
>     RetainPtr<CFRunLoopRef> m_runLoop;
> 
>     CFRunLoopTimerContext m_context;
>-
>-    Lock m_shutdownMutex;
> #else
>     RunLoop::Timer<JSRunLoopTimer> m_timer;
> #endif
> 
>-    Lock m_timerCallbacksLock;
>-    HashSet<TimerNotificationCallback> m_timerSetCallbacks;
>-    
>+    VM* m_vm;
>+
> private:
>+    void setTimeUntilFireImpl(const LockHolder&, Seconds intervalInSeconds);
>+    template <typename Function>
>+    bool didWorkBeforeTimerFire(const LockHolder&, const Function&);
>     void timerDidFire();
>+    void setNeedsBalancedDeref(const LockHolder&);
>+    RefPtr<JSLock> m_apiLock;
>+
>+    HashSet<TimerNotificationCallback> m_timerSetCallbacks;
>+    Lock m_timerCallbacksLock;
>+
>+    bool m_isTimerFiringForBalancedDeref { false };
>+    Lock m_lock;
>+    std::optional<MonotonicTime> m_timeToSetOnTimerFire;
> };
>     
> } // namespace JSC
>Index: Source/JavaScriptCore/runtime/PromiseDeferredTimer.cpp
>===================================================================
>--- Source/JavaScriptCore/runtime/PromiseDeferredTimer.cpp	(revision 234769)
>+++ Source/JavaScriptCore/runtime/PromiseDeferredTimer.cpp	(working copy)
>@@ -1,5 +1,5 @@
> /*
>- * Copyright (C) 2017 Apple Inc. All rights reserved.
>+ * Copyright (C) 2017-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
>@@ -91,12 +91,13 @@ void PromiseDeferredTimer::runRunLoop()
>     ASSERT(CFRunLoopGetCurrent() == m_runLoop.get());
> #endif
>     m_shouldStopRunLoopWhenAllPromisesFinish = true;
>-    if (m_pendingPromises.size())
>+    if (m_pendingPromises.size()) {
> #if USE(CF)
>         CFRunLoopRun();
> #else
>         RunLoop::run();
> #endif
>+    }
> }
> 
> void PromiseDeferredTimer::addPendingPromise(JSPromiseDeferred* ticket, Vector<Strong<JSCell>>&& dependencies)
>@@ -151,7 +152,7 @@ void PromiseDeferredTimer::scheduleWorkS
>     LockHolder locker(m_taskLock);
>     m_tasks.append(std::make_tuple(ticket, WTFMove(task)));
>     if (!isScheduled() && !m_currentlyRunningTask)
>-        scheduleTimer(0_s);
>+        setTimeUntilFire(0_s);
> }
> 
> } // namespace JSC
>Index: Source/JavaScriptCore/runtime/VM.cpp
>===================================================================
>--- Source/JavaScriptCore/runtime/VM.cpp	(revision 234769)
>+++ Source/JavaScriptCore/runtime/VM.cpp	(working copy)
>@@ -572,7 +572,7 @@ VM::~VM()
>         }
>     }
> #endif // ENABLE(DFG_JIT)
>-    
>+
>     waitForAsynchronousDisassembly();
>     
>     // Clear this first to ensure that nobody tries to remove themselves from it.
>@@ -581,6 +581,12 @@ VM::~VM()
>     ASSERT(currentThreadIsHoldingAPILock());
>     m_apiLock->willDestroyVM(this);
>     heap.lastChanceToFinalize();
>+
>+#if USE(CF)
>+    // This needs to happen after heap.lastChanceToFinalize().
>+    for (auto timer : m_runLoopTimers)
>+        timer->willDestroyVM();
>+#endif
>     
>     delete interpreter;
> #ifndef NDEBUG
>Index: Source/WebCore/ChangeLog
>===================================================================
>--- Source/WebCore/ChangeLog	(revision 234769)
>+++ Source/WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,15 @@
>+2018-08-10  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 NOBODY (OOPS!).
>+
>+        * page/cocoa/ResourceUsageThreadCocoa.mm:
>+        (WebCore::ResourceUsageThread::platformThreadBody):
>+        * page/linux/ResourceUsageThreadLinux.cpp:
>+        (WebCore::ResourceUsageThread::platformThreadBody):
>+
> 2018-08-10  Antti Koivisto  <antti@apple.com>
> 
>         Use OptionSet for various RenderLayer flags
>Index: Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm
>===================================================================
>--- Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm	(revision 234769)
>+++ Source/WebCore/page/cocoa/ResourceUsageThreadCocoa.mm	(working copy)
>@@ -248,8 +248,9 @@ void ResourceUsageThread::platformThread
> 
>     data.totalExternalSize = currentGCOwnedExternal;
> 
>-    data.timeOfNextEdenCollection = vm->heap.edenActivityCallback()->nextFireTime();
>-    data.timeOfNextFullCollection = vm->heap.fullActivityCallback()->nextFireTime();
>+    auto now = MonotonicTime::now();
>+    data.timeOfNextEdenCollection = now + vm->heap.edenActivityCallback()->timeUntilFire();
>+    data.timeOfNextFullCollection = now + vm->heap.fullActivityCallback()->timeUntilFire();
> }
> 
> }
>Index: Source/WebCore/page/linux/ResourceUsageThreadLinux.cpp
>===================================================================
>--- Source/WebCore/page/linux/ResourceUsageThreadLinux.cpp	(revision 234769)
>+++ Source/WebCore/page/linux/ResourceUsageThreadLinux.cpp	(working copy)
>@@ -1,5 +1,6 @@
> /*
>  * Copyright (C) 2017 Igalia S.L.
>+ * Copyright (C) 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
>@@ -168,8 +169,9 @@ void ResourceUsageThread::platformThread
> 
>     data.totalExternalSize = currentGCOwnedExternal;
> 
>-    data.timeOfNextEdenCollection = vm->heap.edenActivityCallback()->nextFireTime();
>-    data.timeOfNextFullCollection = vm->heap.fullActivityCallback()->nextFireTime();
>+    auto now = MonotonicTime::now();
>+    data.timeOfNextEdenCollection = now + vm->heap.edenActivityCallback()->timeUntilFire();
>+    data.timeOfNextFullCollection = now + vm->heap.fullActivityCallback()->timeUntilFire();
> }
> 
> } // namespace WebCore

Flags:

saam: review-

Actions: View | Formatted Diff | Diff

Attachments on bug 188426: 346884 | 346885 | 346887 | 346888 | 346890 | 346891 | 346892 | 346893 | 346894 | 346929 | 346996 | 346997 | 347000 | 347001 | 347005 | 347021 | 347029 | 347030 | 347031 | 347032 | 347056 | 347065 | 347102 | 347105 | 347232 | 347584 | 347591 | 347747 | 347825