Merge r231518 - Deferred firing of structure transition watchpoints i…

…s racy https://bugs.webkit.org/show_bug.cgi?id=185438 Reviewed by Saam Barati. Changed DeferredStructureTransitionWatchpointFire to take the watchpoints to fire and fire them in the destructor. When the watchpoints are taken from the original WatchpointSet, that WatchpointSet if marked invalid. * bytecode/Watchpoint.cpp: (JSC::WatchpointSet::fireAllSlow): (JSC::WatchpointSet::take): (JSC::DeferredWatchpointFire::DeferredWatchpointFire): (JSC::DeferredWatchpointFire::~DeferredWatchpointFire): (JSC::DeferredWatchpointFire::fireAll): (JSC::DeferredWatchpointFire::takeWatchpointsToFire): * bytecode/Watchpoint.h: (JSC::WatchpointSet::fireAll): (JSC::InlineWatchpointSet::fireAll): * runtime/JSObject.cpp: (JSC::JSObject::setPrototypeDirect): (JSC::JSObject::convertToDictionary): * runtime/JSObjectInlines.h: (JSC::JSObject::putDirectInternal): * runtime/Structure.cpp: (JSC::Structure::Structure): (JSC::DeferredStructureTransitionWatchpointFire::DeferredStructureTransitionWatchpointFire): (JSC::DeferredStructureTransitionWatchpointFire::~DeferredStructureTransitionWatchpointFire): (JSC::DeferredStructureTransitionWatchpointFire::dump const): (JSC::Structure::didTransitionFromThisStructure const): (JSC::DeferredStructureTransitionWatchpointFire::add): Deleted. * runtime/Structure.h: (JSC::DeferredStructureTransitionWatchpointFire::structure const):
WebKit · Jul 31, 2018 · 0dbc03e · 0dbc03e
1 parent b1db990
commit 0dbc03e
Show file tree

Hide file tree

Showing 7 changed files with 136 additions and 19 deletions.
diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,39 @@
+2018-05-08  Michael Saboff  <msaboff@apple.com>
+
+        Deferred firing of structure transition watchpoints is racy
+        https://bugs.webkit.org/show_bug.cgi?id=185438
+
+        Reviewed by Saam Barati.
+
+        Changed DeferredStructureTransitionWatchpointFire to take the watchpoints to fire
+        and fire them in the destructor.  When the watchpoints are taken from the
+        original WatchpointSet, that WatchpointSet if marked invalid.
+
+        * bytecode/Watchpoint.cpp:
+        (JSC::WatchpointSet::fireAllSlow):
+        (JSC::WatchpointSet::take):
+        (JSC::DeferredWatchpointFire::DeferredWatchpointFire):
+        (JSC::DeferredWatchpointFire::~DeferredWatchpointFire):
+        (JSC::DeferredWatchpointFire::fireAll):
+        (JSC::DeferredWatchpointFire::takeWatchpointsToFire):
+        * bytecode/Watchpoint.h:
+        (JSC::WatchpointSet::fireAll):
+        (JSC::InlineWatchpointSet::fireAll):
+        * runtime/JSObject.cpp:
+        (JSC::JSObject::setPrototypeDirect):
+        (JSC::JSObject::convertToDictionary):
+        * runtime/JSObjectInlines.h:
+        (JSC::JSObject::putDirectInternal):
+        * runtime/Structure.cpp:
+        (JSC::Structure::Structure):
+        (JSC::DeferredStructureTransitionWatchpointFire::DeferredStructureTransitionWatchpointFire):
+        (JSC::DeferredStructureTransitionWatchpointFire::~DeferredStructureTransitionWatchpointFire):
+        (JSC::DeferredStructureTransitionWatchpointFire::dump const):
+        (JSC::Structure::didTransitionFromThisStructure const):
+        (JSC::DeferredStructureTransitionWatchpointFire::add): Deleted.
+        * runtime/Structure.h:
+        (JSC::DeferredStructureTransitionWatchpointFire::structure const):
+
 2018-04-28  Saam Barati  <sbarati@apple.com>
 
         We don't model regexp effects properly

diff --git a/Source/JavaScriptCore/bytecode/Watchpoint.cpp b/Source/JavaScriptCore/bytecode/Watchpoint.cpp
@@ -92,6 +92,16 @@ void WatchpointSet::fireAllSlow(VM& vm, const FireDetail& detail)
     WTF::storeStoreFence();
 }
 
+void WatchpointSet::fireAllSlow(VM&, DeferredWatchpointFire* deferredWatchpoints)
+{
+    ASSERT(state() == IsWatched);
+
+    WTF::storeStoreFence();
+    deferredWatchpoints->takeWatchpointsToFire(this);
+    m_state = IsInvalidated; // Do after moving watchpoints to deferredWatchpoints so deferredWatchpoints gets our current state.
+    WTF::storeStoreFence();
+}
+
 void WatchpointSet::fireAllSlow(VM& vm, const char* reason)
 {
     fireAllSlow(vm, StringFireDetail(reason));
@@ -133,6 +143,15 @@ void WatchpointSet::fireAllWatchpoints(VM& vm, const FireDetail& detail)
     }
 }
 
+void WatchpointSet::take(WatchpointSet* other)
+{
+    ASSERT(state() == ClearWatchpoint);
+    m_set.takeFrom(other->m_set);
+    m_setIsNotEmpty = other->m_setIsNotEmpty;
+    m_state = other->m_state;
+    other->m_setIsNotEmpty = false;
+}
+
 void InlineWatchpointSet::add(Watchpoint* watchpoint)
 {
     inflate()->add(watchpoint);
@@ -159,5 +178,28 @@ void InlineWatchpointSet::freeFat()
     fat()->deref();
 }
 
+DeferredWatchpointFire::DeferredWatchpointFire(VM& vm)
+    : m_vm(vm)
+    , m_watchpointsToFire(ClearWatchpoint)
+{
+}
+
+DeferredWatchpointFire::~DeferredWatchpointFire()
+{
+}
+
+void DeferredWatchpointFire::fireAll()
+{
+    if (m_watchpointsToFire.state() == IsWatched)
+        m_watchpointsToFire.fireAll(m_vm, *this);
+}
+
+void DeferredWatchpointFire::takeWatchpointsToFire(WatchpointSet* watchpointsToFire)
+{
+    ASSERT(m_watchpointsToFire.state() == ClearWatchpoint);
+    ASSERT(watchpointsToFire->state() == IsWatched);
+    m_watchpointsToFire.take(watchpointsToFire);
+}
+
 } // namespace JSC
 
diff --git a/Source/JavaScriptCore/bytecode/Watchpoint.h b/Source/JavaScriptCore/bytecode/Watchpoint.h
@@ -89,10 +89,12 @@ enum WatchpointState {
 };
 
 class InlineWatchpointSet;
+class DeferredWatchpointFire;
 class VM;
 
 class WatchpointSet : public ThreadSafeRefCounted<WatchpointSet> {
     friend class LLIntOffsetsExtractor;
+    friend class DeferredWatchpointFire;
 public:
     JS_EXPORT_PRIVATE WatchpointSet(WatchpointState);
 
@@ -159,6 +161,13 @@ class WatchpointSet : public ThreadSafeRefCounted<WatchpointSet> {
         fireAllSlow(vm, detail);
     }
 
+    void fireAll(VM& vm, DeferredWatchpointFire* deferredWatchpoints)
+    {
+        if (LIKELY(m_state != IsWatched))
+            return;
+        fireAllSlow(vm, deferredWatchpoints);
+    }
+
     void fireAll(VM& vm, const char* reason)
     {
         if (LIKELY(m_state != IsWatched))
@@ -201,10 +210,12 @@ class WatchpointSet : public ThreadSafeRefCounted<WatchpointSet> {
     int8_t* addressOfSetIsNotEmpty() { return &m_setIsNotEmpty; }
 
     JS_EXPORT_PRIVATE void fireAllSlow(VM&, const FireDetail&); // Call only if you've checked isWatched.
+    JS_EXPORT_PRIVATE void fireAllSlow(VM&, DeferredWatchpointFire* deferredWatchpoints);
     JS_EXPORT_PRIVATE void fireAllSlow(VM&, const char* reason); // Ditto.
 
 private:
     void fireAllWatchpoints(VM&, const FireDetail&);
+    void take(WatchpointSet* other);
 
     friend class InlineWatchpointSet;
 
@@ -308,6 +319,18 @@ class InlineWatchpointSet {
         WTF::storeStoreFence();
     }
 
+    void fireAll(VM& vm, DeferredWatchpointFire* deferred)
+    {
+        if (isFat()) {
+            fat()->fireAll(vm, deferred);
+            return;
+        }
+        if (decodeState(m_data) == ClearWatchpoint)
+            return;
+        m_data = encodeState(IsInvalidated);
+        WTF::storeStoreFence();
+    }
+
     void invalidate(VM& vm, const FireDetail& detail)
     {
         if (isFat())
@@ -435,4 +458,19 @@ class InlineWatchpointSet {
     uintptr_t m_data;
 };
 
+class DeferredWatchpointFire : public FireDetail {
+    WTF_MAKE_NONCOPYABLE(DeferredWatchpointFire);
+public:
+    JS_EXPORT_PRIVATE DeferredWatchpointFire(VM&);
+    JS_EXPORT_PRIVATE ~DeferredWatchpointFire();
+
+    JS_EXPORT_PRIVATE void takeWatchpointsToFire(WatchpointSet*);
+    JS_EXPORT_PRIVATE void fireAll();
+
+    void dump(PrintStream& out) const override = 0;
+private:
+    VM& m_vm;
+    WatchpointSet m_watchpointsToFire;
+};
+
 } // namespace JSC
diff --git a/Source/JavaScriptCore/runtime/JSObject.cpp b/Source/JavaScriptCore/runtime/JSObject.cpp
@@ -1652,7 +1652,7 @@ void JSObject::setPrototypeDirect(VM& vm, JSValue prototype)
         prototype.asCell()->didBecomePrototype();
 
     if (structure(vm)->hasMonoProto()) {
-        DeferredStructureTransitionWatchpointFire deferred;
+        DeferredStructureTransitionWatchpointFire deferred(vm, structure(vm));
         Structure* newStructure = Structure::changePrototypeTransition(vm, structure(vm), prototype, deferred);
         setStructure(vm, newStructure);
     } else
@@ -3543,7 +3543,7 @@ bool JSObject::defineOwnProperty(JSObject* object, ExecState* exec, PropertyName
 
 void JSObject::convertToDictionary(VM& vm)
 {
-    DeferredStructureTransitionWatchpointFire deferredWatchpointFire;
+    DeferredStructureTransitionWatchpointFire deferredWatchpointFire(vm, structure(vm));
     setStructure(
         vm, Structure::toCacheableDictionaryTransition(vm, structure(vm), &deferredWatchpointFire));
 }

diff --git a/Source/JavaScriptCore/runtime/JSObjectInlines.h b/Source/JavaScriptCore/runtime/JSObjectInlines.h
@@ -362,7 +362,7 @@ ALWAYS_INLINE bool JSObject::putDirectInternal(VM& vm, PropertyName propertyName
     // We want the structure transition watchpoint to fire after this object has switched
     // structure. This allows adaptive watchpoints to observe if the new structure is the one
     // we want.
-    DeferredStructureTransitionWatchpointFire deferredWatchpointFire;
+    DeferredStructureTransitionWatchpointFire deferredWatchpointFire(vm, structure);
 
     newStructure = Structure::addNewPropertyTransition(
         vm, structure, propertyName, attributes, offset, slot.context(), &deferredWatchpointFire);

diff --git a/Source/JavaScriptCore/runtime/Structure.cpp b/Source/JavaScriptCore/runtime/Structure.cpp
@@ -1035,22 +1035,20 @@ void StructureFireDetail::dump(PrintStream& out) const
     out.print("Structure transition from ", *m_structure);
 }
 
-DeferredStructureTransitionWatchpointFire::DeferredStructureTransitionWatchpointFire()
-    : m_structure(nullptr)
+DeferredStructureTransitionWatchpointFire::DeferredStructureTransitionWatchpointFire(VM& vm, Structure* structure)
+    : DeferredWatchpointFire(vm)
+    , m_structure(structure)
 {
 }
 
 DeferredStructureTransitionWatchpointFire::~DeferredStructureTransitionWatchpointFire()
 {
-    if (m_structure)
-        m_structure->transitionWatchpointSet().fireAll(*m_structure->vm(), StructureFireDetail(m_structure));
+    fireAll();
 }
 
-void DeferredStructureTransitionWatchpointFire::add(const Structure* structure)
+void DeferredStructureTransitionWatchpointFire::dump(PrintStream& out) const
 {
-    RELEASE_ASSERT(!m_structure);
-    RELEASE_ASSERT(structure);
-    m_structure = structure;
+    out.print("Structure transition from ", *m_structure);
 }
 
 void Structure::didTransitionFromThisStructure(DeferredStructureTransitionWatchpointFire* deferred) const
@@ -1060,10 +1058,11 @@ void Structure::didTransitionFromThisStructure(DeferredStructureTransitionWatchp
     // unwise to watch it.
     if (m_transitionWatchpointSet.isBeingWatched())
         const_cast<Structure*>(this)->setTransitionWatchpointIsLikelyToBeFired(true);
-
-    if (deferred)
-        deferred->add(this);
-    else
+
+    if (deferred) {
+        ASSERT(deferred->structure() == this);
+        m_transitionWatchpointSet.fireAll(*vm(), deferred);
+    } else
         m_transitionWatchpointSet.fireAll(*vm(), StructureFireDetail(this));
 }
 

diff --git a/Source/JavaScriptCore/runtime/Structure.h b/Source/JavaScriptCore/runtime/Structure.h
@@ -110,14 +110,16 @@ class StructureFireDetail : public FireDetail {
     const Structure* m_structure;
 };
 
-class DeferredStructureTransitionWatchpointFire {
+class DeferredStructureTransitionWatchpointFire : public DeferredWatchpointFire {
     WTF_MAKE_NONCOPYABLE(DeferredStructureTransitionWatchpointFire);
 public:
-    JS_EXPORT_PRIVATE DeferredStructureTransitionWatchpointFire();
+    JS_EXPORT_PRIVATE DeferredStructureTransitionWatchpointFire(VM&, Structure*);
     JS_EXPORT_PRIVATE ~DeferredStructureTransitionWatchpointFire();
 
-    void add(const Structure*);
-
+    void dump(PrintStream& out) const override;
+
+    const Structure* structure() const { return m_structure; }
+
 private:
     const Structure* m_structure;
 };