some Watchpoints' ::fireInternal method will call operations that mig…

…ht GC where the GC will cause the watchpoint itself to destruct

https://bugs.webkit.org/show_bug.cgi?id=159198
<rdar://problem/26302360>

Reviewed by Filip Pizlo.

Source/JavaScriptCore:

Firing a watchpoint may cause a GC to happen. This GC could destroy various
Watchpoints themselves while they're in the process of firing. It's not safe
for most Watchpoints to be destructed while they're in the middle of firing.
This GC could also destroy the WatchpointSet itself, and it's not in a safe
state to be destroyed. WatchpointSet::fireAllWatchpoints now defers gc for a
while. This prevents a GC from destructing any Watchpoints while they're
in the process of firing. This bug was being hit by the stress GC bots
because we would destruct a particular Watchpoint while it was firing,
and then we would access its field after it had already been destroyed.
This was causing all kinds of weird symptoms. Also, this was easier to
catch when running with guard malloc because the first access after
destruction would lead to a crash.

* bytecode/AdaptiveInferredPropertyValueWatchpointBase.cpp:
(JSC::AdaptiveInferredPropertyValueWatchpointBase::fire):
* bytecode/CodeBlock.cpp:
(JSC::CodeBlock::finishCreation):
* bytecode/VariableWriteFireDetail.cpp:
(JSC::VariableWriteFireDetail::dump):
(JSC::VariableWriteFireDetail::touch):
* bytecode/VariableWriteFireDetail.h:
* bytecode/Watchpoint.cpp:
(JSC::WatchpointSet::add):
(JSC::WatchpointSet::fireAllSlow):
(JSC::WatchpointSet::fireAllWatchpoints):
(JSC::InlineWatchpointSet::add):
(JSC::InlineWatchpointSet::fireAll):
(JSC::InlineWatchpointSet::inflateSlow):
* bytecode/Watchpoint.h:
(JSC::WatchpointSet::startWatching):
(JSC::WatchpointSet::fireAll):
(JSC::WatchpointSet::touch):
(JSC::WatchpointSet::invalidate):
(JSC::WatchpointSet::isBeingWatched):
(JSC::WatchpointSet::offsetOfState):
(JSC::WatchpointSet::addressOfSetIsNotEmpty):
(JSC::InlineWatchpointSet::startWatching):
(JSC::InlineWatchpointSet::fireAll):
(JSC::InlineWatchpointSet::invalidate):
(JSC::InlineWatchpointSet::touch):
* bytecompiler/BytecodeGenerator.cpp:
(JSC::BytecodeGenerator::BytecodeGenerator):
* dfg/DFGOperations.cpp:
* interpreter/Interpreter.cpp:
(JSC::Interpreter::execute):
* jit/JITOperations.cpp:
* jsc.cpp:
(WTF::Masquerader::create):
* llint/LLIntSlowPaths.cpp:
(JSC::LLInt::LLINT_SLOW_PATH_DECL):
* runtime/ArrayBufferNeuteringWatchpoint.cpp:
(JSC::ArrayBufferNeuteringWatchpoint::fireAll):
* runtime/FunctionRareData.cpp:
(JSC::FunctionRareData::clear):
* runtime/InferredType.cpp:
(JSC::InferredType::willStoreValueSlow):
(JSC::InferredType::makeTopSlow):
(JSC::InferredType::set):
(JSC::InferredType::removeStructure):
(JSC::InferredType::InferredStructureWatchpoint::fireInternal):
* runtime/InferredValue.cpp:
(JSC::InferredValue::notifyWriteSlow):
(JSC::InferredValue::ValueCleanup::finalizeUnconditionally):
* runtime/InferredValue.h:
(JSC::InferredValue::notifyWrite):
(JSC::InferredValue::invalidate):
* runtime/JSGlobalObject.cpp:
(JSC::JSGlobalObject::haveABadTime):
* runtime/JSSymbolTableObject.h:
(JSC::symbolTablePutTouchWatchpointSet):
(JSC::symbolTablePutInvalidateWatchpointSet):
* runtime/Structure.cpp:
(JSC::Structure::didCachePropertyReplacement):
(JSC::Structure::startWatchingInternalProperties):
(JSC::DeferredStructureTransitionWatchpointFire::~DeferredStructureTransitionWatchpointFire):
(JSC::DeferredStructureTransitionWatchpointFire::add):
(JSC::Structure::didTransitionFromThisStructure):
(JSC::Structure::prototypeForLookup):
* runtime/StructureInlines.h:
(JSC::Structure::didReplaceProperty):
(JSC::Structure::propertyReplacementWatchpointSet):
* runtime/SymbolTable.h:
(JSC::SymbolTableEntry::isDontEnum):
(JSC::SymbolTableEntry::disableWatching):
* runtime/VM.cpp:
(JSC::VM::addImpureProperty):
(JSC::enableProfilerWithRespectToCount):

Source/WebCore:

* bindings/js/JSDOMWindowBase.cpp:
(WebCore::JSDOMWindowBase::fireFrameClearedWatchpointsForWindow):
* bindings/scripts/CodeGeneratorJS.pm:
(GenerateHeader):
* bindings/scripts/test/JS/JSTestEventTarget.h:
(WebCore::JSTestEventTarget::create):


Canonical link: https://commits.webkit.org/177335@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@202588 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog

@@ -1,3 +1,99 @@
+2016-06-28  Saam Barati  <sbarati@apple.com>
+
+        some Watchpoints' ::fireInternal method will call operations that might GC where the GC will cause the watchpoint itself to destruct
+        https://bugs.webkit.org/show_bug.cgi?id=159198
+        <rdar://problem/26302360>
+
+        Reviewed by Filip Pizlo.
+
+        Firing a watchpoint may cause a GC to happen. This GC could destroy various
+        Watchpoints themselves while they're in the process of firing. It's not safe
+        for most Watchpoints to be destructed while they're in the middle of firing.
+        This GC could also destroy the WatchpointSet itself, and it's not in a safe
+        state to be destroyed. WatchpointSet::fireAllWatchpoints now defers gc for a
+        while. This prevents a GC from destructing any Watchpoints while they're
+        in the process of firing. This bug was being hit by the stress GC bots
+        because we would destruct a particular Watchpoint while it was firing,
+        and then we would access its field after it had already been destroyed.
+        This was causing all kinds of weird symptoms. Also, this was easier to
+        catch when running with guard malloc because the first access after
+        destruction would lead to a crash.
+
+        * bytecode/AdaptiveInferredPropertyValueWatchpointBase.cpp:
+        (JSC::AdaptiveInferredPropertyValueWatchpointBase::fire):
+        * bytecode/CodeBlock.cpp:
+        (JSC::CodeBlock::finishCreation):
+        * bytecode/VariableWriteFireDetail.cpp:
+        (JSC::VariableWriteFireDetail::dump):
+        (JSC::VariableWriteFireDetail::touch):
+        * bytecode/VariableWriteFireDetail.h:
+        * bytecode/Watchpoint.cpp:
+        (JSC::WatchpointSet::add):
+        (JSC::WatchpointSet::fireAllSlow):
+        (JSC::WatchpointSet::fireAllWatchpoints):
+        (JSC::InlineWatchpointSet::add):
+        (JSC::InlineWatchpointSet::fireAll):
+        (JSC::InlineWatchpointSet::inflateSlow):
+        * bytecode/Watchpoint.h:
+        (JSC::WatchpointSet::startWatching):
+        (JSC::WatchpointSet::fireAll):
+        (JSC::WatchpointSet::touch):
+        (JSC::WatchpointSet::invalidate):
+        (JSC::WatchpointSet::isBeingWatched):
+        (JSC::WatchpointSet::offsetOfState):
+        (JSC::WatchpointSet::addressOfSetIsNotEmpty):
+        (JSC::InlineWatchpointSet::startWatching):
+        (JSC::InlineWatchpointSet::fireAll):
+        (JSC::InlineWatchpointSet::invalidate):
+        (JSC::InlineWatchpointSet::touch):
+        * bytecompiler/BytecodeGenerator.cpp:
+        (JSC::BytecodeGenerator::BytecodeGenerator):
+        * dfg/DFGOperations.cpp:
+        * interpreter/Interpreter.cpp:
+        (JSC::Interpreter::execute):
+        * jit/JITOperations.cpp:
+        * jsc.cpp:
+        (WTF::Masquerader::create):
+        * llint/LLIntSlowPaths.cpp:
+        (JSC::LLInt::LLINT_SLOW_PATH_DECL):
+        * runtime/ArrayBufferNeuteringWatchpoint.cpp:
+        (JSC::ArrayBufferNeuteringWatchpoint::fireAll):
+        * runtime/FunctionRareData.cpp:
+        (JSC::FunctionRareData::clear):
+        * runtime/InferredType.cpp:
+        (JSC::InferredType::willStoreValueSlow):
+        (JSC::InferredType::makeTopSlow):
+        (JSC::InferredType::set):
+        (JSC::InferredType::removeStructure):
+        (JSC::InferredType::InferredStructureWatchpoint::fireInternal):
+        * runtime/InferredValue.cpp:
+        (JSC::InferredValue::notifyWriteSlow):
+        (JSC::InferredValue::ValueCleanup::finalizeUnconditionally):
+        * runtime/InferredValue.h:
+        (JSC::InferredValue::notifyWrite):
+        (JSC::InferredValue::invalidate):
+        * runtime/JSGlobalObject.cpp:
+        (JSC::JSGlobalObject::haveABadTime):
+        * runtime/JSSymbolTableObject.h:
+        (JSC::symbolTablePutTouchWatchpointSet):
+        (JSC::symbolTablePutInvalidateWatchpointSet):
+        * runtime/Structure.cpp:
+        (JSC::Structure::didCachePropertyReplacement):
+        (JSC::Structure::startWatchingInternalProperties):
+        (JSC::DeferredStructureTransitionWatchpointFire::~DeferredStructureTransitionWatchpointFire):
+        (JSC::DeferredStructureTransitionWatchpointFire::add):
+        (JSC::Structure::didTransitionFromThisStructure):
+        (JSC::Structure::prototypeForLookup):
+        * runtime/StructureInlines.h:
+        (JSC::Structure::didReplaceProperty):
+        (JSC::Structure::propertyReplacementWatchpointSet):
+        * runtime/SymbolTable.h:
+        (JSC::SymbolTableEntry::isDontEnum):
+        (JSC::SymbolTableEntry::disableWatching):
+        * runtime/VM.cpp:
+        (JSC::VM::addImpureProperty):
+        (JSC::enableProfilerWithRespectToCount):
+
 2016-06-28  Filip Pizlo  <fpizlo@apple.com>
 
         JSRopeString should use release asserts, not debug asserts, about substring bounds

Source/JavaScriptCore/bytecode/AdaptiveInferredPropertyValueWatchpointBase.cpp

@@ -50,10 +50,6 @@ void AdaptiveInferredPropertyValueWatchpointBase::install()
 
 void AdaptiveInferredPropertyValueWatchpointBase::fire(const FireDetail& detail)
 {
-    // We need to defer GC here otherwise we might trigger a GC that could destroy the owner
-    // CodeBlock. In particular, this can happen when we add rare data to a structure when
-    // we EnsureWatchability.
-    DeferGCForAWhile defer(*Heap::heap(m_key.object()));
     // One of the watchpoints fired, but the other one didn't. Make sure that neither of them are
     // in any set anymore. This simplifies things by allowing us to reinstall the watchpoints
     // wherever from scratch.

Source/JavaScriptCore/bytecode/CodeBlock.cpp

@@ -2240,7 +2240,7 @@ void CodeBlock::finishCreation(VM& vm, ScriptExecutable* ownerExecutable, Unlink
                 instructions[i + 5].u.watchpointSet = op.watchpointSet;
             else if (op.type == ClosureVar || op.type == ClosureVarWithVarInjectionChecks) {
                 if (op.watchpointSet)
-                    op.watchpointSet->invalidate(PutToScopeFireDetail(this, ident));
+                    op.watchpointSet->invalidate(vm, PutToScopeFireDetail(this, ident));
             } else if (op.structure)
                 instructions[i + 5].u.structure.set(vm, this, op.structure);
             instructions[i + 6].u.pointer = reinterpret_cast<void*>(op.operand);

Source/JavaScriptCore/bytecode/VariableWriteFireDetail.cpp

@@ -35,9 +35,9 @@ void VariableWriteFireDetail::dump(PrintStream& out) const
     out.print("Write to ", m_name, " in ", JSValue(m_object));
 }
 
-void VariableWriteFireDetail::touch(WatchpointSet* set, JSObject* object, const PropertyName& name)
+void VariableWriteFireDetail::touch(VM& vm, WatchpointSet* set, JSObject* object, const PropertyName& name)
 {
-    set->touch(VariableWriteFireDetail(object, name));
+    set->touch(vm, VariableWriteFireDetail(object, name));
 }
 
 } // namespace JSC

Source/JavaScriptCore/bytecode/VariableWriteFireDetail.h

@@ -43,7 +43,7 @@ class VariableWriteFireDetail : public FireDetail {
 
     JS_EXPORT_PRIVATE void dump(PrintStream&) const override;
 
-    JS_EXPORT_PRIVATE static void touch(WatchpointSet*, JSObject*, const PropertyName&);
+    JS_EXPORT_PRIVATE static void touch(VM&, WatchpointSet*, JSObject*, const PropertyName&);
 
 private:
     JSObject* m_object;

Source/JavaScriptCore/bytecode/Watchpoint.cpp

@@ -26,6 +26,8 @@
 #include "config.h"
 #include "Watchpoint.h"
 
+#include "HeapInlines.h"
+#include "VM.h"
 #include <wtf/CompilationThread.h>
 #include <wtf/PassRefPtr.h>
 
@@ -81,26 +83,33 @@ void WatchpointSet::add(Watchpoint* watchpoint)
     m_state = IsWatched;
 }
 
-void WatchpointSet::fireAllSlow(const FireDetail& detail)
+void WatchpointSet::fireAllSlow(VM& vm, const FireDetail& detail)
 {
     ASSERT(state() == IsWatched);
 
     WTF::storeStoreFence();
     m_state = IsInvalidated; // Do this first. Needed for adaptive watchpoints.
-    fireAllWatchpoints(detail);
+    fireAllWatchpoints(vm, detail);
     WTF::storeStoreFence();
 }
 
-void WatchpointSet::fireAllSlow(const char* reason)
+void WatchpointSet::fireAllSlow(VM& vm, const char* reason)
 {
-    fireAllSlow(StringFireDetail(reason));
+    fireAllSlow(vm, StringFireDetail(reason));
 }
 
-void WatchpointSet::fireAllWatchpoints(const FireDetail& detail)
+void WatchpointSet::fireAllWatchpoints(VM& vm, const FireDetail& detail)
 {
     // In case there are any adaptive watchpoints, we need to make sure that they see that this
     // watchpoint has been already invalidated.
     RELEASE_ASSERT(hasBeenInvalidated());
+
+    // Firing a watchpoint may cause a GC to happen. This GC could destroy various
+    // Watchpoints themselves while they're in the process of firing. It's not safe
+    // for most Watchpoints to be destructed while they're in the middle of firing.
+    // This GC could also destroy us, and we're not in a safe state to be destroyed.
+    // The safest thing to do is to DeferGCForAWhile to prevent this GC from happening.
+    DeferGCForAWhile deferGC(vm.heap);
 
     while (!m_set.isEmpty()) {
         Watchpoint* watchpoint = m_set.begin();
@@ -130,9 +139,9 @@ void InlineWatchpointSet::add(Watchpoint* watchpoint)
     inflate()->add(watchpoint);
 }
 
-void InlineWatchpointSet::fireAll(const char* reason)
+void InlineWatchpointSet::fireAll(VM& vm, const char* reason)
 {
-    fireAll(StringFireDetail(reason));
+    fireAll(vm, StringFireDetail(reason));
 }
 
 WatchpointSet* InlineWatchpointSet::inflateSlow()

Source/JavaScriptCore/bytecode/Watchpoint.h

@@ -90,6 +90,7 @@ enum WatchpointState {
 };
 
 class InlineWatchpointSet;
+class VM;
 
 class WatchpointSet : public ThreadSafeRefCounted<WatchpointSet> {
     friend class LLIntOffsetsExtractor;
@@ -152,43 +153,43 @@ class WatchpointSet : public ThreadSafeRefCounted<WatchpointSet> {
         WTF::storeStoreFence();
     }
 
-    void fireAll(const FireDetail& detail)
+    void fireAll(VM& vm, const FireDetail& detail)
     {
         if (LIKELY(m_state != IsWatched))
             return;
-        fireAllSlow(detail);
+        fireAllSlow(vm, detail);
     }
 
-    void fireAll(const char* reason)
+    void fireAll(VM& vm, const char* reason)
     {
         if (LIKELY(m_state != IsWatched))
             return;
-        fireAllSlow(reason);
+        fireAllSlow(vm, reason);
     }
 
-    void touch(const FireDetail& detail)
+    void touch(VM& vm, const FireDetail& detail)
     {
         if (state() == ClearWatchpoint)
             startWatching();
         else
-            fireAll(detail);
+            fireAll(vm, detail);
     }
 
-    void touch(const char* reason)
+    void touch(VM& vm, const char* reason)
     {
-        touch(StringFireDetail(reason));
+        touch(vm, StringFireDetail(reason));
     }
 
-    void invalidate(const FireDetail& detail)
+    void invalidate(VM& vm, const FireDetail& detail)
     {
         if (state() == IsWatched)
-            fireAll(detail);
+            fireAll(vm, detail);
         m_state = IsInvalidated;
     }
 
-    void invalidate(const char* reason)
+    void invalidate(VM& vm, const char* reason)
     {
-        invalidate(StringFireDetail(reason));
+        invalidate(vm, StringFireDetail(reason));
     }
 
     bool isBeingWatched() const
@@ -200,11 +201,11 @@ class WatchpointSet : public ThreadSafeRefCounted<WatchpointSet> {
     static ptrdiff_t offsetOfState() { return OBJECT_OFFSETOF(WatchpointSet, m_state); }
     int8_t* addressOfSetIsNotEmpty() { return &m_setIsNotEmpty; }
 
-    JS_EXPORT_PRIVATE void fireAllSlow(const FireDetail&); // Call only if you've checked isWatched.
-    JS_EXPORT_PRIVATE void fireAllSlow(const char* reason); // Ditto.
+    JS_EXPORT_PRIVATE void fireAllSlow(VM&, const FireDetail&); // Call only if you've checked isWatched.
+    JS_EXPORT_PRIVATE void fireAllSlow(VM&, const char* reason); // Ditto.
 
 private:
-    void fireAllWatchpoints(const FireDetail&);
+    void fireAllWatchpoints(VM&, const FireDetail&);
 
     friend class InlineWatchpointSet;
 
@@ -296,10 +297,10 @@ class InlineWatchpointSet {
         m_data = encodeState(IsWatched);
     }
 
-    void fireAll(const FireDetail& detail)
+    void fireAll(VM& vm, const FireDetail& detail)
     {
         if (isFat()) {
-            fat()->fireAll(detail);
+            fat()->fireAll(vm, detail);
             return;
         }
         if (decodeState(m_data) == ClearWatchpoint)
@@ -308,20 +309,20 @@ class InlineWatchpointSet {
         WTF::storeStoreFence();
     }
 
-    void invalidate(const FireDetail& detail)
+    void invalidate(VM& vm, const FireDetail& detail)
     {
         if (isFat())
-            fat()->invalidate(detail);
+            fat()->invalidate(vm, detail);
         else
             m_data = encodeState(IsInvalidated);
     }
 
-    JS_EXPORT_PRIVATE void fireAll(const char* reason);
+    JS_EXPORT_PRIVATE void fireAll(VM&, const char* reason);
 
-    void touch(const FireDetail& detail)
+    void touch(VM& vm, const FireDetail& detail)
     {
         if (isFat()) {
-            fat()->touch(detail);
+            fat()->touch(vm, detail);
             return;
         }
         uintptr_t data = m_data;
@@ -335,9 +336,9 @@ class InlineWatchpointSet {
         WTF::storeStoreFence();
     }
 
-    void touch(const char* reason)
+    void touch(VM& vm, const char* reason)
     {
-        touch(StringFireDetail(reason));
+        touch(vm, StringFireDetail(reason));
     }
 
     // Note that for any watchpoint that is visible from the DFG, it would be incorrect to write code like:

Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp

@@ -385,7 +385,7 @@ BytecodeGenerator::BytecodeGenerator(VM& vm, FunctionNode* functionNode, Unlinke
                     // notifyWrite(), since that would be cumbersome. Also, watching formal
                     // parameters when "arguments" is in play is unlikely to be super profitable.
                     // So, we just disable it.
-                    entry.disableWatching();
+                    entry.disableWatching(*m_vm);
                     functionSymbolTable->set(NoLockingNecessary, name, entry);
                 }
                 emitOpcode(op_put_to_scope);

Source/JavaScriptCore/dfg/DFGOperations.cpp

@@ -1445,7 +1445,7 @@ void JIT_OPERATION operationNotifyWrite(ExecState* exec, WatchpointSet* set)
     VM& vm = exec->vm();
     NativeCallFrameTracer tracer(&vm, exec);
 
-    set->touch("Executed NotifyWrite");
+    set->touch(vm, "Executed NotifyWrite");
 }
 
 void JIT_OPERATION operationThrowStackOverflowForVarargs(ExecState* exec)

Source/JavaScriptCore/heap/CopyBarrier.h

@@ -27,6 +27,7 @@
 #define CopyBarrier_h
 
 #include "Heap.h"
+#include "VM.h"
 
 namespace JSC {
 

Source/JavaScriptCore/interpreter/Interpreter.cpp

@@ -1206,7 +1206,7 @@ JSValue Interpreter::execute(EvalExecutable* eval, CallFrame* callFrame, JSValue
     if (numVariables || numFunctions) {
         BatchedTransitionOptimizer optimizer(vm, variableObject);
         if (variableObject->next())
-            variableObject->globalObject()->varInjectionWatchpoint()->fireAll("Executed eval, fired VarInjection watchpoint");
+            variableObject->globalObject()->varInjectionWatchpoint()->fireAll(vm, "Executed eval, fired VarInjection watchpoint");
 
         for (unsigned i = 0; i < numVariables; ++i) {
             const Identifier& ident = codeBlock->variable(i);

Source/JavaScriptCore/jit/JITOperations.cpp

@@ -2078,7 +2078,7 @@ void JIT_OPERATION operationPutToScope(ExecState* exec, Instruction* bytecodePC)
         JSLexicalEnvironment* environment = jsCast<JSLexicalEnvironment*>(scope);
         environment->variableAt(ScopeOffset(pc[6].u.operand)).set(vm, environment, value);
         if (WatchpointSet* set = pc[5].u.watchpointSet)
-            set->touch("Executed op_put_scope<LocalClosureVar>");
+            set->touch(vm, "Executed op_put_scope<LocalClosureVar>");
         return;
     }
 

Source/JavaScriptCore/jsc.cpp

@@ -209,7 +209,7 @@ class Masquerader : public JSNonFinalObject {
 
     static Masquerader* create(VM& vm, JSGlobalObject* globalObject)
     {
-        globalObject->masqueradesAsUndefinedWatchpoint()->fireAll("Masquerading object allocated");
+        globalObject->masqueradesAsUndefinedWatchpoint()->fireAll(vm, "Masquerading object allocated");
         Structure* structure = createStructure(vm, globalObject, jsNull());
         Masquerader* result = new (NotNull, allocateCell<Masquerader>(vm.heap, sizeof(Masquerader))) Masquerader(vm, structure);
         result->finishCreation(vm);

Source/JavaScriptCore/llint/LLIntSlowPaths.cpp

@@ -1554,7 +1554,7 @@ LLINT_SLOW_PATH_DECL(slow_path_put_to_scope)
         // to have already changed the value of the variable. Otherwise we might watch and constant-fold
         // to the Undefined value from before the assignment.
         if (WatchpointSet* set = pc[5].u.watchpointSet)
-            set->touch("Executed op_put_scope<LocalClosureVar>");
+            set->touch(vm, "Executed op_put_scope<LocalClosureVar>");
         LLINT_END();
     }