Title: [234022] trunk/Source/_javascript_Core
- Revision
- 234022
- Author
- [email protected]
- Date
- 2018-07-19 21:47:51 -0700 (Thu, 19 Jul 2018)
Log 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):
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (234021 => 234022)
--- trunk/Source/_javascript_Core/ChangeLog 2018-07-20 04:29:21 UTC (rev 234021)
+++ trunk/Source/_javascript_Core/ChangeLog 2018-07-20 04:47:51 UTC (rev 234022)
@@ -1,3 +1,30 @@
+2018-07-19 Saam Barati <[email protected]>
+
+ 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 <[email protected]>
Unreviewed, rolling out r233998.
Modified: trunk/Source/_javascript_Core/runtime/ObjectConstructor.cpp (234021 => 234022)
--- trunk/Source/_javascript_Core/runtime/ObjectConstructor.cpp 2018-07-20 04:29:21 UTC (rev 234021)
+++ trunk/Source/_javascript_Core/runtime/ObjectConstructor.cpp 2018-07-20 04:47:51 UTC (rev 234022)
@@ -318,43 +318,66 @@
RETURN_IF_EXCEPTION(scope, { });
}
- 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;
- };
+ 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);
- 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)
- 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.
- PropertyName propertyName(entry.key);
- if (propertyName.isPrivateName())
+ // 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;
+ });
+ }
+ }
+ 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;
}
}
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
