PropertySlot should not have Customs have a PropertyOffset of zero
https://bugs.webkit.org/show_bug.cgi?id=204566
<rdar://problem/57466781>
Reviewed by Keith Miller.
JSTests:
* stress/cacheable-custom-accessor-should-not-have-property-offset.js: Added.
Source/JavaScriptCore:
We used to say that PropertyOffset of a cacheable custom was always zero. We
did this because we were using "invalidOffset" to indicate things aren't
cacheable. This patch refactors PropertySlot to not look at PropertyOffset
for cacheability, but instead just uses the cacheability bit. With that
change, we now say that customs always have the invalid PropertyOffset. This
fixes a bug where we used to watch for property changes at the offset inside
an AccessCase. We were doing this for the zero property offset for all
customs. This could trigger a crash inside startWatchingPropertyForReplacements
because the prototype Structure was a dictionary. We allow dictionaries to
be property holders of customs as long as the property is a custom and has
DontDelete property attribute, since DontDelete proves the custom will never
change.
* llint/LLIntSlowPaths.cpp:
(JSC::LLInt::LLINT_SLOW_PATH_DECL):
* runtime/PropertySlot.h:
(JSC::PropertySlot::PropertySlot):
(JSC::PropertySlot::isCacheable const):
(JSC::PropertySlot::setValue):
(JSC::PropertySlot::setCustom):
(JSC::PropertySlot::setCacheableCustom):
(JSC::PropertySlot::setCustomGetterSetter):
(JSC::PropertySlot::setGetterSlot):
(JSC::PropertySlot::setCacheableGetterSlot):
(JSC::PropertySlot::setUndefined):
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@253026 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
index 6c63af8..068673d 100644
--- a/JSTests/ChangeLog
+++ b/JSTests/ChangeLog
@@ -1,3 +1,13 @@
+2019-12-02 Saam Barati <sbarati@apple.com>
+
+ PropertySlot should not have Customs have a PropertyOffset of zero
+ https://bugs.webkit.org/show_bug.cgi?id=204566
+ <rdar://problem/57466781>
+
+ Reviewed by Keith Miller.
+
+ * stress/cacheable-custom-accessor-should-not-have-property-offset.js: Added.
+
2019-12-02 Mark Lam <mark.lam@apple.com>
mozilla-tests.yaml/js1_5/Array/regress-101964.js is frequently failing on JSC EWS bots.
diff --git a/JSTests/stress/cacheable-custom-accessor-should-not-have-property-offset.js b/JSTests/stress/cacheable-custom-accessor-should-not-have-property-offset.js
new file mode 100644
index 0000000..c4cae2e
--- /dev/null
+++ b/JSTests/stress/cacheable-custom-accessor-should-not-have-property-offset.js
@@ -0,0 +1,21 @@
+function main() {
+ const v1 = {length:parseInt};
+ for (let v6 = 0; v6 < 100; v6 = v6 + 2.0) {
+ function v8(v9,v10,v11,v12) {
+ try {
+ const v13 = v9();
+ const v15 = {set:parseInt};
+ const v17 = Object.defineProperty(v13,4294967295,v15);
+ v1.__proto__ = v13;
+ const v18 = v1.arguments;
+ } catch(v19) {
+ }
+ return v8;
+ }
+ const v21 = [293729.1679360643, 2635518607, 293729.1679360643, 293729.1679360643, 293729.1679360643];
+ const v22 = v21.reduce(v8);
+ }
+}
+noDFG(main);
+noFTL(main);
+main();
diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
index b65e340..3466f5e 100644
--- a/Source/JavaScriptCore/ChangeLog
+++ b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,37 @@
+2019-12-02 Saam Barati <sbarati@apple.com>
+
+ PropertySlot should not have Customs have a PropertyOffset of zero
+ https://bugs.webkit.org/show_bug.cgi?id=204566
+ <rdar://problem/57466781>
+
+ Reviewed by Keith Miller.
+
+ We used to say that PropertyOffset of a cacheable custom was always zero. We
+ did this because we were using "invalidOffset" to indicate things aren't
+ cacheable. This patch refactors PropertySlot to not look at PropertyOffset
+ for cacheability, but instead just uses the cacheability bit. With that
+ change, we now say that customs always have the invalid PropertyOffset. This
+ fixes a bug where we used to watch for property changes at the offset inside
+ an AccessCase. We were doing this for the zero property offset for all
+ customs. This could trigger a crash inside startWatchingPropertyForReplacements
+ because the prototype Structure was a dictionary. We allow dictionaries to
+ be property holders of customs as long as the property is a custom and has
+ DontDelete property attribute, since DontDelete proves the custom will never
+ change.
+
+ * llint/LLIntSlowPaths.cpp:
+ (JSC::LLInt::LLINT_SLOW_PATH_DECL):
+ * runtime/PropertySlot.h:
+ (JSC::PropertySlot::PropertySlot):
+ (JSC::PropertySlot::isCacheable const):
+ (JSC::PropertySlot::setValue):
+ (JSC::PropertySlot::setCustom):
+ (JSC::PropertySlot::setCacheableCustom):
+ (JSC::PropertySlot::setCustomGetterSetter):
+ (JSC::PropertySlot::setGetterSlot):
+ (JSC::PropertySlot::setCacheableGetterSlot):
+ (JSC::PropertySlot::setUndefined):
+
2019-12-02 Yusuke Suzuki <ysuzuki@apple.com>
[JSC] Put some destructible objects to IsoSubspace
diff --git a/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp b/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp
index 7316e95..357a46e 100644
--- a/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp
+++ b/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp
@@ -663,7 +663,7 @@
JSValue result = found ? slot.getValue(globalObject, ident) : jsUndefined();
LLINT_CHECK_EXCEPTION();
- if (!LLINT_ALWAYS_ACCESS_SLOW && slot.isCacheable()) {
+ if (!LLINT_ALWAYS_ACCESS_SLOW && slot.isCacheable() && !slot.isUnset()) {
auto& metadata = bytecode.metadata(codeBlock);
{
StructureID oldStructureID = metadata.m_structureID;
@@ -773,7 +773,8 @@
if (!LLINT_ALWAYS_ACCESS_SLOW
&& baseValue.isCell()
- && slot.isCacheable()) {
+ && slot.isCacheable()
+ && !slot.isUnset()) {
{
StructureID oldStructureID;
switch (metadata.m_modeMetadata.mode) {
@@ -815,7 +816,7 @@
metadata.m_modeMetadata.defaultMode.cachedOffset = slot.cachedOffset();
vm.heap.writeBarrier(codeBlock);
}
- } else if (UNLIKELY(metadata.m_modeMetadata.hitCountForLLIntCaching && (slot.isValue() || slot.isUnset()))) {
+ } else if (UNLIKELY(metadata.m_modeMetadata.hitCountForLLIntCaching && slot.isValue())) {
ASSERT(slot.slotBase() != baseValue);
if (!(--metadata.m_modeMetadata.hitCountForLLIntCaching))
diff --git a/Source/JavaScriptCore/runtime/PropertySlot.h b/Source/JavaScriptCore/runtime/PropertySlot.h
index b21de3e..f776c62 100644
--- a/Source/JavaScriptCore/runtime/PropertySlot.h
+++ b/Source/JavaScriptCore/runtime/PropertySlot.h
@@ -117,7 +117,7 @@
, m_thisValue(thisValue)
, m_slotBase(nullptr)
, m_watchpointSet(nullptr)
- , m_cacheability(CachingAllowed)
+ , m_cacheability(CachingDisallowed)
, m_propertyType(TypeUnset)
, m_internalMethodType(internalMethodType)
, m_additionalDataType(AdditionalDataType::None)
@@ -133,7 +133,7 @@
JSValue getValue(JSGlobalObject*, unsigned propertyName) const;
JSValue getPureResult() const;
- bool isCacheable() const { return m_cacheability == CachingAllowed && m_offset != invalidOffset; }
+ bool isCacheable() const { return isUnset() || m_cacheability == CachingAllowed; }
bool isUnset() const { return m_propertyType == TypeUnset; }
bool isValue() const { return m_propertyType == TypeValue; }
bool isAccessor() const { return m_propertyType == TypeGetter; }
@@ -217,7 +217,8 @@
ASSERT(slotBase);
m_slotBase = slotBase;
m_propertyType = TypeValue;
- m_offset = invalidOffset;
+
+ ASSERT(m_cacheability == CachingDisallowed);
}
void setValue(JSObject* slotBase, unsigned attributes, JSValue value, PropertyOffset offset)
@@ -232,6 +233,8 @@
m_slotBase = slotBase;
m_propertyType = TypeValue;
m_offset = offset;
+
+ m_cacheability = CachingAllowed;
}
void setValue(JSString*, unsigned attributes, JSValue value)
@@ -244,7 +247,8 @@
m_slotBase = 0;
m_propertyType = TypeValue;
- m_offset = invalidOffset;
+
+ ASSERT(m_cacheability == CachingDisallowed);
}
void setValueModuleNamespace(JSObject* slotBase, unsigned attributes, JSValue value, JSModuleEnvironment* environment, ScopeOffset scopeOffset)
@@ -267,7 +271,7 @@
ASSERT(slotBase);
m_slotBase = slotBase;
m_propertyType = TypeCustom;
- m_offset = invalidOffset;
+ ASSERT(m_cacheability == CachingDisallowed);
}
void setCustom(JSObject* slotBase, unsigned attributes, GetValueFunc getValue, DOMAttributeAnnotation domAttribute)
@@ -289,7 +293,8 @@
ASSERT(slotBase);
m_slotBase = slotBase;
m_propertyType = TypeCustom;
- m_offset = !invalidOffset;
+
+ m_cacheability = CachingAllowed;
}
void setCacheableCustom(JSObject* slotBase, unsigned attributes, GetValueFunc getValue, DOMAttributeAnnotation domAttribute)
@@ -313,7 +318,8 @@
ASSERT(slotBase);
m_slotBase = slotBase;
m_propertyType = TypeCustomAccessor;
- m_offset = invalidOffset;
+
+ ASSERT(m_cacheability == CachingDisallowed);
}
void setGetterSlot(JSObject* slotBase, unsigned attributes, GetterSetter* getterSetter)
@@ -327,7 +333,8 @@
ASSERT(slotBase);
m_slotBase = slotBase;
m_propertyType = TypeGetter;
- m_offset = invalidOffset;
+
+ ASSERT(m_cacheability == CachingDisallowed);
}
void setCacheableGetterSlot(JSObject* slotBase, unsigned attributes, GetterSetter* getterSetter, PropertyOffset offset)
@@ -342,6 +349,8 @@
m_slotBase = slotBase;
m_propertyType = TypeGetter;
m_offset = offset;
+
+ m_cacheability = CachingAllowed;
}
JSValue thisValue() const
@@ -361,7 +370,6 @@
m_slotBase = 0;
m_propertyType = TypeValue;
- m_offset = invalidOffset;
}
void setWatchpointSet(WatchpointSet& set)
