Changeset 223161 in webkit

Timestamp:

Oct 10, 2017 5:53:59 PM (7 years ago)

Author:

sbarati@apple.com

Message:

Prototype structure transition should be a deferred transition
​https://bugs.webkit.org/show_bug.cgi?id=177734

Reviewed by Keith Miller.

Absence ObjectPropertyConditions work by verifying both that the Structure
does not have a particular property and that its prototype has
remained constant. However, the prototype transition was firing
the transition watchpoint before setting the object's structure.
This meant that isValid for Absence would never return false because
the prototype changed. Clearly this is wrong. The reason this didn't
break OPCs in general is that we'd also check if we could still watch
the OPC. In this case, we can't still watch it because we're inspecting
a structure with an invalidated transition watchpoint. To fix
this weird quirk of the code, I'm making it so that doing a prototype
transition uses the DeferredStructureTransitionWatchpointFire machinery.

This patch also fixes some dead code that I left in regarding
poly proto in OPC.

• bytecode/PropertyCondition.cpp:

(JSC::PropertyCondition::isStillValidAssumingImpurePropertyWatchpoint const):

• runtime/JSObject.cpp:

(JSC::JSObject::setPrototypeDirect):

• runtime/Structure.cpp:

(JSC::Structure::changePrototypeTransition):

• runtime/Structure.h:

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• bytecode/PropertyCondition.cpp (modified) (1 diff)
• runtime/JSObject.cpp (modified) (1 diff)
• runtime/Structure.cpp (modified) (1 diff)
• runtime/Structure.h (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2017-10-10  Saam Barati  <sbarati@apple.com>
+
+        Prototype structure transition should be a deferred transition
+        https://bugs.webkit.org/show_bug.cgi?id=177734
+
+        Reviewed by Keith Miller.
+
+        Absence ObjectPropertyConditions work by verifying both that the Structure 
+        does not have a particular property and that its prototype has
+        remained constant. However, the prototype transition was firing
+        the transition watchpoint before setting the object's structure.
+        This meant that isValid for Absence would never return false because
+        the prototype changed. Clearly this is wrong. The reason this didn't
+        break OPCs in general is that we'd also check if we could still watch
+        the OPC. In this case, we can't still watch it because we're inspecting
+        a structure with an invalidated transition watchpoint. To fix
+        this weird quirk of the code, I'm making it so that doing a prototype
+        transition uses the DeferredStructureTransitionWatchpointFire machinery.
+        
+        This patch also fixes some dead code that I left in regarding
+        poly proto in OPC.
+
+        * bytecode/PropertyCondition.cpp:
+        (JSC::PropertyCondition::isStillValidAssumingImpurePropertyWatchpoint const):
+        * runtime/JSObject.cpp:
+        (JSC::JSObject::setPrototypeDirect):
+        * runtime/Structure.cpp:
+        (JSC::Structure::changePrototypeTransition):
+        * runtime/Structure.h:
+
 2017-10-10  Robin Morisset  <rmorisset@apple.com>
 

trunk/Source/JavaScriptCore/bytecode/PropertyCondition.cpp

@@ -123 +123 @@
         }
 
-        JSObject* currentPrototype;
-        if (structure->hasMonoProto())
-            currentPrototype = structure->storedPrototypeObject();
-        else {
-            RELEASE_ASSERT(base);
-            currentPrototype = jsDynamicCast<JSObject*>(*structure->vm(), base->getPrototypeDirect());
-        }
-
-        if (currentPrototype != prototype()) {
+        if (structure->storedPrototypeObject() != prototype()) {
             if (PropertyConditionInternal::verbose) {
                 dataLog(

trunk/Source/JavaScriptCore/runtime/JSObject.cpp

@@ -1635 +1635 @@
     
     if (structure(vm)->hasMonoProto()) {
-        Structure* newStructure = Structure::changePrototypeTransition(vm, structure(vm), prototype);
+        DeferredStructureTransitionWatchpointFire deferred;
+        Structure* newStructure = Structure::changePrototypeTransition(vm, structure(vm), prototype, deferred);
         setStructure(vm, newStructure);
     } else

trunk/Source/JavaScriptCore/runtime/Structure.cpp

@@ -546 +546 @@
 }
 
-Structure* Structure::changePrototypeTransition(VM& vm, Structure* structure, JSValue prototype)
+Structure* Structure::changePrototypeTransition(VM& vm, Structure* structure, JSValue prototype, DeferredStructureTransitionWatchpointFire& deferred)
 {
     ASSERT(prototype.isObject() || prototype.isNull());
 
     DeferGC deferGC(vm.heap);
-    Structure* transition = create(vm, structure);
+    Structure* transition = create(vm, structure, &deferred);
 
     transition->m_prototype.set(vm, transition, prototype);

trunk/Source/JavaScriptCore/runtime/Structure.h

@@ -182 +182 @@
     JS_EXPORT_PRIVATE static Structure* addPropertyTransitionToExistingStructure(Structure*, PropertyName, unsigned attributes, PropertyOffset&);
     static Structure* removePropertyTransition(VM&, Structure*, PropertyName, PropertyOffset&);
-    static Structure* changePrototypeTransition(VM&, Structure*, JSValue prototype);
+    static Structure* changePrototypeTransition(VM&, Structure*, JSValue prototype, DeferredStructureTransitionWatchpointFire&);
     JS_EXPORT_PRIVATE static Structure* attributeChangeTransition(VM&, Structure*, PropertyName, unsigned attributes);
     JS_EXPORT_PRIVATE static Structure* toCacheableDictionaryTransition(VM&, Structure*, DeferredStructureTransitionWatchpointFire* = nullptr);