- Revision
- 258157
- Author
- [email protected]
- Date
- 2020-03-09 13:03:15 -0700 (Mon, 09 Mar 2020)
Log Message
Cherry-pick r257605. rdar://problem/59870340
Poly proto should work with property delete transitions
https://bugs.webkit.org/show_bug.cgi?id=208261
Reviewed by Saam Barati.
JSTests:
* stress/delete-property-poly-proto.js: Added.
(A.prototype.set x):
(A):
(B):
Source/_javascript_Core:
This patch fixes a bug where the combination of inline caching
and poly proto cause us to cache a setter call along a prototype chain that
is no longer the correct setter to call. This is exposed as a result of
https://bugs.webkit.org/show_bug.cgi?id=206430 since DefineOwnProperty used
to transition to uncacheable dictionary.
The case looks like this:
A - setter for x redefines x
|
B
|
C
We set (new C).x
Right now, we first call A's setter, then we try to figure out what the state of things
were before it was called in order to cache it. We just assume that A's setter still exists, and we cache it
without ever checking, In this patch, we ensure that the property exists and the attributes match in order to prevent crashing.
In the code, A = target, C = base.
Get is correct because it collects caching information before any calls.
The bug https://bugs.webkit.org/show_bug.cgi?id=208337 tracks the remaining semantic bugs around this code.
* jit/Repatch.cpp:
(JSC::tryCachePutByID):
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@257605 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Modified Paths
Added Paths
Diff
Modified: branches/safari-609-branch/JSTests/ChangeLog (258156 => 258157)
--- branches/safari-609-branch/JSTests/ChangeLog 2020-03-09 20:03:12 UTC (rev 258156)
+++ branches/safari-609-branch/JSTests/ChangeLog 2020-03-09 20:03:15 UTC (rev 258157)
@@ -1,3 +1,64 @@
+2020-03-09 Alan Coon <[email protected]>
+
+ Cherry-pick r257605. rdar://problem/59870340
+
+ Poly proto should work with property delete transitions
+ https://bugs.webkit.org/show_bug.cgi?id=208261
+
+ Reviewed by Saam Barati.
+
+ JSTests:
+
+ * stress/delete-property-poly-proto.js: Added.
+ (A.prototype.set x):
+ (A):
+ (B):
+
+ Source/_javascript_Core:
+
+ This patch fixes a bug where the combination of inline caching
+ and poly proto cause us to cache a setter call along a prototype chain that
+ is no longer the correct setter to call. This is exposed as a result of
+ https://bugs.webkit.org/show_bug.cgi?id=206430 since DefineOwnProperty used
+ to transition to uncacheable dictionary.
+
+ The case looks like this:
+ A - setter for x redefines x
+ |
+ B
+ |
+ C
+
+ We set (new C).x
+
+ Right now, we first call A's setter, then we try to figure out what the state of things
+ were before it was called in order to cache it. We just assume that A's setter still exists, and we cache it
+ without ever checking, In this patch, we ensure that the property exists and the attributes match in order to prevent crashing.
+
+ In the code, A = target, C = base.
+
+ Get is correct because it collects caching information before any calls.
+
+ The bug https://bugs.webkit.org/show_bug.cgi?id=208337 tracks the remaining semantic bugs around this code.
+
+ * jit/Repatch.cpp:
+ (JSC::tryCachePutByID):
+
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@257605 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2020-02-27 Justin Michaud <[email protected]>
+
+ Poly proto should work with property delete transitions
+ https://bugs.webkit.org/show_bug.cgi?id=208261
+
+ Reviewed by Saam Barati.
+
+ * stress/delete-property-poly-proto.js: Added.
+ (A.prototype.set x):
+ (A):
+ (B):
+
2020-02-21 Russell Epstein <[email protected]>
Apply patch. rdar://problem/59654268
Added: branches/safari-609-branch/JSTests/stress/delete-property-poly-proto.js (0 => 258157)
--- branches/safari-609-branch/JSTests/stress/delete-property-poly-proto.js (rev 0)
+++ branches/safari-609-branch/JSTests/stress/delete-property-poly-proto.js 2020-03-09 20:03:15 UTC (rev 258157)
@@ -0,0 +1,41 @@
+//@ requireOptions("--forcePolyProto=1", "--useLLInt=0")
+
+class A {
+ set x(v) {
+ if (v === 1) {
+ delete A.prototype.x;
+ }
+ }
+
+ get y() {
+ if (this._y === 1) {
+ delete A.prototype.y;
+ }
+ this._y++;
+ }
+
+ set z(v) {
+ if (v === 1) {
+ delete A.prototype.z;
+ }
+ }
+}
+
+class B extends A {}
+
+let a = new B();
+for (let i = 0; i < 10; i++) {
+ a.x = i;
+}
+
+for (let i = 0; i < 10; i++) {
+ a["z"] = i;
+}
+
+a._y = 0;
+for (let i = 0; i < 15; i++) {
+ a.y;
+}
+
+if (a._y != 2)
+ throw new Error()
Added: branches/safari-609-branch/JSTests/stress/poly-proto-setter-adds-setter-in-middle.js (0 => 258157)
--- branches/safari-609-branch/JSTests/stress/poly-proto-setter-adds-setter-in-middle.js (rev 0)
+++ branches/safari-609-branch/JSTests/stress/poly-proto-setter-adds-setter-in-middle.js 2020-03-09 20:03:15 UTC (rev 258157)
@@ -0,0 +1,31 @@
+//@ requireOptions("--forcePolyProto=1", "--useLLInt=0", "--repatchBufferingCountdown=0")
+let correct = false
+
+class A {
+ set x(v) {
+ if (v == 1) {
+ Object.defineProperty(B.prototype, "x", {
+ set: function() {
+ correct = true
+ }
+ });
+ }
+ }
+}
+
+class B extends A {
+ set y(v) {}
+ set y1(v) {}
+ set y2(v) {}
+}
+
+class C extends B {
+
+}
+
+let a = new C();
+for (let i = 0; i < 30; i++) {
+ a.x = i;
+}
+
+// FIXME: Check that correct is true: https://bugs.webkit.org/show_bug.cgi?id=208337
Added: branches/safari-609-branch/JSTests/stress/poly-proto-setter-changes-setter-2.js (0 => 258157)
--- branches/safari-609-branch/JSTests/stress/poly-proto-setter-changes-setter-2.js (rev 0)
+++ branches/safari-609-branch/JSTests/stress/poly-proto-setter-changes-setter-2.js 2020-03-09 20:03:15 UTC (rev 258157)
@@ -0,0 +1,38 @@
+//@ requireOptions("--forcePolyProto=1", "--useLLInt=0", "--repatchBufferingCountdown=0")
+function assert_eq(a, b) {
+
+}
+noInline(assert_eq)
+
+class A {
+ set x(v) {
+ if (v == 1) {
+ Object.defineProperty(A.prototype, "x", {
+ get: function() {
+ return 42
+ },
+ set: undefined
+ });
+ }
+ if (v > 1)
+ throw new Error()
+ }
+}
+
+class B extends A {
+ set y(v) {}
+ set y1(v) {}
+ set y2(v) {}
+}
+
+class C extends B {
+
+}
+
+let a = new C();
+for (let i = 0; i < 30; i++) {
+ a.x = i;
+ assert_eq(a.x, i == 0 ? undefined : 42)
+}
+
+
Added: branches/safari-609-branch/JSTests/stress/poly-proto-setter-changes-setter.js (0 => 258157)
--- branches/safari-609-branch/JSTests/stress/poly-proto-setter-changes-setter.js (rev 0)
+++ branches/safari-609-branch/JSTests/stress/poly-proto-setter-changes-setter.js 2020-03-09 20:03:15 UTC (rev 258157)
@@ -0,0 +1,34 @@
+//@ requireOptions("--forcePolyProto=1", "--useLLInt=0", "--repatchBufferingCountdown=0")
+function assert_eq(a, b) {
+ if (a !== b)
+ throw new Error("assertion failed: " + a + " === " + b);
+}
+noInline(assert_eq)
+
+class A {
+ set x(v) {
+ if (v == 1) {
+ Object.defineProperty(A.prototype, "x", {
+ value: 42,
+ writable: true
+ });
+ }
+ }
+}
+
+class B extends A {
+ set y(v) {}
+ set y1(v) {}
+ set y2(v) {}
+}
+
+class C extends B {
+
+}
+
+let a = new C();
+for (let i = 0; i < 30; i++) {
+ a.x = i;
+ assert_eq(a.x, i == 0 ? undefined : i == 1 ? 42 : i);
+}
+
Modified: branches/safari-609-branch/Source/_javascript_Core/ChangeLog (258156 => 258157)
--- branches/safari-609-branch/Source/_javascript_Core/ChangeLog 2020-03-09 20:03:12 UTC (rev 258156)
+++ branches/safari-609-branch/Source/_javascript_Core/ChangeLog 2020-03-09 20:03:15 UTC (rev 258157)
@@ -1,5 +1,89 @@
2020-03-09 Alan Coon <[email protected]>
+ Cherry-pick r257605. rdar://problem/59870340
+
+ Poly proto should work with property delete transitions
+ https://bugs.webkit.org/show_bug.cgi?id=208261
+
+ Reviewed by Saam Barati.
+
+ JSTests:
+
+ * stress/delete-property-poly-proto.js: Added.
+ (A.prototype.set x):
+ (A):
+ (B):
+
+ Source/_javascript_Core:
+
+ This patch fixes a bug where the combination of inline caching
+ and poly proto cause us to cache a setter call along a prototype chain that
+ is no longer the correct setter to call. This is exposed as a result of
+ https://bugs.webkit.org/show_bug.cgi?id=206430 since DefineOwnProperty used
+ to transition to uncacheable dictionary.
+
+ The case looks like this:
+ A - setter for x redefines x
+ |
+ B
+ |
+ C
+
+ We set (new C).x
+
+ Right now, we first call A's setter, then we try to figure out what the state of things
+ were before it was called in order to cache it. We just assume that A's setter still exists, and we cache it
+ without ever checking, In this patch, we ensure that the property exists and the attributes match in order to prevent crashing.
+
+ In the code, A = target, C = base.
+
+ Get is correct because it collects caching information before any calls.
+
+ The bug https://bugs.webkit.org/show_bug.cgi?id=208337 tracks the remaining semantic bugs around this code.
+
+ * jit/Repatch.cpp:
+ (JSC::tryCachePutByID):
+
+
+ git-svn-id: https://svn.webkit.org/repository/webkit/trunk@257605 268f45cc-cd09-0410-ab3c-d52691b4dbfc
+
+ 2020-02-27 Justin Michaud <[email protected]>
+
+ Poly proto should work with property delete transitions
+ https://bugs.webkit.org/show_bug.cgi?id=208261
+
+ Reviewed by Saam Barati.
+
+ This patch fixes a bug where the combination of inline caching
+ and poly proto cause us to cache a setter call along a prototype chain that
+ is no longer the correct setter to call. This is exposed as a result of
+ https://bugs.webkit.org/show_bug.cgi?id=206430 since DefineOwnProperty used
+ to transition to uncacheable dictionary.
+
+ The case looks like this:
+ A - setter for x redefines x
+ |
+ B
+ |
+ C
+
+ We set (new C).x
+
+ Right now, we first call A's setter, then we try to figure out what the state of things
+ were before it was called in order to cache it. We just assume that A's setter still exists, and we cache it
+ without ever checking, In this patch, we ensure that the property exists and the attributes match in order to prevent crashing.
+
+ In the code, A = target, C = base.
+
+ Get is correct because it collects caching information before any calls.
+
+ The bug https://bugs.webkit.org/show_bug.cgi?id=208337 tracks the remaining semantic bugs around this code.
+
+ * jit/Repatch.cpp:
+ (JSC::tryCachePutByID):
+
+2020-03-09 Alan Coon <[email protected]>
+
Cherry-pick r255542. rdar://problem/59870340
[JSC] Hold StructureID instead of Structure* in PolyProtoAccessChain and DFG::CommonData
Modified: branches/safari-609-branch/Source/_javascript_Core/jit/Repatch.cpp (258156 => 258157)
--- branches/safari-609-branch/Source/_javascript_Core/jit/Repatch.cpp 2020-03-09 20:03:12 UTC (rev 258156)
+++ branches/safari-609-branch/Source/_javascript_Core/jit/Repatch.cpp 2020-03-09 20:03:15 UTC (rev 258157)
@@ -688,7 +688,10 @@
prototypeAccessChain = PolyProtoAccessChain::create(globalObject, baseCell, slot.base());
if (!prototypeAccessChain)
return GiveUpOnCache;
- offset = prototypeAccessChain->slotBaseStructure(vm, baseCell->structure(vm))->get(vm, ident.impl());
+ unsigned attributes;
+ offset = prototypeAccessChain->slotBaseStructure(vm, baseCell->structure(vm))->get(vm, ident.impl(), attributes);
+ if (!isValidOffset(offset) || !(attributes & PropertyAttribute::Accessor))
+ return RetryCacheLater;
} else {
prototypeAccessChain = nullptr;
conditionSet = generateConditionsForPrototypePropertyHit(
