Title: [225793] tags/Safari-605.1.17

Diff

Modified: tags/Safari-605.1.17/JSTests/ChangeLog (225792 => 225793)


--- tags/Safari-605.1.17/JSTests/ChangeLog	2017-12-12 19:19:11 UTC (rev 225792)
+++ tags/Safari-605.1.17/JSTests/ChangeLog	2017-12-12 19:20:41 UTC (rev 225793)
@@ -1,3 +1,21 @@
+2017-12-12  Jason Marcell  <jmarc...@apple.com>
+
+        Cherry-pick r225768. rdar://problem/34657871
+
+    2017-12-11  Saam Barati  <sbar...@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  <jfbast...@apple.com>
 
         WebAssembly: don't eagerly checksum

Added: tags/Safari-605.1.17/JSTests/stress/disable-caching-when-lazy-materializing-error-property-on-put.js (0 => 225793)


--- tags/Safari-605.1.17/JSTests/stress/disable-caching-when-lazy-materializing-error-property-on-put.js	                        (rev 0)
+++ tags/Safari-605.1.17/JSTests/stress/disable-caching-when-lazy-materializing-error-property-on-put.js	2017-12-12 19:20:41 UTC (rev 225793)
@@ -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: tags/Safari-605.1.17/Source/_javascript_Core/ChangeLog (225792 => 225793)


--- tags/Safari-605.1.17/Source/_javascript_Core/ChangeLog	2017-12-12 19:19:11 UTC (rev 225792)
+++ tags/Safari-605.1.17/Source/_javascript_Core/ChangeLog	2017-12-12 19:20:41 UTC (rev 225793)
@@ -1,3 +1,31 @@
+2017-12-12  Jason Marcell  <jmarc...@apple.com>
+
+        Cherry-pick r225768. rdar://problem/34657871
+
+    2017-12-11  Saam Barati  <sbar...@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-10  Filip Pizlo  <fpi...@apple.com>
 
         Harden a few assertions in GC sweep

Modified: tags/Safari-605.1.17/Source/_javascript_Core/jit/Repatch.cpp (225792 => 225793)


--- tags/Safari-605.1.17/Source/_javascript_Core/jit/Repatch.cpp	2017-12-12 19:19:11 UTC (rev 225792)
+++ tags/Safari-605.1.17/Source/_javascript_Core/jit/Repatch.cpp	2017-12-12 19:20:41 UTC (rev 225793)
@@ -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: tags/Safari-605.1.17/Source/_javascript_Core/runtime/ErrorInstance.cpp (225792 => 225793)


--- tags/Safari-605.1.17/Source/_javascript_Core/runtime/ErrorInstance.cpp	2017-12-12 19:19:11 UTC (rev 225792)
+++ tags/Safari-605.1.17/Source/_javascript_Core/runtime/ErrorInstance.cpp	2017-12-12 19:20:41 UTC (rev 225793)
@@ -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: tags/Safari-605.1.17/Source/_javascript_Core/runtime/ErrorInstance.h (225792 => 225793)


--- tags/Safari-605.1.17/Source/_javascript_Core/runtime/ErrorInstance.h	2017-12-12 19:19:11 UTC (rev 225792)
+++ tags/Safari-605.1.17/Source/_javascript_Core/runtime/ErrorInstance.h	2017-12-12 19:20:41 UTC (rev 225793)
@@ -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: tags/Safari-605.1.17/Source/_javascript_Core/runtime/Structure.cpp (225792 => 225793)


--- tags/Safari-605.1.17/Source/_javascript_Core/runtime/Structure.cpp	2017-12-12 19:19:11 UTC (rev 225792)
+++ tags/Safari-605.1.17/Source/_javascript_Core/runtime/Structure.cpp	2017-12-12 19:20:41 UTC (rev 225793)
@@ -873,6 +873,7 @@
 
 void Structure::didCachePropertyReplacement(VM& vm, PropertyOffset offset)
 {
+    RELEASE_ASSERT(isValidOffset(offset));
     ensurePropertyReplacementWatchpointSet(vm, offset)->fireAll(vm, "Did cache property replacement");
 }
 
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to