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):
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@233124 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
index 92dd9b4..49cef2e 100644
--- a/Source/JavaScriptCore/ChangeLog
+++ b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,27 @@
+2018-06-22 Keith Miller <keith_miller@apple.com>
+
+ 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 <utatane.tea@gmail.com>
[WTF] Use Ref<> for the result type of non-failing factory functions
diff --git a/Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.cpp b/Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.cpp
index 49389c1..2d1963a 100644
--- a/Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.cpp
+++ b/Source/JavaScriptCore/bytecode/ObjectPropertyConditionSet.cpp
@@ -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;
}
diff --git a/Source/JavaScriptCore/bytecode/PropertyCondition.cpp b/Source/JavaScriptCore/bytecode/PropertyCondition.cpp
index 4203a16..43634be 100644
--- a/Source/JavaScriptCore/bytecode/PropertyCondition.cpp
+++ b/Source/JavaScriptCore/bytecode/PropertyCondition.cpp
@@ -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);
diff --git a/Source/JavaScriptCore/dfg/DFGGraph.cpp b/Source/JavaScriptCore/dfg/DFGGraph.cpp
index 95c6412..c4927fa 100644
--- a/Source/JavaScriptCore/dfg/DFGGraph.cpp
+++ b/Source/JavaScriptCore/dfg/DFGGraph.cpp
@@ -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)
diff --git a/Source/JavaScriptCore/runtime/JSObject.h b/Source/JavaScriptCore/runtime/JSObject.h
index c2699cb..3df0bd5 100644
--- a/Source/JavaScriptCore/runtime/JSObject.h
+++ b/Source/JavaScriptCore/runtime/JSObject.h
@@ -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)
