- Revision
- 248219
- Author
- [email protected]
- Date
- 2019-08-03 20:22:38 -0700 (Sat, 03 Aug 2019)
Log Message
Merge r245018 - 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):
Modified Paths
Added Paths
Diff
Modified: releases/WebKitGTK/webkit-2.24/JSTests/ChangeLog (248218 => 248219)
--- releases/WebKitGTK/webkit-2.24/JSTests/ChangeLog 2019-08-04 03:22:34 UTC (rev 248218)
+++ releases/WebKitGTK/webkit-2.24/JSTests/ChangeLog 2019-08-04 03:22:38 UTC (rev 248219)
@@ -1,3 +1,44 @@
+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-06 Yusuke Suzuki <[email protected]>
[JSC] We should check OOM for description string of Symbol
Added: releases/WebKitGTK/webkit-2.24/JSTests/stress/cache-put-by-id-delete-prototype.js (0 => 248219)
--- releases/WebKitGTK/webkit-2.24/JSTests/stress/cache-put-by-id-delete-prototype.js (rev 0)
+++ releases/WebKitGTK/webkit-2.24/JSTests/stress/cache-put-by-id-delete-prototype.js 2019-08-04 03:22:38 UTC (rev 248219)
@@ -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: releases/WebKitGTK/webkit-2.24/JSTests/stress/cache-put-by-id-different-__proto__.js (0 => 248219)
--- releases/WebKitGTK/webkit-2.24/JSTests/stress/cache-put-by-id-different-__proto__.js (rev 0)
+++ releases/WebKitGTK/webkit-2.24/JSTests/stress/cache-put-by-id-different-__proto__.js 2019-08-04 03:22:38 UTC (rev 248219)
@@ -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: releases/WebKitGTK/webkit-2.24/JSTests/stress/cache-put-by-id-different-attributes.js (0 => 248219)
--- releases/WebKitGTK/webkit-2.24/JSTests/stress/cache-put-by-id-different-attributes.js (rev 0)
+++ releases/WebKitGTK/webkit-2.24/JSTests/stress/cache-put-by-id-different-attributes.js 2019-08-04 03:22:38 UTC (rev 248219)
@@ -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: releases/WebKitGTK/webkit-2.24/JSTests/stress/cache-put-by-id-different-offset.js (0 => 248219)
--- releases/WebKitGTK/webkit-2.24/JSTests/stress/cache-put-by-id-different-offset.js (rev 0)
+++ releases/WebKitGTK/webkit-2.24/JSTests/stress/cache-put-by-id-different-offset.js 2019-08-04 03:22:38 UTC (rev 248219)
@@ -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: releases/WebKitGTK/webkit-2.24/JSTests/stress/cache-put-by-id-insert-prototype.js (0 => 248219)
--- releases/WebKitGTK/webkit-2.24/JSTests/stress/cache-put-by-id-insert-prototype.js (rev 0)
+++ releases/WebKitGTK/webkit-2.24/JSTests/stress/cache-put-by-id-insert-prototype.js 2019-08-04 03:22:38 UTC (rev 248219)
@@ -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: releases/WebKitGTK/webkit-2.24/JSTests/stress/cache-put-by-id-poly-proto.js (0 => 248219)
--- releases/WebKitGTK/webkit-2.24/JSTests/stress/cache-put-by-id-poly-proto.js (rev 0)
+++ releases/WebKitGTK/webkit-2.24/JSTests/stress/cache-put-by-id-poly-proto.js 2019-08-04 03:22:38 UTC (rev 248219)
@@ -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: releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/ChangeLog (248218 => 248219)
--- releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/ChangeLog 2019-08-04 03:22:34 UTC (rev 248218)
+++ releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/ChangeLog 2019-08-04 03:22:38 UTC (rev 248219)
@@ -1,3 +1,26 @@
+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-06 Yusuke Suzuki <[email protected]>
[JSC] We should check OOM for description string of Symbol
Modified: releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/jit/Repatch.cpp (248218 => 248219)
--- releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/jit/Repatch.cpp 2019-08-04 03:22:34 UTC (rev 248218)
+++ releases/WebKitGTK/webkit-2.24/Source/_javascript_Core/jit/Repatch.cpp 2019-08-04 03:22:38 UTC (rev 248219)
@@ -576,9 +576,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();
}
}
