- Revision
- 245918
- Author
- [email protected]
- Date
- 2019-05-30 17:29:53 -0700 (Thu, 30 May 2019)
Log Message
Cherry-pick r245018. rdar://problem/51264878
tryCachePutByID should not crash if target offset changes
https://bugs.webkit.org/show_bug.cgi?id=197311
<rdar://problem/48033612>
Reviewed by Filip Pizlo.
JSTests:
Add a series of tests related tryCachePutByID. Two of these tests used to crash and were fixed
by this patch: `cache-put-by-id-different-attributes.js` and `cache-put-by-id-different-offset.js`
* stress/cache-put-by-id-delete-prototype.js: Added.
(A.prototype.set y):
(A):
(B.prototype.set y):
(B):
(C):
* stress/cache-put-by-id-different-__proto__.js: Added.
(A.prototype.set y):
(A):
(B1):
(B2.prototype.set y):
(B2):
(C):
(D):
* stress/cache-put-by-id-different-attributes.js: Added.
(Foo):
(set x):
* stress/cache-put-by-id-different-offset.js: Added.
(Foo):
(set x):
* stress/cache-put-by-id-insert-prototype.js: Added.
(A.prototype.set y):
(A):
(C):
* stress/cache-put-by-id-poly-proto.js: Added.
(Foo):
(set _):
(createBar.Bar):
(createBar):
Source/_javascript_Core:
When tryCachePutID is called with a cacheable setter, if the target object where the setter was
found is still in the prototype chain and there's no poly protos in the chain, we use
generateConditionsForPrototypePropertyHit to validate that the target object remains the same.
It checks for the absence of the property in every object in the prototype chain from the base
down to the target object and checks that the property is still present in the target object. It
also bails if there are any uncacheable objects, proxies or dictionary objects in the prototype
chain. However, it does not consider two edge cases:
- It asserts that the property should still be at the same offset in the target object, but this
assertion does not hold if the setter deletes properties of the object and causes the structure
to be flattened after the deletion. Instead of asserting, we just use the updated offset.
- It does not check whether the new slot is also a setter, which leads to a crash in case it's not.
* jit/Repatch.cpp:
(JSC::tryCachePutByID):
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@245018 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Modified Paths
Added Paths
Diff
Modified: branches/safari-607-branch/JSTests/ChangeLog (245917 => 245918)
--- branches/safari-607-branch/JSTests/ChangeLog 2019-05-31 00:29:49 UTC (rev 245917)
+++ branches/safari-607-branch/JSTests/ChangeLog 2019-05-31 00:29:53 UTC (rev 245918)
@@ -1,5 +1,111 @@
2019-05-30 Kocsen Chung <[email protected]>
+ Cherry-pick r245018. rdar://problem/51264878
+
+ tryCachePutByID should not crash if target offset changes
+ https://bugs.webkit.org/show_bug.cgi?id=197311
+ <rdar://problem/48033612>
+
+ Reviewed by Filip Pizlo.
+
+ JSTests:
+
+ Add a series of tests related tryCachePutByID. Two of these tests used to crash and were fixed
+ by this patch: `cache-put-by-id-different-attributes.js` and `cache-put-by-id-different-offset.js`
+
+ * stress/cache-put-by-id-delete-prototype.js: Added.
+ (A.prototype.set y):
+ (A):
+ (B.prototype.set y):
+ (B):
+ (C):
+ * stress/cache-put-by-id-different-__proto__.js: Added.
+ (A.prototype.set y):
+ (A):
+ (B1):
+ (B2.prototype.set y):
+ (B2):
+ (C):
+ (D):
+ * stress/cache-put-by-id-different-attributes.js: Added.
+ (Foo):
+ (set x):
+ * stress/cache-put-by-id-different-offset.js: Added.
+ (Foo):
+ (set x):
+ * stress/cache-put-by-id-insert-prototype.js: Added.
+ (A.prototype.set y):
+ (A):
+ (C):
+ * stress/cache-put-by-id-poly-proto.js: Added.
+ (Foo):
+ (set _):
+ (createBar.Bar):
+ (createBar):
+
+ Source/_javascript_Core:
+
+ When tryCachePutID is called with a cacheable setter, if the target object where the setter was
+ found is still in the prototype chain and there's no poly protos in the chain, we use
+ generateConditionsForPrototypePropertyHit to validate that the target object remains the same.
+ It checks for the absence of the property in every object in the prototype chain from the base
+ down to the target object and checks that the property is still present in the target object. It
+ also bails if there are any uncacheable objects, proxies or dictionary objects in the prototype
+ chain. However, it does not consider two edge cases:
+ - It asserts that the property should still be at the same offset in the target object, but this
+ assertion does not hold if the setter deletes properties of the object and causes the structure
+ to be flattened after the deletion. Instead of asserting, we just use the updated offset.
+ - It does not check whether the new slot is also a setter, which leads to a crash in case it's not.
+
+ * jit/Repatch.cpp:
+ (JSC::tryCachePutByID):
+
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@245018 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2019-05-07 Tadeu Zagallo <[email protected]>
+
+ tryCachePutByID should not crash if target offset changes
+ https://bugs.webkit.org/show_bug.cgi?id=197311
+ <rdar://problem/48033612>
+
+ Reviewed by Filip Pizlo.
+
+ Add a series of tests related tryCachePutByID. Two of these tests used to crash and were fixed
+ by this patch: `cache-put-by-id-different-attributes.js` and `cache-put-by-id-different-offset.js`
+
+ * stress/cache-put-by-id-delete-prototype.js: Added.
+ (A.prototype.set y):
+ (A):
+ (B.prototype.set y):
+ (B):
+ (C):
+ * stress/cache-put-by-id-different-__proto__.js: Added.
+ (A.prototype.set y):
+ (A):
+ (B1):
+ (B2.prototype.set y):
+ (B2):
+ (C):
+ (D):
+ * stress/cache-put-by-id-different-attributes.js: Added.
+ (Foo):
+ (set x):
+ * stress/cache-put-by-id-different-offset.js: Added.
+ (Foo):
+ (set x):
+ * stress/cache-put-by-id-insert-prototype.js: Added.
+ (A.prototype.set y):
+ (A):
+ (C):
+ * stress/cache-put-by-id-poly-proto.js: Added.
+ (Foo):
+ (set _):
+ (createBar.Bar):
+ (createBar):
+
+2019-05-30 Kocsen Chung <[email protected]>
+
Cherry-pick r244950. rdar://problem/51265731
TypedArrays should not store properties that are canonical numeric indices
Added: branches/safari-607-branch/JSTests/stress/cache-put-by-id-delete-prototype.js (0 => 245918)
--- branches/safari-607-branch/JSTests/stress/cache-put-by-id-delete-prototype.js (rev 0)
+++ branches/safari-607-branch/JSTests/stress/cache-put-by-id-delete-prototype.js 2019-05-31 00:29:53 UTC (rev 245918)
@@ -0,0 +1,26 @@
+//@ runNoLLInt
+
+let calledA = false;
+let counter = 0;
+
+class A {
+ set y(_) {
+ calledA = true;
+ }
+}
+
+class B extends A {
+ set y(_) {
+ if (counter++ === 9)
+ delete B.prototype.y;
+ }
+}
+class C extends B { }
+
+let c = new C();
+for (let i = 0; i < 11; i++) {
+ c.y = 42;
+}
+
+if (!calledA)
+ throw new Error('The setter for A.y should have been called');
Added: branches/safari-607-branch/JSTests/stress/cache-put-by-id-different-__proto__.js (0 => 245918)
--- branches/safari-607-branch/JSTests/stress/cache-put-by-id-different-__proto__.js (rev 0)
+++ branches/safari-607-branch/JSTests/stress/cache-put-by-id-different-__proto__.js 2019-05-31 00:29:53 UTC (rev 245918)
@@ -0,0 +1,33 @@
+//@ runNoLLInt
+
+let counter = 0;
+class A {
+ static set y(x) {
+ if (counter++ === 9) {
+ C.__proto__ = B2;
+ }
+ }
+}
+
+class B1 extends A {
+}
+
+let calledB2 = false;
+class B2 extends A {
+ static set y(x) {
+ calledB2 = true;
+ }
+}
+
+class C extends B1 {
+}
+
+class D extends C {
+}
+
+for (let i = 0; i < 11; i++) {
+ D.y = 42;
+}
+
+if (!calledB2)
+ throw new Error('The setter for B2.y should have been called');
Added: branches/safari-607-branch/JSTests/stress/cache-put-by-id-different-attributes.js (0 => 245918)
--- branches/safari-607-branch/JSTests/stress/cache-put-by-id-different-attributes.js (rev 0)
+++ branches/safari-607-branch/JSTests/stress/cache-put-by-id-different-attributes.js 2019-05-31 00:29:53 UTC (rev 245918)
@@ -0,0 +1,23 @@
+//@ runNoLLInt
+
+function Foo() { }
+
+Foo.prototype.x = 0;
+
+Object.defineProperty(Foo.prototype, 'y', {
+ set(x) {
+ if (Foo.prototype.x++ === 9) {
+ Object.defineProperty(Foo.prototype, 'y', {
+ value: 13,
+ writable: true,
+ });
+ if (typeof $vm !== 'undefined')
+ $vm.flattenDictionaryObject(Foo.prototype);
+ }
+ },
+ configurable: true,
+});
+
+let foo = new Foo();
+for (let i = 0; i < 11; i++)
+ foo.y = 42;
Added: branches/safari-607-branch/JSTests/stress/cache-put-by-id-different-offset.js (0 => 245918)
--- branches/safari-607-branch/JSTests/stress/cache-put-by-id-different-offset.js (rev 0)
+++ branches/safari-607-branch/JSTests/stress/cache-put-by-id-different-offset.js 2019-05-31 00:29:53 UTC (rev 245918)
@@ -0,0 +1,19 @@
+//@ runNoLLInt
+
+function Foo() { }
+
+Foo.prototype.x = 0;
+
+Object.defineProperty(Foo.prototype, 'y', {
+ set(x) {
+ if (Foo.prototype.x++ === 1) {
+ delete Foo.prototype.x;
+ if (typeof $vm !== 'undefined')
+ $vm.flattenDictionaryObject(Foo.prototype);
+ }
+ }
+});
+
+let foo = new Foo();
+while (typeof foo.x === 'number')
+ foo.y = 42;
Added: branches/safari-607-branch/JSTests/stress/cache-put-by-id-insert-prototype.js (0 => 245918)
--- branches/safari-607-branch/JSTests/stress/cache-put-by-id-insert-prototype.js (rev 0)
+++ branches/safari-607-branch/JSTests/stress/cache-put-by-id-insert-prototype.js 2019-05-31 00:29:53 UTC (rev 245918)
@@ -0,0 +1,27 @@
+//@ runNoLLInt
+
+let calledB = false;
+let counter = 0;
+
+class A {
+ set y(_) {
+ if (counter++ === 9) {
+ Object.defineProperty(B.prototype, 'y', {
+ set(_) {
+ calledB = true;
+ }
+ });
+ }
+ }
+}
+
+class B extends A { }
+class C extends B { }
+
+let c = new C();
+for (let i = 0; i < 11; i++) {
+ c.y = 42;
+}
+
+if (!calledB)
+ throw new Error('The setter for B.y should have been called');
Added: branches/safari-607-branch/JSTests/stress/cache-put-by-id-poly-proto.js (0 => 245918)
--- branches/safari-607-branch/JSTests/stress/cache-put-by-id-poly-proto.js (rev 0)
+++ branches/safari-607-branch/JSTests/stress/cache-put-by-id-poly-proto.js 2019-05-31 00:29:53 UTC (rev 245918)
@@ -0,0 +1,34 @@
+//@ runNoLLInt
+
+function Foo() { }
+
+let x = 0;
+
+Object.defineProperty(Foo.prototype, 'y', {
+ set(_) {
+ if (x++ === 9) {
+ Object.defineProperty(Foo.prototype, 'y', {
+ value: 13,
+ writable: true,
+ });
+ if (typeof $vm !== 'undefined')
+ $vm.flattenDictionaryObject(Foo.prototype);
+ }
+ },
+ configurable: true,
+});
+
+function createBar() {
+ class Bar extends Foo {
+ constructor() {
+ super();
+ this._y = 0;
+ }
+ }
+ return new Bar();
+};
+
+for (let i = 0; i < 16; i++) {
+ let bar = createBar();
+ bar.y = 42;
+}
Modified: branches/safari-607-branch/Source/_javascript_Core/ChangeLog (245917 => 245918)
--- branches/safari-607-branch/Source/_javascript_Core/ChangeLog 2019-05-31 00:29:49 UTC (rev 245917)
+++ branches/safari-607-branch/Source/_javascript_Core/ChangeLog 2019-05-31 00:29:53 UTC (rev 245918)
@@ -1,5 +1,93 @@
2019-05-30 Kocsen Chung <[email protected]>
+ Cherry-pick r245018. rdar://problem/51264878
+
+ tryCachePutByID should not crash if target offset changes
+ https://bugs.webkit.org/show_bug.cgi?id=197311
+ <rdar://problem/48033612>
+
+ Reviewed by Filip Pizlo.
+
+ JSTests:
+
+ Add a series of tests related tryCachePutByID. Two of these tests used to crash and were fixed
+ by this patch: `cache-put-by-id-different-attributes.js` and `cache-put-by-id-different-offset.js`
+
+ * stress/cache-put-by-id-delete-prototype.js: Added.
+ (A.prototype.set y):
+ (A):
+ (B.prototype.set y):
+ (B):
+ (C):
+ * stress/cache-put-by-id-different-__proto__.js: Added.
+ (A.prototype.set y):
+ (A):
+ (B1):
+ (B2.prototype.set y):
+ (B2):
+ (C):
+ (D):
+ * stress/cache-put-by-id-different-attributes.js: Added.
+ (Foo):
+ (set x):
+ * stress/cache-put-by-id-different-offset.js: Added.
+ (Foo):
+ (set x):
+ * stress/cache-put-by-id-insert-prototype.js: Added.
+ (A.prototype.set y):
+ (A):
+ (C):
+ * stress/cache-put-by-id-poly-proto.js: Added.
+ (Foo):
+ (set _):
+ (createBar.Bar):
+ (createBar):
+
+ Source/_javascript_Core:
+
+ When tryCachePutID is called with a cacheable setter, if the target object where the setter was
+ found is still in the prototype chain and there's no poly protos in the chain, we use
+ generateConditionsForPrototypePropertyHit to validate that the target object remains the same.
+ It checks for the absence of the property in every object in the prototype chain from the base
+ down to the target object and checks that the property is still present in the target object. It
+ also bails if there are any uncacheable objects, proxies or dictionary objects in the prototype
+ chain. However, it does not consider two edge cases:
+ - It asserts that the property should still be at the same offset in the target object, but this
+ assertion does not hold if the setter deletes properties of the object and causes the structure
+ to be flattened after the deletion. Instead of asserting, we just use the updated offset.
+ - It does not check whether the new slot is also a setter, which leads to a crash in case it's not.
+
+ * jit/Repatch.cpp:
+ (JSC::tryCachePutByID):
+
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@245018 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2019-05-07 Tadeu Zagallo <[email protected]>
+
+ tryCachePutByID should not crash if target offset changes
+ https://bugs.webkit.org/show_bug.cgi?id=197311
+ <rdar://problem/48033612>
+
+ Reviewed by Filip Pizlo.
+
+ When tryCachePutID is called with a cacheable setter, if the target object where the setter was
+ found is still in the prototype chain and there's no poly protos in the chain, we use
+ generateConditionsForPrototypePropertyHit to validate that the target object remains the same.
+ It checks for the absence of the property in every object in the prototype chain from the base
+ down to the target object and checks that the property is still present in the target object. It
+ also bails if there are any uncacheable objects, proxies or dictionary objects in the prototype
+ chain. However, it does not consider two edge cases:
+ - It asserts that the property should still be at the same offset in the target object, but this
+ assertion does not hold if the setter deletes properties of the object and causes the structure
+ to be flattened after the deletion. Instead of asserting, we just use the updated offset.
+ - It does not check whether the new slot is also a setter, which leads to a crash in case it's not.
+
+ * jit/Repatch.cpp:
+ (JSC::tryCachePutByID):
+
+2019-05-30 Kocsen Chung <[email protected]>
+
Cherry-pick r244950. rdar://problem/51265731
TypedArrays should not store properties that are canonical numeric indices
Modified: branches/safari-607-branch/Source/_javascript_Core/jit/Repatch.cpp (245917 => 245918)
--- branches/safari-607-branch/Source/_javascript_Core/jit/Repatch.cpp 2019-05-31 00:29:49 UTC (rev 245917)
+++ branches/safari-607-branch/Source/_javascript_Core/jit/Repatch.cpp 2019-05-31 00:29:53 UTC (rev 245918)
@@ -575,9 +575,10 @@
if (!conditionSet.isValid())
return GiveUpOnCache;
- PropertyOffset conditionSetOffset = conditionSet.slotBaseCondition().offset();
- if (UNLIKELY(offset != conditionSetOffset))
- CRASH_WITH_INFO(offset, conditionSetOffset, slot.base()->type(), baseCell->type(), conditionSet.size());
+ if (!(conditionSet.slotBaseCondition().attributes() & PropertyAttribute::Accessor))
+ return GiveUpOnCache;
+
+ offset = conditionSet.slotBaseCondition().offset();
}
}
