Changeset 201180 in webkit

Timestamp:

May 19, 2016 2:02:44 PM (8 years ago)

Author:

mark.lam@apple.com

Message:

Code that null checks the VM pointer before any use should ref the VM.
​https://bugs.webkit.org/show_bug.cgi?id=157864

Reviewed by Filip Pizlo and Keith Miller.

JSLock::willReleaseLock() and HeapTimer::timerDidFire() need to reference the VM
through a RefPtr. Otherwise, there's no guarantee that the VM won't be deleted
after their null checks.

• bytecode/CodeBlock.h:

(JSC::CodeBlock::vm):
(JSC::CodeBlock::setVM): Deleted.

• Not used, and suggests that it can be changed during the lifetime of the CodeBlock (which should not be).

• heap/HeapTimer.cpp:

(JSC::HeapTimer::timerDidFire):

• runtime/JSLock.cpp:

(JSC::JSLock::willReleaseLock):

• Store the VM pointer in a RefPtr first, and null check the RefPtr instead of the raw VM pointer. This makes the null check a strong guarantee that the VM pointer is valid while these functions are using it.

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• bytecode/CodeBlock.h (modified) (1 diff)
• heap/HeapTimer.cpp (modified) (2 diffs)
• runtime/JSLock.cpp (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2016-05-19  Mark Lam  <mark.lam@apple.com>
+
+        Code that null checks the VM pointer before any use should ref the VM.
+        https://bugs.webkit.org/show_bug.cgi?id=157864
+
+        Reviewed by Filip Pizlo and Keith Miller.
+
+        JSLock::willReleaseLock() and HeapTimer::timerDidFire() need to reference the VM
+        through a RefPtr.  Otherwise, there's no guarantee that the VM won't be deleted
+        after their null checks.
+
+        * bytecode/CodeBlock.h:
+        (JSC::CodeBlock::vm):
+        (JSC::CodeBlock::setVM): Deleted.
+        - Not used, and suggests that it can be changed during the lifetime of the
+          CodeBlock (which should not be).
+
+        * heap/HeapTimer.cpp:
+        (JSC::HeapTimer::timerDidFire):
+        * runtime/JSLock.cpp:
+        (JSC::JSLock::willReleaseLock):
+        - Store the VM pointer in a RefPtr first, and null check the RefPtr instead of
+          the raw VM pointer.  This makes the null check a strong guarantee that the
+          VM pointer is valid while these functions are using it.
+
 2016-05-19  Saam barati  <sbarati@apple.com>
 

trunk/Source/JavaScriptCore/bytecode/CodeBlock.h

@@ -341 +341 @@
     ScriptExecutable* ownerScriptExecutable() const { return jsCast<ScriptExecutable*>(m_ownerExecutable.get()); }
 
-    void setVM(VM* vm) { m_vm = vm; }
-    VM* vm() { return m_vm; }
+    VM* vm() const { return m_vm; }
 
     void setThisRegister(VirtualRegister thisRegister) { m_thisRegister = thisRegister; }

trunk/Source/JavaScriptCore/heap/HeapTimer.cpp

@@ -81 +81 @@
     apiLock->lock();
 
-    VM* vm = apiLock->vm();
-    // The VM has been destroyed, so we should just give up.
+    RefPtr<VM> vm = apiLock->vm();
     if (!vm) {
+        // The VM has been destroyed, so we should just give up.
         apiLock->unlock();
         return;
@@ -99 +99 @@
 
     {
-        JSLockHolder locker(vm);
+        JSLockHolder locker(vm.get());
         heapTimer->doWork();
     }

trunk/Source/JavaScriptCore/runtime/JSLock.cpp

@@ -178 +178 @@
 void JSLock::willReleaseLock()
 {
-    if (m_vm) {
-        m_vm->drainMicrotasks();
-
-        m_vm->heap.releaseDelayedReleasedObjects();
-        m_vm->setStackPointerAtVMEntry(nullptr);
+    RefPtr<VM> vm = m_vm;
+    if (vm) {
+        vm->drainMicrotasks();
+
+        vm->heap.releaseDelayedReleasedObjects();
+        vm->setStackPointerAtVMEntry(nullptr);
     }