Changeset 225768 in webkit

Timestamp:

Dec 11, 2017 7:24:43 PM (6 years ago)

Author:

sbarati@apple.com

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

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/disable-caching-when-lazy-materializing-error-property-on-put.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/jit/Repatch.cpp (modified) (1 diff)
• Source/JavaScriptCore/runtime/ErrorInstance.cpp (modified) (4 diffs)
• Source/JavaScriptCore/runtime/ErrorInstance.h (modified) (1 diff)
• Source/JavaScriptCore/runtime/Structure.cpp (modified) (1 diff)

trunk/JSTests/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/JavaScriptCore/jit/Repatch.cpp

@@ -423 +423 @@
         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());
             

trunk/Source/JavaScriptCore/runtime/ErrorInstance.cpp

@@ -203 +203 @@
 }
 
-void ErrorInstance::materializeErrorInfoIfNeeded(VM& vm)
+bool ErrorInstance::materializeErrorInfoIfNeeded(VM& vm)
 {
     if (m_errorInfoMaterialized)
-        return;
+        return false;
     
     addErrorInfo(vm, m_stackTrace.get(), this);
@@ -215 +215 @@
     
     m_errorInfoMaterialized = true;
-}
-
-void ErrorInstance::materializeErrorInfoIfNeeded(VM& vm, PropertyName propertyName)
+    return true;
+}
+
+bool ErrorInstance::materializeErrorInfoIfNeeded(VM& vm, PropertyName propertyName)
 {
     if (propertyName == vm.propertyNames->line
@@ -223 +224 @@
         || propertyName == vm.propertyNames->sourceURL
         || propertyName == vm.propertyNames->stack)
-        materializeErrorInfoIfNeeded(vm);
+        return materializeErrorInfoIfNeeded(vm);
+    return false;
 }
 
@@ -277 +279 @@
     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);
 }

trunk/Source/JavaScriptCore/runtime/ErrorInstance.h

@@ -70 +70 @@
     Vector<StackFrame>* stackTrace() { return m_stackTrace.get(); }
 
-    void materializeErrorInfoIfNeeded(VM&);
-    void materializeErrorInfoIfNeeded(VM&, PropertyName);
+    bool materializeErrorInfoIfNeeded(VM&);
+    bool materializeErrorInfoIfNeeded(VM&, PropertyName);
 
 protected:

trunk/Source/JavaScriptCore/runtime/Structure.cpp

@@ -874 +874 @@
 void Structure::didCachePropertyReplacement(VM& vm, PropertyOffset offset)
 {
+    RELEASE_ASSERT(isValidOffset(offset));
     ensurePropertyReplacementWatchpointSet(vm, offset)->fireAll(vm, "Did cache property replacement");
 }