- Revision
- 233124
- Author
- [email protected]
- Date
- 2018-06-23 03:47:58 -0700 (Sat, 23 Jun 2018)
Log Message
We need to have a getDirectConcurrently for use in the compilers
https://bugs.webkit.org/show_bug.cgi?id=186954
Reviewed by Mark Lam.
It used to be that the propertyStorage of an object never shrunk
so if you called getDirect with some offset it would never be an
OOB read. However, this property storage can shrink when calling
flattenDictionaryStructure. Fortunately, flattenDictionaryStructure
holds the Structure's ConcurrentJSLock while shrinking. This patch,
adds a getDirectConcurrently that will safely try to load from the
butterfly.
* bytecode/ObjectPropertyConditionSet.cpp:
* bytecode/PropertyCondition.cpp:
(JSC::PropertyCondition::isStillValidAssumingImpurePropertyWatchpoint const):
(JSC::PropertyCondition::attemptToMakeEquivalenceWithoutBarrier const):
* dfg/DFGGraph.cpp:
(JSC::DFG::Graph::tryGetConstantProperty):
* runtime/JSObject.h:
(JSC::JSObject::getDirectConcurrently const):
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (233123 => 233124)
--- trunk/Source/_javascript_Core/ChangeLog 2018-06-23 09:59:04 UTC (rev 233123)
+++ trunk/Source/_javascript_Core/ChangeLog 2018-06-23 10:47:58 UTC (rev 233124)
@@ -1,3 +1,27 @@
+2018-06-22 Keith Miller <[email protected]>
+
+ We need to have a getDirectConcurrently for use in the compilers
+ https://bugs.webkit.org/show_bug.cgi?id=186954
+
+ Reviewed by Mark Lam.
+
+ It used to be that the propertyStorage of an object never shrunk
+ so if you called getDirect with some offset it would never be an
+ OOB read. However, this property storage can shrink when calling
+ flattenDictionaryStructure. Fortunately, flattenDictionaryStructure
+ holds the Structure's ConcurrentJSLock while shrinking. This patch,
+ adds a getDirectConcurrently that will safely try to load from the
+ butterfly.
+
+ * bytecode/ObjectPropertyConditionSet.cpp:
+ * bytecode/PropertyCondition.cpp:
+ (JSC::PropertyCondition::isStillValidAssumingImpurePropertyWatchpoint const):
+ (JSC::PropertyCondition::attemptToMakeEquivalenceWithoutBarrier const):
+ * dfg/DFGGraph.cpp:
+ (JSC::DFG::Graph::tryGetConstantProperty):
+ * runtime/JSObject.h:
+ (JSC::JSObject::getDirectConcurrently const):
+
2018-06-22 Yusuke Suzuki <[email protected]>
[WTF] Use Ref<> for the result type of non-failing factory functions
Modified: trunk/Source/_javascript_Core/bytecode/ObjectPropertyConditionSet.cpp (233123 => 233124)
--- trunk/Source/_javascript_Core/bytecode/ObjectPropertyConditionSet.cpp 2018-06-23 09:59:04 UTC (rev 233123)
+++ trunk/Source/_javascript_Core/bytecode/ObjectPropertyConditionSet.cpp 2018-06-23 10:47:58 UTC (rev 233124)
@@ -222,7 +222,9 @@
PropertyOffset offset = structure->getConcurrently(uid, attributes);
if (offset == invalidOffset)
return ObjectPropertyCondition();
- JSValue value = object->getDirect(offset);
+ JSValue value = object->getDirectConcurrently(structure, offset);
+ if (!value)
+ return ObjectPropertyCondition();
result = ObjectPropertyCondition::equivalence(vm, owner, object, uid, value);
break;
}
Modified: trunk/Source/_javascript_Core/bytecode/PropertyCondition.cpp (233123 => 233124)
--- trunk/Source/_javascript_Core/bytecode/PropertyCondition.cpp 2018-06-23 09:59:04 UTC (rev 233123)
+++ trunk/Source/_javascript_Core/bytecode/PropertyCondition.cpp 2018-06-23 10:47:58 UTC (rev 233124)
@@ -237,7 +237,7 @@
return false;
}
- JSValue currentValue = base->getDirect(currentOffset);
+ JSValue currentValue = base->getDirectConcurrently(structure, currentOffset);
if (currentValue != requiredValue()) {
if (PropertyConditionInternal::verbose) {
dataLog(
@@ -392,9 +392,8 @@
PropertyCondition PropertyCondition::attemptToMakeEquivalenceWithoutBarrier(VM& vm, JSObject* base) const
{
Structure* structure = base->structure(vm);
- if (!structure->isValidOffset(offset()))
- return PropertyCondition();
- JSValue value = base->getDirect(offset());
+
+ JSValue value = base->getDirectConcurrently(structure, offset());
if (!isValidValueForPresence(vm, value))
return PropertyCondition();
return equivalenceWithoutBarrier(uid(), value);
Modified: trunk/Source/_javascript_Core/dfg/DFGGraph.cpp (233123 => 233124)
--- trunk/Source/_javascript_Core/dfg/DFGGraph.cpp 2018-06-23 09:59:04 UTC (rev 233123)
+++ trunk/Source/_javascript_Core/dfg/DFGGraph.cpp 2018-06-23 10:47:58 UTC (rev 233124)
@@ -1244,7 +1244,7 @@
for (unsigned i = structureSet.size(); i--;) {
RegisteredStructure structure = structureSet[i];
-
+
WatchpointSet* set = structure->propertyReplacementWatchpointSet(offset);
if (!set || !set->isStillValid())
return JSValue();
@@ -1263,22 +1263,24 @@
//
// One argument in favor of this code is that it should definitely work because the butterfly
// is always set before the structure. However, we don't currently have a fence between those
- // stores. It's not clear if this matters, however. We don't ever shrink the property storage.
- // So, for this to fail, you'd need an access on a constant object pointer such that the inline
- // caches told us that the object had a structure that it did not *yet* have, and then later,
- // the object transitioned to that structure that the inline caches had alraedy seen. And then
- // the processor reordered the stores. Seems unlikely and difficult to test. I believe that
- // this is worth revisiting but it isn't worth losing sleep over. Filed:
+ // stores. It's not clear if this matters, however. We only shrink the propertyStorage while
+ // holding the Structure's lock. So, for this to fail, you'd need an access on a constant
+ // object pointer such that the inline caches told us that the object had a structure that it
+ // did not *yet* have, and then later,the object transitioned to that structure that the inline
+ // caches had already seen. And then the processor reordered the stores. Seems unlikely and
+ // difficult to test. I believe that this is worth revisiting but it isn't worth losing sleep
+ // over. Filed:
// https://bugs.webkit.org/show_bug.cgi?id=134641
//
// For now, we just do the minimal thing: defend against the structure right now being
// incompatible with the getDirect we're trying to do. The easiest way to do that is to
// determine if the structure belongs to the proven set.
-
- if (!structureSet.toStructureSet().contains(object->structure(m_vm)))
+
+ Structure* structure = object->structure(m_vm);
+ if (!structureSet.toStructureSet().contains(structure))
return JSValue();
-
- return object->getDirect(offset);
+
+ return object->getDirectConcurrently(structure, offset);
}
JSValue Graph::tryGetConstantProperty(JSValue base, Structure* structure, PropertyOffset offset)
Modified: trunk/Source/_javascript_Core/runtime/JSObject.h (233123 => 233124)
--- trunk/Source/_javascript_Core/runtime/JSObject.h 2018-06-23 09:59:04 UTC (rev 233123)
+++ trunk/Source/_javascript_Core/runtime/JSObject.h 2018-06-23 10:47:58 UTC (rev 233124)
@@ -717,6 +717,7 @@
// Fast access to known property offsets.
ALWAYS_INLINE JSValue getDirect(PropertyOffset offset) const { return locationForOffset(offset)->get(); }
+ JSValue getDirectConcurrently(Structure* expectedStructure, PropertyOffset) const;
void putDirect(VM& vm, PropertyOffset offset, JSValue value) { locationForOffset(offset)->set(vm, this, value); }
void putDirectWithoutBarrier(PropertyOffset offset, JSValue value) { locationForOffset(offset)->setWithoutWriteBarrier(value); }
void putDirectUndefined(PropertyOffset offset) { locationForOffset(offset)->setUndefined(); }
@@ -1320,6 +1321,18 @@
return getPrototypeMethod(this, exec);
}
+// Normally, we never shrink the butterfly so if we know an offset is valid for some
+// past structure then it should be valid for any new structure. However, we may sometimes
+// shrink the butterfly when we are holding the Structure's ConcurrentJSLock, such as when we
+// flatten an object.
+inline JSValue JSObject::getDirectConcurrently(Structure* structure, PropertyOffset offset) const
+{
+ ConcurrentJSLocker locker(structure->lock());
+ if (!structure->isValidOffset(offset))
+ return { };
+ return getDirect(offset);
+}
+
// It is safe to call this method with a PropertyName that is actually an index,
// but if so will always return false (doesn't search index storage).
ALWAYS_INLINE bool JSObject::getOwnNonIndexPropertySlot(VM& vm, Structure* structure, PropertyName propertyName, PropertySlot& slot)
