- Revision
- 259654
- Author
- alanc...@apple.com
- Date
- 2020-04-07 11:43:30 -0700 (Tue, 07 Apr 2020)
Log Message
Cherry-pick r257605. rdar://problem/61231926
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.2.1.2-branch/JSTests/ChangeLog (259653 => 259654)
--- branches/safari-609.2.1.2-branch/JSTests/ChangeLog 2020-04-07 18:43:25 UTC (rev 259653)
+++ branches/safari-609.2.1.2-branch/JSTests/ChangeLog 2020-04-07 18:43:30 UTC (rev 259654)
@@ -1,3 +1,64 @@
+2020-04-07 Alan Coon <alanc...@apple.com>
+
+ Cherry-pick r257605. rdar://problem/61231926
+
+ 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 <justin_mich...@apple.com>
+
+ 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-04-03 Alan Coon <alanc...@apple.com>
Cherry-pick r256766. rdar://problem/61231942
Added: branches/safari-609.2.1.2-branch/JSTests/stress/delete-property-poly-proto.js (0 => 259654)
--- branches/safari-609.2.1.2-branch/JSTests/stress/delete-property-poly-proto.js (rev 0)
+++ branches/safari-609.2.1.2-branch/JSTests/stress/delete-property-poly-proto.js 2020-04-07 18:43:30 UTC (rev 259654)
@@ -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.2.1.2-branch/JSTests/stress/poly-proto-setter-adds-setter-in-middle.js (0 => 259654)
--- branches/safari-609.2.1.2-branch/JSTests/stress/poly-proto-setter-adds-setter-in-middle.js (rev 0)
+++ branches/safari-609.2.1.2-branch/JSTests/stress/poly-proto-setter-adds-setter-in-middle.js 2020-04-07 18:43:30 UTC (rev 259654)
@@ -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.2.1.2-branch/JSTests/stress/poly-proto-setter-changes-setter-2.js (0 => 259654)
--- branches/safari-609.2.1.2-branch/JSTests/stress/poly-proto-setter-changes-setter-2.js (rev 0)
+++ branches/safari-609.2.1.2-branch/JSTests/stress/poly-proto-setter-changes-setter-2.js 2020-04-07 18:43:30 UTC (rev 259654)
@@ -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.2.1.2-branch/JSTests/stress/poly-proto-setter-changes-setter.js (0 => 259654)
--- branches/safari-609.2.1.2-branch/JSTests/stress/poly-proto-setter-changes-setter.js (rev 0)
+++ branches/safari-609.2.1.2-branch/JSTests/stress/poly-proto-setter-changes-setter.js 2020-04-07 18:43:30 UTC (rev 259654)
@@ -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.2.1.2-branch/Source/_javascript_Core/ChangeLog (259653 => 259654)
--- branches/safari-609.2.1.2-branch/Source/_javascript_Core/ChangeLog 2020-04-07 18:43:25 UTC (rev 259653)
+++ branches/safari-609.2.1.2-branch/Source/_javascript_Core/ChangeLog 2020-04-07 18:43:30 UTC (rev 259654)
@@ -1,5 +1,89 @@
2020-04-07 Alan Coon <alanc...@apple.com>
+ Cherry-pick r257605. rdar://problem/61231926
+
+ 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 <justin_mich...@apple.com>
+
+ 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-04-07 Alan Coon <alanc...@apple.com>
+
Cherry-pick r255542. rdar://problem/61231926
[JSC] Hold StructureID instead of Structure* in PolyProtoAccessChain and DFG::CommonData
Modified: branches/safari-609.2.1.2-branch/Source/_javascript_Core/jit/Repatch.cpp (259653 => 259654)
--- branches/safari-609.2.1.2-branch/Source/_javascript_Core/jit/Repatch.cpp 2020-04-07 18:43:25 UTC (rev 259653)
+++ branches/safari-609.2.1.2-branch/Source/_javascript_Core/jit/Repatch.cpp 2020-04-07 18:43:30 UTC (rev 259654)
@@ -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(
