DFG doesn't properly handle a property that is change to read only in a prototype

https://bugs.webkit.org/show_bug.cgi?id=173321

Reviewed by Filip Pizlo.

JSTests:

* ChakraCore.yaml: Renabled fieldopts/objtypespec-newobj-invalidation.1.js.
* stress/regress-173321.js: Added new regression test.
(shouldBe):
(SimpleObject):
(test):

Source/JavaScriptCore:

We need to check for ReadOnly as well as a not being a Setter when checking
an AbsenceOfSetter.

* bytecode/PropertyCondition.cpp:
(JSC::PropertyCondition::isStillValidAssumingImpurePropertyWatchpoint):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@218203 268f45cc-cd09-0410-ab3c-d52691b4dbfc

diff --git a/JSTests/ChakraCore.yaml b/JSTests/ChakraCore.yaml
index 0ab84ab..bee974a 100644 (file)
--- a/JSTests/ChakraCore.yaml
+++ b/JSTests/ChakraCore.yaml
@@ -2044,8 +2044,7 @@
 - path: ChakraCore/test/fieldopts/objtypespec-newobj.2.js
   cmd: runChakra :baseline, "NoException", "objtypespec-newobj.2.baseline", []
 - path: ChakraCore/test/fieldopts/objtypespec-newobj-invalidation.1.js
-  # FIXME: Re-enable once flakiness is resolved (https://bugs.webkit.org/show_bug.cgi?id=162567)
-  cmd: runChakra :skip, "NoException", "objtypespec-newobj-invalidation.1.baseline", []
+  cmd: runChakra :baseline, "NoException", "objtypespec-newobj-invalidation.1.baseline", []
 - path: ChakraCore/test/fieldopts/objtypespec-newobj-invalidation.2.js
   # Different behavior when run on 32 bit JSC.
   cmd: runChakra :skip, "NoException", "objtypespec-newobj-invalidation.2.baseline", []

diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
index d580c8d..32f88ea 100644 (file)
--- a/JSTests/ChangeLog
+++ b/JSTests/ChangeLog
@@ -1,3 +1,16 @@
+2017-06-13  Michael Saboff  <msaboff@apple.com>
+
+        DFG doesn't properly handle a property that is change to read only in a prototype
+        https://bugs.webkit.org/show_bug.cgi?id=173321
+
+        Reviewed by Filip Pizlo.
+
+        * ChakraCore.yaml: Renabled fieldopts/objtypespec-newobj-invalidation.1.js.
+        * stress/regress-173321.js: Added new regression test.
+        (shouldBe):
+        (SimpleObject):
+        (test):
+
 2017-06-12  Saam Barati  <sbarati@apple.com>
 
         Update test262 test expectation since r218082 makes new tests pass.

diff --git a/JSTests/stress/regress-173321.js b/JSTests/stress/regress-173321.js
new file mode 100644 (file)
index 0000000..75ca1ae
--- /dev/null
+++ b/JSTests/stress/regress-173321.js
@@ -0,0 +1,56 @@
+var checks = 0;
+
+function shouldBe(o, testObj)
+{
+    checks = checks + 1;
+
+    if (o.a != testObj.a)
+        throw "Check #" + checks + " o.a should be " + testObj.a + " , is " + o.a;
+
+    if (o.b != testObj.b)
+        throw "Check #" + checks + " o.b should be " + testObj.b + " , is " + o.b;
+
+    if (o.c != testObj.c)
+        throw "Check #" + checks + " o.c should be " + testObj.c + " , is " + o.c;
+
+    if (o.p != testObj.p)
+        throw "Check #" + checks + " o.p should be " + testObj.p + " , is " + o.p;
+
+    if (o.x != testObj.x)
+        throw "Check #" + checks + " o.x should be " + testObj.x + " , is " + o.x;
+
+    if (o.y != testObj.y)
+        throw "Check #" + checks + " o.y should be " + testObj.y + " , is " + o.y;
+}
+
+var testObjInitial = { a: 0, b: 1, c: 2, p: 100, x: 10, y: 11 };
+var testObjAfterReadOnlyProperty = { a: 101, b: 1, c: 2, p: 100, x: 10, y: 11 };
+
+var SimpleObject = function () {
+    this.a = 0;
+    this.b = 1;
+    this.c = 2;
+}
+
+var proto = { p: 100 };
+
+SimpleObject.prototype = proto;
+
+var test = function () {
+    var o = new SimpleObject();
+    o.x = 10;
+    o.y = 11;
+    return o;
+}
+
+shouldBe(test(), testObjInitial);
+shouldBe(test(), testObjInitial);
+shouldBe(test(), testObjInitial);
+
+// Change the prototype chain by making "a" read-only.
+Object.defineProperty(proto, "a", { value: 101, writable: false });
+
+// Run a bunch of times to tier up.
+for (var i = 0; i < 10000; i++)
+    shouldBe(test(), testObjAfterReadOnlyProperty);
+

diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
index fecede4..babd0ef 100644 (file)
--- a/Source/JavaScriptCore/ChangeLog
+++ b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,16 @@
+2017-06-13  Michael Saboff  <msaboff@apple.com>
+
+        DFG doesn't properly handle a property that is change to read only in a prototype
+        https://bugs.webkit.org/show_bug.cgi?id=173321
+
+        Reviewed by Filip Pizlo.
+
+        We need to check for ReadOnly as well as a not being a Setter when checking
+        an AbsenceOfSetter.
+
+        * bytecode/PropertyCondition.cpp:
+        (JSC::PropertyCondition::isStillValidAssumingImpurePropertyWatchpoint):
+
 2017-06-13  Daniel Bates  <dabates@apple.com>
 
         Implement W3C Secure Contexts Draft Specification

diff --git a/Source/JavaScriptCore/bytecode/PropertyCondition.cpp b/Source/JavaScriptCore/bytecode/PropertyCondition.cpp
index a8388df..dd8f532 100644 (file)
--- a/Source/JavaScriptCore/bytecode/PropertyCondition.cpp
+++ b/Source/JavaScriptCore/bytecode/PropertyCondition.cpp
@@ -134,7 +134,10 @@ bool PropertyCondition::isStillValidAssumingImpurePropertyWatchpoint(
         unsigned currentAttributes;
         PropertyOffset currentOffset = structure->getConcurrently(uid(), currentAttributes);
         if (currentOffset != invalidOffset) {
-            if (currentAttributes & (Accessor | CustomAccessor)) {
+            // FIXME: Given the addition of the check for ReadOnly attributes, we should refactor
+            // instances of AbsenceOfSetter.
+            // https://bugs.webkit.org/show_bug.cgi?id=173322 - Refactor AbsenceOfSetter to something like AbsenceOfSetEffects
+            if (currentAttributes & (ReadOnly | Accessor | CustomAccessor)) {
                 if (verbose) {
                     dataLog(
                         "Invalid because we expected not to have a setter, but we have one at offset ",