- Revision
- 245018
- Author
- tzaga...@apple.com
- Date
- 2019-05-07 11:39:27 -0700 (Tue, 07 May 2019)
Log Message
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: trunk/JSTests/ChangeLog (245017 => 245018)
--- trunk/JSTests/ChangeLog 2019-05-07 17:41:42 UTC (rev 245017)
+++ trunk/JSTests/ChangeLog 2019-05-07 18:39:27 UTC (rev 245018)
@@ -1,3 +1,44 @@
+2019-05-07 Tadeu Zagallo <tzaga...@apple.com>
+
+ 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-07 Saam Barati <sbar...@apple.com>
Don't OSR enter into an FTL CodeBlock that has been jettisoned
Added: trunk/JSTests/stress/cache-put-by-id-delete-prototype.js (0 => 245018)
--- trunk/JSTests/stress/cache-put-by-id-delete-prototype.js (rev 0)
+++ trunk/JSTests/stress/cache-put-by-id-delete-prototype.js 2019-05-07 18:39:27 UTC (rev 245018)
@@ -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: trunk/JSTests/stress/cache-put-by-id-different-__proto__.js (0 => 245018)
--- trunk/JSTests/stress/cache-put-by-id-different-__proto__.js (rev 0)
+++ trunk/JSTests/stress/cache-put-by-id-different-__proto__.js 2019-05-07 18:39:27 UTC (rev 245018)
@@ -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: trunk/JSTests/stress/cache-put-by-id-different-attributes.js (0 => 245018)
--- trunk/JSTests/stress/cache-put-by-id-different-attributes.js (rev 0)
+++ trunk/JSTests/stress/cache-put-by-id-different-attributes.js 2019-05-07 18:39:27 UTC (rev 245018)
@@ -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: trunk/JSTests/stress/cache-put-by-id-different-offset.js (0 => 245018)
--- trunk/JSTests/stress/cache-put-by-id-different-offset.js (rev 0)
+++ trunk/JSTests/stress/cache-put-by-id-different-offset.js 2019-05-07 18:39:27 UTC (rev 245018)
@@ -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: trunk/JSTests/stress/cache-put-by-id-insert-prototype.js (0 => 245018)
--- trunk/JSTests/stress/cache-put-by-id-insert-prototype.js (rev 0)
+++ trunk/JSTests/stress/cache-put-by-id-insert-prototype.js 2019-05-07 18:39:27 UTC (rev 245018)
@@ -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: trunk/JSTests/stress/cache-put-by-id-poly-proto.js (0 => 245018)
--- trunk/JSTests/stress/cache-put-by-id-poly-proto.js (rev 0)
+++ trunk/JSTests/stress/cache-put-by-id-poly-proto.js 2019-05-07 18:39:27 UTC (rev 245018)
@@ -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: trunk/Source/_javascript_Core/ChangeLog (245017 => 245018)
--- trunk/Source/_javascript_Core/ChangeLog 2019-05-07 17:41:42 UTC (rev 245017)
+++ trunk/Source/_javascript_Core/ChangeLog 2019-05-07 18:39:27 UTC (rev 245018)
@@ -1,3 +1,26 @@
+2019-05-07 Tadeu Zagallo <tzaga...@apple.com>
+
+ 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-07 Saam Barati <sbar...@apple.com>
Don't OSR enter into an FTL CodeBlock that has been jettisoned
Modified: trunk/Source/_javascript_Core/jit/Repatch.cpp (245017 => 245018)
--- trunk/Source/_javascript_Core/jit/Repatch.cpp 2019-05-07 17:41:42 UTC (rev 245017)
+++ trunk/Source/_javascript_Core/jit/Repatch.cpp 2019-05-07 18:39:27 UTC (rev 245018)
@@ -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();
}
}
