Deferred firing of structure transition watchpoints is racy
authormsaboff@apple.com <msaboff@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 8 May 2018 23:20:33 +0000 (23:20 +0000)
committermsaboff@apple.com <msaboff@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 8 May 2018 23:20:33 +0000 (23:20 +0000)
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):

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/Watchpoint.cpp
Source/JavaScriptCore/bytecode/Watchpoint.h
Source/JavaScriptCore/runtime/JSObject.cpp
Source/JavaScriptCore/runtime/JSObjectInlines.h
Source/JavaScriptCore/runtime/Structure.cpp
Source/JavaScriptCore/runtime/Structure.h

index 84c8f93..6293d12 100644 (file)
@@ -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-05-08  Eric Carlson  <eric.carlson@apple.com>
 
         Consecutive messages logged as JSON are coalesced
index fbe952d..7ab385f 100644 (file)
@@ -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
 
index cc6a34f..052a7b9 100644 (file)
@@ -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 @@ public:
         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 @@ public:
     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 @@ public:
         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 @@ private:
     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
index 42c888e..2ac12d9 100644 (file)
@@ -1647,7 +1647,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
@@ -3534,7 +3534,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));
 }
index 5e2701f..b3f2786 100644 (file)
@@ -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);
index a66ce56..3d6d24d 100644 (file)
@@ -1032,22 +1032,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
@@ -1057,10 +1055,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));
 }
 
index 37696bb..0aaa139 100644 (file)
@@ -110,14 +110,16 @@ private:
     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;
 };