Add a JSRunLoopTimer registry in VM.
authormark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 16 Jun 2017 04:38:35 +0000 (04:38 +0000)
committermark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 16 Jun 2017 04:38:35 +0000 (04:38 +0000)
https://bugs.webkit.org/show_bug.cgi?id=173429
<rdar://problem/31287961>

Reviewed by Filip Pizlo.

Source/JavaScriptCore:

This way, we can be sure we've got every JSRunLoopTimer instance covered if we
need to change their run loop (e.g. when setting to the WebThread's run loop).

* heap/Heap.cpp:
(JSC::Heap::Heap):
(JSC::Heap::setRunLoop): Deleted.
* heap/Heap.h:
(JSC::Heap::runLoop): Deleted.
* runtime/JSRunLoopTimer.cpp:
(JSC::JSRunLoopTimer::JSRunLoopTimer):
(JSC::JSRunLoopTimer::setRunLoop):
(JSC::JSRunLoopTimer::~JSRunLoopTimer):
* runtime/VM.cpp:
(JSC::VM::VM):
(JSC::VM::registerRunLoopTimer):
(JSC::VM::unregisterRunLoopTimer):
(JSC::VM::setRunLoop):
* runtime/VM.h:
(JSC::VM::runLoop):

Source/WebCore:

No new tests needed because:
1. it's already covered: it was also originally discovered by our API tests while
   running on the iOS simulator. The test was intermittently failing on a debug
   build.
2. the issue is racy (it depends on a JSRunLoopTimer firing at the right time).
   Hence, it's non trivial to write a better test than the one we already have.

* bindings/js/CommonVM.cpp:
(WebCore::commonVMSlow):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@218381 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/heap/Heap.cpp
Source/JavaScriptCore/heap/Heap.h
Source/JavaScriptCore/runtime/JSRunLoopTimer.cpp
Source/JavaScriptCore/runtime/VM.cpp
Source/JavaScriptCore/runtime/VM.h
Source/WebCore/ChangeLog
Source/WebCore/bindings/js/CommonVM.cpp

index 27a67eb..38106aa 100644 (file)
@@ -1,3 +1,31 @@
+2017-06-15  Mark Lam  <mark.lam@apple.com>
+
+        Add a JSRunLoopTimer registry in VM.
+        https://bugs.webkit.org/show_bug.cgi?id=173429
+        <rdar://problem/31287961>
+
+        Reviewed by Filip Pizlo.
+
+        This way, we can be sure we've got every JSRunLoopTimer instance covered if we
+        need to change their run loop (e.g. when setting to the WebThread's run loop).
+
+        * heap/Heap.cpp:
+        (JSC::Heap::Heap):
+        (JSC::Heap::setRunLoop): Deleted.
+        * heap/Heap.h:
+        (JSC::Heap::runLoop): Deleted.
+        * runtime/JSRunLoopTimer.cpp:
+        (JSC::JSRunLoopTimer::JSRunLoopTimer):
+        (JSC::JSRunLoopTimer::setRunLoop):
+        (JSC::JSRunLoopTimer::~JSRunLoopTimer):
+        * runtime/VM.cpp:
+        (JSC::VM::VM):
+        (JSC::VM::registerRunLoopTimer):
+        (JSC::VM::unregisterRunLoopTimer):
+        (JSC::VM::setRunLoop):
+        * runtime/VM.h:
+        (JSC::VM::runLoop):
+
 2017-06-15  Joseph Pecoraro  <pecoraro@apple.com>
 
         [Cocoa] Modernize some internal initializers to use instancetype instead of id
index eaa41db..3a7d2f2 100644 (file)
@@ -289,9 +289,6 @@ Heap::Heap(VM* vm, HeapType heapType)
     // schedule the timer if we've never done a collection.
     , m_lastFullGCLength(0.01)
     , m_lastEdenGCLength(0.01)
-#if USE(CF)
-    , m_runLoop(CFRunLoopGetCurrent())
-#endif // USE(CF)
     , m_fullActivityCallback(GCActivityCallback::createFullTimer(this))
     , m_edenActivityCallback(GCActivityCallback::createEdenTimer(this))
     , m_sweeper(adoptRef(new IncrementalSweeper(this)))
@@ -2589,16 +2586,6 @@ void Heap::didFreeBlock(size_t capacity)
 #endif
 }
 
-#if USE(CF)
-void Heap::setRunLoop(CFRunLoopRef runLoop)
-{
-    m_runLoop = runLoop;
-    m_fullActivityCallback->setRunLoop(runLoop);
-    m_edenActivityCallback->setRunLoop(runLoop);
-    m_sweeper->setRunLoop(runLoop);
-}
-#endif // USE(CF)
-
 void Heap::addCoreConstraints()
 {
     m_constraintSet->add(
index cbf7030..ff8689e 100644 (file)
@@ -369,11 +369,6 @@ public:
     
     size_t numOpaqueRoots() const { return m_opaqueRoots.size(); }
 
-#if USE(CF)
-    CFRunLoopRef runLoop() const { return m_runLoop.get(); }
-    JS_EXPORT_PRIVATE void setRunLoop(CFRunLoopRef);
-#endif // USE(CF)
-
     HeapVerifier* verifier() const { return m_verifier.get(); }
     
     void addHeapFinalizerCallback(const HeapFinalizerCallback&);
@@ -623,9 +618,6 @@ private:
     Vector<WeakBlock*> m_logicallyEmptyWeakBlocks;
     size_t m_indexOfNextLogicallyEmptyWeakBlockToSweep { WTF::notFound };
     
-#if USE(CF)
-    RetainPtr<CFRunLoopRef> m_runLoop;
-#endif // USE(CF)
     RefPtr<FullGCActivityCallback> m_fullActivityCallback;
     RefPtr<GCActivityCallback> m_edenActivityCallback;
     RefPtr<IncrementalSweeper> m_sweeper;
index 91a2c32..6424b71 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2012, 2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2012-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
@@ -69,7 +69,7 @@ JSRunLoopTimer::JSRunLoopTimer(VM* vm)
     : m_vm(vm)
     , m_apiLock(&vm->apiLock())
 {
-    setRunLoop(vm->heap.runLoop());
+    m_vm->registerRunLoopTimer(this);
 }
 
 void JSRunLoopTimer::setRunLoop(CFRunLoopRef runLoop)
@@ -81,8 +81,8 @@ void JSRunLoopTimer::setRunLoop(CFRunLoopRef runLoop)
         m_timer.clear();
     }
 
+    m_runLoop = runLoop;
     if (runLoop) {
-        m_runLoop = 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));
@@ -92,7 +92,7 @@ void JSRunLoopTimer::setRunLoop(CFRunLoopRef runLoop)
 
 JSRunLoopTimer::~JSRunLoopTimer()
 {
-    setRunLoop(0);
+    m_vm->unregisterRunLoopTimer(this);
 }
 
 void JSRunLoopTimer::timerDidFireCallback(CFRunLoopTimerRef, void* contextPtr)
index 681c28d..2591b60 100644 (file)
@@ -156,6 +156,9 @@ static bool enableAssembler(ExecutableAllocator& executableAllocator)
 
 VM::VM(VMType vmType, HeapType heapType)
     : m_apiLock(adoptRef(new JSLock(this)))
+#if USE(CF)
+    , m_runLoop(CFRunLoopGetCurrent())
+#endif // USE(CF)
     , heap(this, heapType)
     , auxiliarySpace("Auxiliary", heap, AllocatorAttributes(DoesNotNeedDestruction, HeapCell::Auxiliary))
     , cellSpace("JSCell", heap, AllocatorAttributes(DoesNotNeedDestruction, HeapCell::JSCell))
@@ -931,4 +934,29 @@ RegisterAtOffsetList* VM::getAllCalleeSaveRegisterOffsets()
 }
 #endif // ENABLE(JIT)
 
+#if USE(CF)
+void VM::registerRunLoopTimer(JSRunLoopTimer* timer)
+{
+    ASSERT(runLoop());
+    ASSERT(!m_runLoopTimers.contains(timer));
+    m_runLoopTimers.add(timer);
+    timer->setRunLoop(runLoop());
+}
+
+void VM::unregisterRunLoopTimer(JSRunLoopTimer* timer)
+{
+    ASSERT(m_runLoopTimers.contains(timer));
+    m_runLoopTimers.remove(timer);
+    timer->setRunLoop(nullptr);
+}
+
+void VM::setRunLoop(CFRunLoopRef runLoop)
+{
+    ASSERT(runLoop);
+    m_runLoop = runLoop;
+    for (auto timer : m_runLoopTimers)
+        timer->setRunLoop(runLoop);
+}
+#endif // USE(CF)
+
 } // namespace JSC
index 19e343c..67f0608 100644 (file)
@@ -109,6 +109,7 @@ class Interpreter;
 class JSCustomGetterSetterFunction;
 class JSGlobalObject;
 class JSObject;
+class JSRunLoopTimer;
 class JSWebAssemblyInstance;
 class LLIntOffsetsExtractor;
 class NativeExecutable;
@@ -279,6 +280,11 @@ public:
 
 private:
     RefPtr<JSLock> m_apiLock;
+#if USE(CF)
+    // These need to be initialized before heap below.
+    HashSet<JSRunLoopTimer*> m_runLoopTimers;
+    RetainPtr<CFRunLoopRef> m_runLoop;
+#endif
 
 public:
     Heap heap;
@@ -669,6 +675,13 @@ public:
     ThreadIdentifier throwingThread() const { return m_throwingThread; }
 #endif
 
+#if USE(CF)
+    CFRunLoopRef runLoop() const { return m_runLoop.get(); }
+    void registerRunLoopTimer(JSRunLoopTimer*);
+    void unregisterRunLoopTimer(JSRunLoopTimer*);
+    JS_EXPORT_PRIVATE void setRunLoop(CFRunLoopRef);
+#endif // USE(CF)
+
 private:
     friend class LLIntOffsetsExtractor;
 
index 39b5dd8..ddcc96d 100644 (file)
@@ -1,3 +1,21 @@
+2017-06-15  Mark Lam  <mark.lam@apple.com>
+
+        Add a JSRunLoopTimer registry in VM.
+        https://bugs.webkit.org/show_bug.cgi?id=173429
+        <rdar://problem/31287961>
+
+        Reviewed by Filip Pizlo.
+
+        No new tests needed because:
+        1. it's already covered: it was also originally discovered by our API tests while
+           running on the iOS simulator. The test was intermittently failing on a debug
+           build.
+        2. the issue is racy (it depends on a JSRunLoopTimer firing at the right time).
+           Hence, it's non trivial to write a better test than the one we already have.
+
+        * bindings/js/CommonVM.cpp:
+        (WebCore::commonVMSlow):
+
 2017-06-15  Antoine Quint  <graouts@apple.com>
 
         REGRESSION: AirPlay button is incorrectly highlighted in inline and fullscreen
index 94c9fd3..aa79eee 100644 (file)
@@ -54,7 +54,7 @@ VM& commonVMSlow()
     g_commonVMOrNull = &VM::createLeaked(LargeHeap).leakRef();
     g_commonVMOrNull->heap.acquireAccess(); // At any time, we may do things that affect the GC.
 #if PLATFORM(IOS)
-    g_commonVMOrNull->heap.setRunLoop(WebThreadRunLoop());
+    g_commonVMOrNull->setRunLoop(WebThreadRunLoop());
     g_commonVMOrNull->heap.machineThreads().addCurrentThread();
 #endif