Attachment 369117 Details for Bug 197311 – Patch

[patch] Patch

bug-197311-20190506120042.patch (text/plain), 9.22 KB, created by Tadeu Zagallo on 2019-05-06 03:00:50 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Tadeu Zagallo

Created: 2019-05-06 03:00:50 PDT

Size: 9.22 KB

patch

obsolete

>Subversion Revision: 244956
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 7d960395584de6aa830c033c9f0c21d4162c62f6..f883e5e21120e24480b5e5ac716cec79b300d060 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,26 @@
>+2019-05-06  Tadeu Zagallo  <tzagallo@apple.com>
>+
>+        tryCachePutByID should not crash if target offset changes
>+        https://bugs.webkit.org/show_bug.cgi?id=197311
>+        <rdar://problem/48033612>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        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-04  Tadeu Zagallo  <tzagallo@apple.com>
> 
>         TypedArrays should not store properties that are canonical numeric indices
>diff --git a/Source/JavaScriptCore/jit/Repatch.cpp b/Source/JavaScriptCore/jit/Repatch.cpp
>index cda7376f8268e143187efd009e14170644bb97f7..f07d325aaa17fc91ff8c8c23643e7329863ee836 100644
>--- a/Source/JavaScriptCore/jit/Repatch.cpp
>+++ b/Source/JavaScriptCore/jit/Repatch.cpp
>@@ -576,9 +576,10 @@ static InlineCacheAction tryCachePutByID(ExecState* exec, JSValue baseValue, Str
>                         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();
>                     }
> 
>                 }
>diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
>index c2316a6960e6628407f9cdb7dc3efee2b59cc899..2338369ed88b23c5e82c7c7ff73956d23f96458c 100644
>--- a/JSTests/ChangeLog
>+++ b/JSTests/ChangeLog
>@@ -1,3 +1,44 @@
>+2019-05-06  Tadeu Zagallo  <tzagallo@apple.com>
>+
>+        tryCachePutByID should not crash if target offset changes
>+        https://bugs.webkit.org/show_bug.cgi?id=197311
>+        <rdar://problem/48033612>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        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-04  Tadeu Zagallo  <tzagallo@apple.com>
> 
>         TypedArrays should not store properties that are canonical numeric indices
>diff --git a/JSTests/stress/cache-put-by-id-delete-prototype.js b/JSTests/stress/cache-put-by-id-delete-prototype.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..e758698dfe138915b431b1c2156a0c59cdcbe4d7
>--- /dev/null
>+++ b/JSTests/stress/cache-put-by-id-delete-prototype.js
>@@ -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');
>diff --git a/JSTests/stress/cache-put-by-id-different-__proto__.js b/JSTests/stress/cache-put-by-id-different-__proto__.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..c04b58e72c80da66b05238c87fb3863da25611e1
>--- /dev/null
>+++ b/JSTests/stress/cache-put-by-id-different-__proto__.js
>@@ -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');
>diff --git a/JSTests/stress/cache-put-by-id-different-attributes.js b/JSTests/stress/cache-put-by-id-different-attributes.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..b38fc78a1449d71fd13cf43973098507fbcd393e
>--- /dev/null
>+++ b/JSTests/stress/cache-put-by-id-different-attributes.js
>@@ -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;
>diff --git a/JSTests/stress/cache-put-by-id-different-offset.js b/JSTests/stress/cache-put-by-id-different-offset.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..7f29713e7c2853cac7cd4a43ef6757ec8ae39bfc
>--- /dev/null
>+++ b/JSTests/stress/cache-put-by-id-different-offset.js
>@@ -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;
>diff --git a/JSTests/stress/cache-put-by-id-insert-prototype.js b/JSTests/stress/cache-put-by-id-insert-prototype.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..f2fac6795165983e6fdccf309f6a5fe17d682549
>--- /dev/null
>+++ b/JSTests/stress/cache-put-by-id-insert-prototype.js
>@@ -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');
>diff --git a/JSTests/stress/cache-put-by-id-poly-proto.js b/JSTests/stress/cache-put-by-id-poly-proto.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..b79baf0921e2d342526299d8d116eee6f9403293
>--- /dev/null
>+++ b/JSTests/stress/cache-put-by-id-poly-proto.js
>@@ -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;
>+}

Actions: View | Formatted Diff | Diff

Attachments on bug 197311: 368318 | 368401 | 369117 | 369129