ObjectToStringAdaptiveStructureWatchpoint should not fire if it's dying imminently.

https://bugs.webkit.org/show_bug.cgi?id=174948
<rdar://problem/33495680>

Reviewed by Filip Pizlo.

JSTests:

* stress/regress-174948.js: Added.

Source/JavaScriptCore:

ObjectToStringAdaptiveStructureWatchpoint is owned by StructureRareData.  If its
owner StructureRareData is already known to be dead (in terms of GC liveness) but
hasn't been destructed yet (i.e. not swept by the GC yet), we should ignore all
requests to fire this watchpoint.

If the GC had the chance to sweep the StructureRareData, thereby destructing the
ObjectToStringAdaptiveStructureWatchpoint, it (the watchpoint) would have removed
itself from the WatchpointSet it was on.  Hence, it would not have been fired.

But since the watchpoint hasn't been destructed yet, it still remains on the
WatchpointSet and needs to guard against being fired in this state.  The fix is
to simply return early if its owner StructureRareData is not live.  This has the
effect of the watchpoint fire being a no-op, which is equivalent to the watchpoint
not firing as we would expect.

This patch also removes some cargo cult copying of watchpoint code which
instantiates a StringFireDetail.  In a few cases, that StringFireDetail is never
used.  This patch removes these unnecessary instantiations.

* bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.cpp:
(JSC::LLIntPrototypeLoadAdaptiveStructureWatchpoint::fireInternal):
* runtime/StructureRareData.cpp:
(JSC::ObjectToStringAdaptiveStructureWatchpoint::fireInternal):
(JSC::ObjectToStringAdaptiveInferredPropertyValueWatchpoint::handleFire):

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

diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
index 3f406e09d2ec913641ba2768810d9df02641d965..e3dabd9e99debe631044be3cb489045c1773553f 100644 (file)
--- a/JSTests/ChangeLog
+++ b/JSTests/ChangeLog
@@ -1,3 +1,13 @@
+2017-07-28  Mark Lam  <mark.lam@apple.com>
+
+        ObjectToStringAdaptiveStructureWatchpoint should not fire if it's dying imminently.
+        https://bugs.webkit.org/show_bug.cgi?id=174948
+        <rdar://problem/33495680>
+
+        Reviewed by Filip Pizlo.
+
+        * stress/regress-174948.js: Added.
+
 2017-07-28  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         ASSERTION FAILED: candidate->op() == PhantomCreateRest || candidate->op() == PhantomDirectArguments || candidate->op() == PhantomClonedArguments || candidate->op() == PhantomSpread || candidate->op() == PhantomNewArrayWithSpread

diff --git a/JSTests/stress/regress-174948.js b/JSTests/stress/regress-174948.js
new file mode 100644 (file)
index 0000000..acc0dd3
--- /dev/null
+++ b/JSTests/stress/regress-174948.js
@@ -0,0 +1,5 @@
++new function() {};
+new function() {};
+edenGC();
+(function() {}).prototype[0] = 0;
+

diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
index f0e13768b920ea9d3e991c08a6021bd2d5cfdccb..35c7ee20b042fb60637e26550f471e0ee9348148 100644 (file)
--- a/Source/JavaScriptCore/ChangeLog
+++ b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,36 @@
+2017-07-28  Mark Lam  <mark.lam@apple.com>
+
+        ObjectToStringAdaptiveStructureWatchpoint should not fire if it's dying imminently.
+        https://bugs.webkit.org/show_bug.cgi?id=174948
+        <rdar://problem/33495680>
+
+        Reviewed by Filip Pizlo.
+
+        ObjectToStringAdaptiveStructureWatchpoint is owned by StructureRareData.  If its
+        owner StructureRareData is already known to be dead (in terms of GC liveness) but
+        hasn't been destructed yet (i.e. not swept by the GC yet), we should ignore all
+        requests to fire this watchpoint.
+
+        If the GC had the chance to sweep the StructureRareData, thereby destructing the
+        ObjectToStringAdaptiveStructureWatchpoint, it (the watchpoint) would have removed
+        itself from the WatchpointSet it was on.  Hence, it would not have been fired.
+
+        But since the watchpoint hasn't been destructed yet, it still remains on the
+        WatchpointSet and needs to guard against being fired in this state.  The fix is
+        to simply return early if its owner StructureRareData is not live.  This has the
+        effect of the watchpoint fire being a no-op, which is equivalent to the watchpoint
+        not firing as we would expect.
+
+        This patch also removes some cargo cult copying of watchpoint code which
+        instantiates a StringFireDetail.  In a few cases, that StringFireDetail is never
+        used.  This patch removes these unnecessary instantiations.
+
+        * bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.cpp:
+        (JSC::LLIntPrototypeLoadAdaptiveStructureWatchpoint::fireInternal):
+        * runtime/StructureRareData.cpp:
+        (JSC::ObjectToStringAdaptiveStructureWatchpoint::fireInternal):
+        (JSC::ObjectToStringAdaptiveInferredPropertyValueWatchpoint::handleFire):
+
 2017-07-28  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         ASSERTION FAILED: candidate->op() == PhantomCreateRest || candidate->op() == PhantomDirectArguments || candidate->op() == PhantomClonedArguments || candidate->op() == PhantomSpread || candidate->op() == PhantomNewArrayWithSpread

diff --git a/Source/JavaScriptCore/bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.cpp b/Source/JavaScriptCore/bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.cpp
index 9a5ac011232f32aaa9afc8b3e78a04a898b7809f..d6cc22d5150df8ecdb8bb8a35c5acd2c57f71e50 100644 (file)
--- a/Source/JavaScriptCore/bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.cpp
+++ b/Source/JavaScriptCore/bytecode/LLIntPrototypeLoadAdaptiveStructureWatchpoint.cpp
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2016-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
@@ -47,18 +47,13 @@ void LLIntPrototypeLoadAdaptiveStructureWatchpoint::install()
     m_key.object()->structure()->addTransitionWatchpoint(this);
 }
 
-void LLIntPrototypeLoadAdaptiveStructureWatchpoint::fireInternal(const FireDetail& detail)
+void LLIntPrototypeLoadAdaptiveStructureWatchpoint::fireInternal(const FireDetail&)
 {
     if (m_key.isWatchable(PropertyCondition::EnsureWatchability)) {
         install();
         return;
     }
 
-    StringPrintStream out;
-    out.print("ObjectToStringValue Adaptation of ", m_key, " failed: ", detail);
-
-    StringFireDetail stringDetail(out.toCString().data());
-
     CodeBlock::clearLLIntGetByIdCache(m_getByIdInstruction);
 }
 

diff --git a/Source/JavaScriptCore/runtime/StructureRareData.cpp b/Source/JavaScriptCore/runtime/StructureRareData.cpp
index 21d6248d4b620d270ae285735d07f72465422d6d..ae301e269fe07a2c7a9d4333a28b81373522f3f6 100644 (file)
--- a/Source/JavaScriptCore/runtime/StructureRareData.cpp
+++ b/Source/JavaScriptCore/runtime/StructureRareData.cpp
@@ -192,18 +192,16 @@ void ObjectToStringAdaptiveStructureWatchpoint::install()
     m_key.object()->structure()->addTransitionWatchpoint(this);
 }
 
-void ObjectToStringAdaptiveStructureWatchpoint::fireInternal(const FireDetail& detail)
+void ObjectToStringAdaptiveStructureWatchpoint::fireInternal(const FireDetail&)
 {
+    if (!m_structureRareData->isLive())
+        return;
+
     if (m_key.isWatchable(PropertyCondition::EnsureWatchability)) {
         install();
         return;
     }
 
-    StringPrintStream out;
-    out.print("ObjectToStringValue Adaptation of ", m_key, " failed: ", detail);
-
-    StringFireDetail stringDetail(out.toCString().data());
-
     m_structureRareData->clearObjectToStringValue();
 }
 
@@ -218,13 +216,8 @@ bool ObjectToStringAdaptiveInferredPropertyValueWatchpoint::isValid() const
     return m_structureRareData->isLive();
 }
 
-void ObjectToStringAdaptiveInferredPropertyValueWatchpoint::handleFire(const FireDetail& detail)
+void ObjectToStringAdaptiveInferredPropertyValueWatchpoint::handleFire(const FireDetail&)
 {
-    StringPrintStream out;
-    out.print("Adaptation of ", key(), " failed: ", detail);
-    
-    StringFireDetail stringDetail(out.toCString().data());
-    
     m_structureRareData->clearObjectToStringValue();
 }