Changeset 234022 in webkit

Timestamp:

Jul 19, 2018 9:47:51 PM (6 years ago)

Author:

sbarati@apple.com

Message:

Conservatively make Object.assign's fast path do a two phase protocol of loading everything then storing everything to try to prevent a crash
​https://bugs.webkit.org/show_bug.cgi?id=187836
<rdar://problem/42409527>

Reviewed by Mark Lam.

We have crash reports that we're crashing on source->getDirect in Object.assign's
fast path. Mark investigated this and determined we end up with a nullptr for
butterfly. This is curious, because source's Structure indicated that it has
out of line properties. My leading hypothesis for this at the moment is a bit
handwavy, but it's essentially:

• We end up firing a watchpoint when assigning to the target (this can happen

if a watchpoint was set up for storing to that particular field)

• When we fire that watchpoint, we end up doing some kind work on the source,

perhaps causing it to flattenDictionaryStructure. Therefore, we end up
mutating source.

I'm not super convinced this is what we're running into, but just by reading
the code, I think it needs to be something similar to this. Seeing if this change
fixes the crasher will give us good data to determine if something like this is
happening or if the bug is something else entirely.

• runtime/ObjectConstructor.cpp:

(JSC::objectConstructorAssign):

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• runtime/ObjectConstructor.cpp (modified) (1 diff)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2018-07-19  Saam Barati  <sbarati@apple.com>
+
+        Conservatively make Object.assign's fast path do a two phase protocol of loading everything then storing everything to try to prevent a crash
+        https://bugs.webkit.org/show_bug.cgi?id=187836
+        <rdar://problem/42409527>
+
+        Reviewed by Mark Lam.
+
+        We have crash reports that we're crashing on source->getDirect in Object.assign's
+        fast path. Mark investigated this and determined we end up with a nullptr for
+        butterfly. This is curious, because source's Structure indicated that it has
+        out of line properties. My leading hypothesis for this at the moment is a bit
+        handwavy, but it's essentially:
+        - We end up firing a watchpoint when assigning to the target (this can happen
+        if a watchpoint was set up for storing to that particular field)
+        - When we fire that watchpoint, we end up doing some kind work on the source,
+        perhaps causing it to flattenDictionaryStructure. Therefore, we end up
+        mutating source.
+        
+        I'm not super convinced this is what we're running into, but just by reading
+        the code, I think it needs to be something similar to this. Seeing if this change
+        fixes the crasher will give us good data to determine if something like this is
+        happening or if the bug is something else entirely.
+
+        * runtime/ObjectConstructor.cpp:
+        (JSC::objectConstructorAssign):
+
 2018-07-19  Commit Queue  <commit-queue@webkit.org>
 

trunk/Source/JavaScriptCore/runtime/ObjectConstructor.cpp

@@ -319 +319 @@
             }
 
-            auto canPerformFastPropertyEnumerationForObjectAssign = [] (Structure* structure) {
-                if (structure->typeInfo().overridesGetOwnPropertySlot())
-                    return false;
-                if (structure->typeInfo().overridesGetPropertyNames())
-                    return false;
-                // FIXME: Indexed properties can be handled.
-                // https://bugs.webkit.org/show_bug.cgi?id=185358
-                if (hasIndexedProperties(structure->indexingType()))
-                    return false;
-                if (structure->hasGetterSetterProperties())
-                    return false;
-                if (structure->isUncacheableDictionary())
-                    return false;
-                // Cannot perform fast [[Put]] to |target| if the property names of the |source| contain "__proto__".
-                if (structure->hasUnderscoreProtoPropertyExcludingOriginalProto())
-                    return false;
-                return true;
-            };
-
-            Structure* structure = source->structure(vm);
-            if (canPerformFastPropertyEnumerationForObjectAssign(structure)) {
-                // |source| Structure does not have any getters. And target can perform fast put.
-                // So enumerating properties and putting properties are non observable.
-                structure->forEachProperty(vm, [&] (const PropertyMapEntry& entry) -> bool {
-                    if (entry.attributes & PropertyAttribute::DontEnum)
+            bool canDoFastPath;
+            Vector<RefPtr<UniquedStringImpl>, 8> properties;
+            MarkedArgumentBuffer values;
+            {
+                auto canPerformFastPropertyEnumerationForObjectAssign = [] (Structure* structure) {
+                    if (structure->typeInfo().overridesGetOwnPropertySlot())
+                        return false;
+                    if (structure->typeInfo().overridesGetPropertyNames())
+                        return false;
+                    // FIXME: Indexed properties can be handled.
+                    // https://bugs.webkit.org/show_bug.cgi?id=185358
+                    if (hasIndexedProperties(structure->indexingType()))
+                        return false;
+                    if (structure->hasGetterSetterProperties())
+                        return false;
+                    if (structure->isUncacheableDictionary())
+                        return false;
+                    // Cannot perform fast [[Put]] to |target| if the property names of the |source| contain "__proto__".
+                    if (structure->hasUnderscoreProtoPropertyExcludingOriginalProto())
+                        return false;
+                    return true;
+                };
+
+                Structure* structure = source->structure(vm);
+                canDoFastPath = canPerformFastPropertyEnumerationForObjectAssign(structure);
+                if (canDoFastPath) {
+                    // |source| Structure does not have any getters. And target can perform fast put.
+                    // So enumerating properties and putting properties are non observable.
+
+                    // FIXME: It doesn't seem like we should have to do this in two phases, but
+                    // we're running into crashes where it appears that source is transitioning
+                    // under us, and even ends up in a state where it has a null butterfly. My
+                    // leading hypothesis here is that we fire some value replacement watchpoint
+                    // that ends up transitioning the structure underneath us.
+                    // https://bugs.webkit.org/show_bug.cgi?id=187837
+
+                    structure->forEachProperty(vm, [&] (const PropertyMapEntry& entry) -> bool {
+                        if (entry.attributes & PropertyAttribute::DontEnum)
+                            return true;
+
+                        PropertyName propertyName(entry.key);
+                        if (propertyName.isPrivateName())
+                            return true;
+                        
+                        properties.append(entry.key);
+                        values.appendWithCrashOnOverflow(source->getDirect(entry.offset));
+
                         return true;
-
-                    PropertyName propertyName(entry.key);
-                    if (propertyName.isPrivateName())
-                        return true;
-
+                    });
+                }
+            }
+
+            if (canDoFastPath) {
+                for (size_t i = 0; i < properties.size(); ++i) {
                     // FIXME: We could put properties in a batching manner to accelerate Object.assign more.
                     // https://bugs.webkit.org/show_bug.cgi?id=185358
                     PutPropertySlot putPropertySlot(target, true);
-                    target->putOwnDataProperty(vm, propertyName, source->getDirect(entry.offset), putPropertySlot);
-                    return true;
-                });
+                    target->putOwnDataProperty(vm, properties[i].get(), values.at(i), putPropertySlot);
+                }
+
                 continue;
             }