Attachment 339886 Details for Bug 185438 – Patch

[patch] Patch

185438.patch (text/plain), 11.40 KB, created by Michael Saboff on 2018-05-08 15:07:47 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Michael Saboff

Created: 2018-05-08 15:07:47 PDT

Size: 11.40 KB

patch

obsolete

>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 231515)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -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 NOBODY (OOPS!).
>+
>+        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: Source/JavaScriptCore/bytecode/Watchpoint.cpp
>===================================================================
>--- Source/JavaScriptCore/bytecode/Watchpoint.cpp	(revision 231493)
>+++ Source/JavaScriptCore/bytecode/Watchpoint.cpp	(working copy)
>@@ -92,6 +92,16 @@ void WatchpointSet::fireAllSlow(VM& vm, 
>     WTF::storeStoreFence();
> }
> 
>+void WatchpointSet::fireAllSlow(VM&, DeferredWatchpointFire* deferredWatchpoints)
>+{
>+    ASSERT(state() == IsWatched);
>+
>+    WTF::storeStoreFence();
>+    deferredWatchpoints->takeWatchpointsToFire(this);
>+    m_state = IsInvalidated; // Do after moving watchpoints to deferred to transfer original.
>+    WTF::storeStoreFence();
>+}
>+
> void WatchpointSet::fireAllSlow(VM& vm, const char* reason)
> {
>     fireAllSlow(vm, StringFireDetail(reason));
>@@ -133,6 +143,15 @@ void WatchpointSet::fireAllWatchpoints(V
>     }
> }
> 
>+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: Source/JavaScriptCore/bytecode/Watchpoint.h
>===================================================================
>--- Source/JavaScriptCore/bytecode/Watchpoint.h	(revision 231493)
>+++ Source/JavaScriptCore/bytecode/Watchpoint.h	(working copy)
>@@ -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: Source/JavaScriptCore/runtime/JSObject.cpp
>===================================================================
>--- Source/JavaScriptCore/runtime/JSObject.cpp	(revision 231493)
>+++ Source/JavaScriptCore/runtime/JSObject.cpp	(working copy)
>@@ -1647,7 +1647,7 @@ void JSObject::setPrototypeDirect(VM& vm
>         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(JSObjec
> 
> void JSObject::convertToDictionary(VM& vm)
> {
>-    DeferredStructureTransitionWatchpointFire deferredWatchpointFire;
>+    DeferredStructureTransitionWatchpointFire deferredWatchpointFire(vm, structure(vm));
>     setStructure(
>         vm, Structure::toCacheableDictionaryTransition(vm, structure(vm), &deferredWatchpointFire));
> }
>Index: Source/JavaScriptCore/runtime/JSObjectInlines.h
>===================================================================
>--- Source/JavaScriptCore/runtime/JSObjectInlines.h	(revision 231493)
>+++ Source/JavaScriptCore/runtime/JSObjectInlines.h	(working copy)
>@@ -362,7 +362,7 @@ ALWAYS_INLINE bool JSObject::putDirectIn
>     // 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: Source/JavaScriptCore/runtime/Structure.cpp
>===================================================================
>--- Source/JavaScriptCore/runtime/Structure.cpp	(revision 231493)
>+++ Source/JavaScriptCore/runtime/Structure.cpp	(working copy)
>@@ -201,7 +201,6 @@ Structure::Structure(VM& vm, JSGlobalObj
>     setTransitionWatchpointIsLikelyToBeFired(false);
>     setHasBeenDictionary(false);
>     setIsAddingPropertyForTransition(false);
>- 
>     ASSERT(inlineCapacity <= JSFinalObject::maxInlineCapacity());
>     ASSERT(static_cast<PropertyOffset>(inlineCapacity) < firstOutOfLineOffset);
>     ASSERT(!hasRareData());
>@@ -1032,22 +1031,20 @@ void StructureFireDetail::dump(PrintStre
>     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 +1054,11 @@ void Structure::didTransitionFromThisStr
>     // 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: Source/JavaScriptCore/runtime/Structure.h
>===================================================================
>--- Source/JavaScriptCore/runtime/Structure.h	(revision 231493)
>+++ Source/JavaScriptCore/runtime/Structure.h	(working copy)
>@@ -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;
> };

Flags:

saam: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 185438: 339886