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/JavaScriptCore:

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):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@225768 268f45cc-cd09-0410-ab3c-d52691b4dbfc

diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
index 1236efc..de8ae5d 100644 (file)
--- a/JSTests/ChangeLog
+++ b/JSTests/ChangeLog
@@ -1,3 +1,17 @@
+2017-12-11  Saam Barati  <sbarati@apple.com>
+
+        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  <jfbastien@apple.com>
 
         WebAssembly: don't eagerly checksum

diff --git a/JSTests/stress/disable-caching-when-lazy-materializing-error-property-on-put.js b/JSTests/stress/disable-caching-when-lazy-materializing-error-property-on-put.js
new file mode 100644 (file)
index 0000000..6b08e71
--- /dev/null
+++ b/JSTests/stress/disable-caching-when-lazy-materializing-error-property-on-put.js
@@ -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");
+}

diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
index c1d7f17..d276265 100644 (file)
--- a/Source/JavaScriptCore/ChangeLog
+++ b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,27 @@
+2017-12-11  Saam Barati  <sbarati@apple.com>
+
+        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  <Hironori.Fujii@sony.com>
 
         [WinCairo] DLLLauncherMain should use SetDllDirectory

diff --git a/Source/JavaScriptCore/jit/Repatch.cpp b/Source/JavaScriptCore/jit/Repatch.cpp
index 0c9047d..a7ca6af 100644 (file)
--- a/Source/JavaScriptCore/jit/Repatch.cpp
+++ b/Source/JavaScriptCore/jit/Repatch.cpp
@@ -422,6 +422,14 @@ static InlineCacheAction tryCachePutByID(ExecState* exec, JSValue baseValue, Str
 
         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

diff --git a/Source/JavaScriptCore/runtime/ErrorInstance.cpp b/Source/JavaScriptCore/runtime/ErrorInstance.cpp
index 4e711aa..40c6258 100644 (file)
--- a/Source/JavaScriptCore/runtime/ErrorInstance.cpp
+++ b/Source/JavaScriptCore/runtime/ErrorInstance.cpp
@@ -202,10 +202,10 @@ String ErrorInstance::sanitizedToString(ExecState* exec)
     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 @@ void ErrorInstance::materializeErrorInfoIfNeeded(VM& vm)
     }
     
     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 @@ bool ErrorInstance::put(JSCell* cell, ExecState* exec, PropertyName propertyName
 {
     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);
 }
 

diff --git a/Source/JavaScriptCore/runtime/ErrorInstance.h b/Source/JavaScriptCore/runtime/ErrorInstance.h
index d86fbc0..56b3ae2 100644 (file)
--- a/Source/JavaScriptCore/runtime/ErrorInstance.h
+++ b/Source/JavaScriptCore/runtime/ErrorInstance.h
@@ -69,8 +69,8 @@ public:
     
     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*);

diff --git a/Source/JavaScriptCore/runtime/Structure.cpp b/Source/JavaScriptCore/runtime/Structure.cpp
index b3ce029..490bc5f 100644 (file)
--- a/Source/JavaScriptCore/runtime/Structure.cpp
+++ b/Source/JavaScriptCore/runtime/Structure.cpp
@@ -873,6 +873,7 @@ void Structure::startWatchingPropertyForReplacements(VM& vm, PropertyName proper
 
 void Structure::didCachePropertyReplacement(VM& vm, PropertyOffset offset)
 {
+    RELEASE_ASSERT(isValidOffset(offset));
     ensurePropertyReplacementWatchpointSet(vm, offset)->fireAll(vm, "Did cache property replacement");
 }