Rolling out r243032 and r243071 because the fix is incorrect.
authormark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 24 Mar 2019 04:15:56 +0000 (04:15 +0000)
committermark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 24 Mar 2019 04:15:56 +0000 (04:15 +0000)
https://bugs.webkit.org/show_bug.cgi?id=195892
<rdar://problem/48981239>

Not reviewed.

JSTests:

* stress/check-object-property-condition-liveness-before-accessing-it-when-watchpoints-fire.js: Removed.

Source/JavaScriptCore:

The fix is incorrect: it relies on being able to determine liveness of an object
in an ObjectPropertyCondition based on the state of the object's MarkedBit.
However, there's no guarantee that GC has run and that the MarkedBit is already
set even if the object is live.  As a result, we may not re-install adaptive
watchpoints based on presumed dead objects which are actually live.

I'm rolling this out, and will implement a more comprehensive fix to handle
watchpoint liveness later.

* bytecode/AdaptiveInferredPropertyValueWatchpointBase.cpp:
(JSC::AdaptiveInferredPropertyValueWatchpointBase::fire):
* bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.cpp:
(JSC::LLIntPrototypeLoadAdaptiveStructureWatchpoint::fireInternal):
* bytecode/ObjectPropertyCondition.cpp:
(JSC::ObjectPropertyCondition::dumpInContext const):
* bytecode/StructureStubClearingWatchpoint.cpp:
(JSC::StructureStubClearingWatchpoint::fireInternal):
* dfg/DFGAdaptiveStructureWatchpoint.cpp:
(JSC::DFG::AdaptiveStructureWatchpoint::fireInternal):
* runtime/StructureRareData.cpp:
(JSC::ObjectToStringAdaptiveStructureWatchpoint::fireInternal):

LayoutTests:

* platform/mac/TestExpectations:

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

JSTests/ChangeLog
JSTests/stress/check-object-property-condition-liveness-before-accessing-it-when-watchpoints-fire.js [deleted file]
LayoutTests/ChangeLog
LayoutTests/platform/mac/TestExpectations
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/AdaptiveInferredPropertyValueWatchpointBase.cpp
Source/JavaScriptCore/bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.cpp
Source/JavaScriptCore/bytecode/ObjectPropertyCondition.cpp
Source/JavaScriptCore/bytecode/StructureStubClearingWatchpoint.cpp
Source/JavaScriptCore/dfg/DFGAdaptiveStructureWatchpoint.cpp
Source/JavaScriptCore/runtime/StructureRareData.cpp

index aeb8feb..9e23fd7 100644 (file)
@@ -1,3 +1,13 @@
+2019-03-23  Mark Lam  <mark.lam@apple.com>
+
+        Rolling out r243032 and r243071 because the fix is incorrect.
+        https://bugs.webkit.org/show_bug.cgi?id=195892
+        <rdar://problem/48981239>
+
+        Not reviewed.
+
+        * stress/check-object-property-condition-liveness-before-accessing-it-when-watchpoints-fire.js: Removed.
+
 2019-03-22  Mark Lam  <mark.lam@apple.com>
 
         Placate exception check validation in genericTypedArrayViewProtoFuncLastIndexOf().
diff --git a/JSTests/stress/check-object-property-condition-liveness-before-accessing-it-when-watchpoints-fire.js b/JSTests/stress/check-object-property-condition-liveness-before-accessing-it-when-watchpoints-fire.js
deleted file mode 100644 (file)
index 15af0c8..0000000
+++ /dev/null
@@ -1,30 +0,0 @@
-//@ requireOptions("--forceEagerCompilation=true")
-
-// This test should not crash.
-
-let a;
-
-function bar() {
-    a / 1;
-    a = null;
-}
-
-function foo(s) {
-    try {
-        eval(s);
-    } catch (e) {
-        gc();
-        bar();
-        '' + e + 0;
-    }
-}
-
-foo('zz');
-foo('class A { y() {} }; a=new A; zz');
-foo('class C { y() {} }; gc();');
-
-class A {
-    y() {}
-}
-
-A.prototype.z = 0
index 3bf5fdc..9892459 100644 (file)
@@ -1,3 +1,13 @@
+2019-03-23  Mark Lam  <mark.lam@apple.com>
+
+        Rolling out r243032 and r243071 because the fix is incorrect.
+        https://bugs.webkit.org/show_bug.cgi?id=195892
+        <rdar://problem/48981239>
+
+        Not reviewed.
+
+        * platform/mac/TestExpectations:
+
 2019-03-23  Justin Fan  <justin_fan@apple.com>
 
         [Web GPU] Prototype compute pipeline with MSL
index 29246ea..0a20469 100644 (file)
@@ -1069,7 +1069,7 @@ webkit.org/b/167184 [ Debug ] inspector/indexeddb/clearObjectStore.html [ Pass T
 webkit.org/b/159684 [ Debug ] inspector/indexeddb/deleteDatabaseNamesWithSpace.html [ Pass Timeout ]
 webkit.org/b/153894 [ Debug ] inspector/model/color.html [ Pass Timeout ]
 webkit.org/b/148636 inspector/model/parse-script-syntax-tree.html [ Pass Timeout ]
-webkit.org/b/148636 inspector/model/remote-object.html [ Pass Timeout Failure ]
+webkit.org/b/148636 inspector/model/remote-object.html [ Pass Timeout ]
 webkit.org/b/167265 inspector/network/client-blocked-load.html [ Pass Timeout ]
 webkit.org/b/148636 inspector/page/main-frame-resource.html [ Pass Timeout ]
 webkit.org/b/168146 [ Debug ] inspector/protocol/inspector-backend-invocation-return-value.html [ Pass Timeout ]
index f375a88..461b7ad 100644 (file)
@@ -1,3 +1,33 @@
+2019-03-23  Mark Lam  <mark.lam@apple.com>
+
+        Rolling out r243032 and r243071 because the fix is incorrect.
+        https://bugs.webkit.org/show_bug.cgi?id=195892
+        <rdar://problem/48981239>
+
+        Not reviewed.
+
+        The fix is incorrect: it relies on being able to determine liveness of an object
+        in an ObjectPropertyCondition based on the state of the object's MarkedBit.
+        However, there's no guarantee that GC has run and that the MarkedBit is already
+        set even if the object is live.  As a result, we may not re-install adaptive
+        watchpoints based on presumed dead objects which are actually live.
+
+        I'm rolling this out, and will implement a more comprehensive fix to handle
+        watchpoint liveness later.
+
+        * bytecode/AdaptiveInferredPropertyValueWatchpointBase.cpp:
+        (JSC::AdaptiveInferredPropertyValueWatchpointBase::fire):
+        * bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.cpp:
+        (JSC::LLIntPrototypeLoadAdaptiveStructureWatchpoint::fireInternal):
+        * bytecode/ObjectPropertyCondition.cpp:
+        (JSC::ObjectPropertyCondition::dumpInContext const):
+        * bytecode/StructureStubClearingWatchpoint.cpp:
+        (JSC::StructureStubClearingWatchpoint::fireInternal):
+        * dfg/DFGAdaptiveStructureWatchpoint.cpp:
+        (JSC::DFG::AdaptiveStructureWatchpoint::fireInternal):
+        * runtime/StructureRareData.cpp:
+        (JSC::ObjectToStringAdaptiveStructureWatchpoint::fireInternal):
+
 2019-03-23  Keith Miller  <keith_miller@apple.com>
 
         Refactor clz/ctz and fix getLSBSet.
index b3727db..b26fbf6 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2015-2019 Apple Inc. All rights reserved.
+ * Copyright (C) 2015-2017 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -62,9 +62,7 @@ void AdaptiveInferredPropertyValueWatchpointBase::fire(VM& vm, const FireDetail&
     if (!isValid())
         return;
 
-    // FIXME: The m_key.isStillLive() check should not be needed if this watchpoint was removed
-    // when m_key's m_object died. https://bugs.webkit.org/show_bug.cgi?id=195829
-    if (m_key.isStillLive() && m_key.isWatchable(PropertyCondition::EnsureWatchability)) {
+    if (m_key.isWatchable(PropertyCondition::EnsureWatchability)) {
         install(vm);
         return;
     }
index a131fa1..6beddf9 100644 (file)
@@ -49,9 +49,7 @@ void LLIntPrototypeLoadAdaptiveStructureWatchpoint::install(VM& vm)
 
 void LLIntPrototypeLoadAdaptiveStructureWatchpoint::fireInternal(VM& vm, const FireDetail&)
 {
-    // FIXME: The m_key.isStillLive() check should not be needed if this watchpoint was removed
-    // when m_key's m_object died. https://bugs.webkit.org/show_bug.cgi?id=195829
-    if (m_key.isStillLive() && m_key.isWatchable(PropertyCondition::EnsureWatchability)) {
+    if (m_key.isWatchable(PropertyCondition::EnsureWatchability)) {
         install(vm);
         return;
     }
index 565bd5a..3aad094 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2015-2019 Apple Inc. All rights reserved.
+ * Copyright (C) 2015 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -37,14 +37,7 @@ void ObjectPropertyCondition::dumpInContext(PrintStream& out, DumpContext* conte
         out.print("<invalid>");
         return;
     }
-
-    // FIXME: The m_key.isStillLive() check should not be needed if the watchpoint using this
-    // condition was removed when m_object died. https://bugs.webkit.org/show_bug.cgi?id=195829
-    if (!isStillLive()) {
-        out.print("<not live>");
-        return;
-    }
-
+    
     out.print("<", inContext(JSValue(m_object), context), ": ", inContext(m_condition, context), ">");
 }
 
index 783f7ea..648b1e8 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2012-2019 Apple Inc. All rights reserved.
+ * Copyright (C) 2012, 2015-2016 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -36,9 +36,7 @@ namespace JSC {
 
 void StructureStubClearingWatchpoint::fireInternal(VM& vm, const FireDetail&)
 {
-    // FIXME: The m_key.isStillLive() check should not be needed if this watchpoint was removed
-    // when m_key's m_object died. https://bugs.webkit.org/show_bug.cgi?id=195829
-    if (!m_key.isStillLive() || !m_key.isWatchable(PropertyCondition::EnsureWatchability)) {
+    if (!m_key || !m_key.isWatchable(PropertyCondition::EnsureWatchability)) {
         // This will implicitly cause my own demise: stub reset removes all watchpoints.
         // That works, because deleting a watchpoint removes it from the set's list, and
         // the set's list traversal for firing is robust against the set changing.
index 8d6e194..d4a8b9a 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2015-2019 Apple Inc. All rights reserved.
+ * Copyright (C) 2015 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -52,9 +52,7 @@ void AdaptiveStructureWatchpoint::install(VM& vm)
 
 void AdaptiveStructureWatchpoint::fireInternal(VM& vm, const FireDetail& detail)
 {
-    // FIXME: The m_key.isStillLive() check should not be needed if this watchpoint was removed
-    // when m_key's m_object died. https://bugs.webkit.org/show_bug.cgi?id=195829
-    if (m_key.isStillLive() && m_key.isWatchable(PropertyCondition::EnsureWatchability)) {
+    if (m_key.isWatchable(PropertyCondition::EnsureWatchability)) {
         install(vm);
         return;
     }
index 4568932..d5606cd 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2013-2019 Apple Inc. All rights reserved.
+ * Copyright (C) 2013-2017 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -191,9 +191,7 @@ void ObjectToStringAdaptiveStructureWatchpoint::fireInternal(VM& vm, const FireD
     if (!m_structureRareData->isLive())
         return;
 
-    // FIXME: The m_key.isStillLive() check should not be needed if this watchpoint was removed
-    // when m_key's m_object died. https://bugs.webkit.org/show_bug.cgi?id=195829
-    if (m_key.isStillLive() && m_key.isWatchable(PropertyCondition::EnsureWatchability)) {
+    if (m_key.isWatchable(PropertyCondition::EnsureWatchability)) {
         install(vm);
         return;
     }