- Revision
- 225768
- Author
- [email protected]
- Date
- 2017-12-11 19:24:43 -0800 (Mon, 11 Dec 2017)
Log Message
We need to disableCaching() in ErrorInstance when we materialize properties
https://bugs.webkit.org/show_bug.cgi?id=180343
<rdar://problem/35833002>
Reviewed by Mark Lam.
JSTests:
* stress/disable-caching-when-lazy-materializing-error-property-on-put.js: Added.
(assert):
(makeError):
(storeToStack):
(storeToStackAlreadyMaterialized):
Source/_javascript_Core:
This patch fixes a bug in ErrorInstance where we forgot to call PutPropertySlot::disableCaching
on puts() to a property that we lazily materialized. Forgetting to do this goes against the
PutPropertySlot's caching API. This lazy materialization caused the ErrorInstance to transition
from a Structure A to a Structure B. However, we were telling the IC that we were caching an
existing property only found on Structure B. This is obviously wrong as it would lead to an
OOB store if we didn't already crash when generating the IC.
* jit/Repatch.cpp:
(JSC::tryCachePutByID):
* runtime/ErrorInstance.cpp:
(JSC::ErrorInstance::materializeErrorInfoIfNeeded):
(JSC::ErrorInstance::put):
* runtime/ErrorInstance.h:
* runtime/Structure.cpp:
(JSC::Structure::didCachePropertyReplacement):
Modified Paths
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (225767 => 225768)
--- trunk/JSTests/ChangeLog 2017-12-12 02:45:13 UTC (rev 225767)
+++ trunk/JSTests/ChangeLog 2017-12-12 03:24:43 UTC (rev 225768)
@@ -1,3 +1,17 @@
+2017-12-11 Saam Barati <[email protected]>
+
+ We need to disableCaching() in ErrorInstance when we materialize properties
+ https://bugs.webkit.org/show_bug.cgi?id=180343
+ <rdar://problem/35833002>
+
+ Reviewed by Mark Lam.
+
+ * stress/disable-caching-when-lazy-materializing-error-property-on-put.js: Added.
+ (assert):
+ (makeError):
+ (storeToStack):
+ (storeToStackAlreadyMaterialized):
+
2017-12-05 JF Bastien <[email protected]>
WebAssembly: don't eagerly checksum
Added: trunk/JSTests/stress/disable-caching-when-lazy-materializing-error-property-on-put.js (0 => 225768)
--- trunk/JSTests/stress/disable-caching-when-lazy-materializing-error-property-on-put.js (rev 0)
+++ trunk/JSTests/stress/disable-caching-when-lazy-materializing-error-property-on-put.js 2017-12-12 03:24:43 UTC (rev 225768)
@@ -0,0 +1,27 @@
+function assert(b) {
+ if (!b)
+ throw new Error;
+}
+
+function makeError() { return new Error; }
+noInline(makeError);
+
+function storeToStack(e) {
+ e.stack = "foo";
+}
+noInline(storeToStack);
+
+function storeToStackAlreadyMaterialized(e) {
+ e.stack = "bar";
+}
+noInline(storeToStackAlreadyMaterialized);
+
+for (let i = 0; i < 10000; ++i) {
+ let e = makeError();
+ storeToStack(e);
+ assert(e.stack === "foo");
+ if (!!(i % 2))
+ e.fooBar = 25;
+ storeToStackAlreadyMaterialized(e);
+ assert(e.stack === "bar");
+}
Modified: trunk/Source/_javascript_Core/ChangeLog (225767 => 225768)
--- trunk/Source/_javascript_Core/ChangeLog 2017-12-12 02:45:13 UTC (rev 225767)
+++ trunk/Source/_javascript_Core/ChangeLog 2017-12-12 03:24:43 UTC (rev 225768)
@@ -1,3 +1,27 @@
+2017-12-11 Saam Barati <[email protected]>
+
+ We need to disableCaching() in ErrorInstance when we materialize properties
+ https://bugs.webkit.org/show_bug.cgi?id=180343
+ <rdar://problem/35833002>
+
+ Reviewed by Mark Lam.
+
+ This patch fixes a bug in ErrorInstance where we forgot to call PutPropertySlot::disableCaching
+ on puts() to a property that we lazily materialized. Forgetting to do this goes against the
+ PutPropertySlot's caching API. This lazy materialization caused the ErrorInstance to transition
+ from a Structure A to a Structure B. However, we were telling the IC that we were caching an
+ existing property only found on Structure B. This is obviously wrong as it would lead to an
+ OOB store if we didn't already crash when generating the IC.
+
+ * jit/Repatch.cpp:
+ (JSC::tryCachePutByID):
+ * runtime/ErrorInstance.cpp:
+ (JSC::ErrorInstance::materializeErrorInfoIfNeeded):
+ (JSC::ErrorInstance::put):
+ * runtime/ErrorInstance.h:
+ * runtime/Structure.cpp:
+ (JSC::Structure::didCachePropertyReplacement):
+
2017-12-11 Fujii Hironori <[email protected]>
[WinCairo] DLLLauncherMain should use SetDllDirectory
Modified: trunk/Source/_javascript_Core/jit/Repatch.cpp (225767 => 225768)
--- trunk/Source/_javascript_Core/jit/Repatch.cpp 2017-12-12 02:45:13 UTC (rev 225767)
+++ trunk/Source/_javascript_Core/jit/Repatch.cpp 2017-12-12 03:24:43 UTC (rev 225768)
@@ -422,6 +422,14 @@
if (slot.base() == baseValue && slot.isCacheablePut()) {
if (slot.type() == PutPropertySlot::ExistingProperty) {
+ // This assert helps catch bugs if we accidentally forget to disable caching
+ // when we transition then store to an existing property. This is common among
+ // paths that reify lazy properties. If we reify a lazy property and forget
+ // to disable caching, we may come down this path. The Replace IC does not
+ // know how to model these types of structure transitions (or any structure
+ // transition for that matter).
+ RELEASE_ASSERT(baseValue.asCell()->structure(vm) == structure);
+
structure->didCachePropertyReplacement(vm, slot.cachedOffset());
if (stubInfo.cacheType == CacheType::Unset
Modified: trunk/Source/_javascript_Core/runtime/ErrorInstance.cpp (225767 => 225768)
--- trunk/Source/_javascript_Core/runtime/ErrorInstance.cpp 2017-12-12 02:45:13 UTC (rev 225767)
+++ trunk/Source/_javascript_Core/runtime/ErrorInstance.cpp 2017-12-12 03:24:43 UTC (rev 225768)
@@ -202,10 +202,10 @@
return builder.toString();
}
-void ErrorInstance::materializeErrorInfoIfNeeded(VM& vm)
+bool ErrorInstance::materializeErrorInfoIfNeeded(VM& vm)
{
if (m_errorInfoMaterialized)
- return;
+ return false;
addErrorInfo(vm, m_stackTrace.get(), this);
{
@@ -214,15 +214,17 @@
}
m_errorInfoMaterialized = true;
+ return true;
}
-void ErrorInstance::materializeErrorInfoIfNeeded(VM& vm, PropertyName propertyName)
+bool ErrorInstance::materializeErrorInfoIfNeeded(VM& vm, PropertyName propertyName)
{
if (propertyName == vm.propertyNames->line
|| propertyName == vm.propertyNames->column
|| propertyName == vm.propertyNames->sourceURL
|| propertyName == vm.propertyNames->stack)
- materializeErrorInfoIfNeeded(vm);
+ return materializeErrorInfoIfNeeded(vm);
+ return false;
}
void ErrorInstance::visitChildren(JSCell* cell, SlotVisitor& visitor)
@@ -276,7 +278,9 @@
{
VM& vm = exec->vm();
ErrorInstance* thisObject = jsCast<ErrorInstance*>(cell);
- thisObject->materializeErrorInfoIfNeeded(vm, propertyName);
+ bool materializedProperties = thisObject->materializeErrorInfoIfNeeded(vm, propertyName);
+ if (materializedProperties)
+ slot.disableCaching();
return Base::put(thisObject, exec, propertyName, value, slot);
}
Modified: trunk/Source/_javascript_Core/runtime/ErrorInstance.h (225767 => 225768)
--- trunk/Source/_javascript_Core/runtime/ErrorInstance.h 2017-12-12 02:45:13 UTC (rev 225767)
+++ trunk/Source/_javascript_Core/runtime/ErrorInstance.h 2017-12-12 03:24:43 UTC (rev 225768)
@@ -69,8 +69,8 @@
Vector<StackFrame>* stackTrace() { return m_stackTrace.get(); }
- void materializeErrorInfoIfNeeded(VM&);
- void materializeErrorInfoIfNeeded(VM&, PropertyName);
+ bool materializeErrorInfoIfNeeded(VM&);
+ bool materializeErrorInfoIfNeeded(VM&, PropertyName);
protected:
explicit ErrorInstance(VM&, Structure*);
Modified: trunk/Source/_javascript_Core/runtime/Structure.cpp (225767 => 225768)
--- trunk/Source/_javascript_Core/runtime/Structure.cpp 2017-12-12 02:45:13 UTC (rev 225767)
+++ trunk/Source/_javascript_Core/runtime/Structure.cpp 2017-12-12 03:24:43 UTC (rev 225768)
@@ -873,6 +873,7 @@
void Structure::didCachePropertyReplacement(VM& vm, PropertyOffset offset)
{
+ RELEASE_ASSERT(isValidOffset(offset));
ensurePropertyReplacementWatchpointSet(vm, offset)->fireAll(vm, "Did cache property replacement");
}
