Assertion failure in js/dom/global-constructors-attributes-dedicated-worker.html
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 31 Oct 2013 03:52:23 +0000 (03:52 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 31 Oct 2013 03:52:23 +0000 (03:52 +0000)
https://bugs.webkit.org/show_bug.cgi?id=123551
<rdar://problem/15356238>

Reviewed by Mark Hahnenberg.

WatchpointSets have always had this "fire everything on deletion" policy because it
seemed like a good fail-safe at the time I first implemented WatchpointSets. But
it's actually causing bugs rather than providing safety:

- Everyone who registers Watchpoints with WatchpointSets have separate mechanisms
  for either keeping the WatchpointSets alive or noticing when they are collected.
  So this wasn't actually providing any safety.

  One example of this is Structures, where:

  - CodeBlocks that register Watchpoints on Structure's WatchpointSet will also
    register weak references to the Structure, and the GC will jettison a CodeBlock
    if the Structure(s) it cares about dies.

  - StructureStubInfos that register Watchpoints on Structure's WatchpointSet will
    also be cleared by GC if the Structures die.

- The WatchpointSet destructor would get invoked from finalization/destruction.
  This would then cause CodeBlock::jettison() to be called on a CodeBlock, but that
  method requires doing things that access heap objects. This would usually cause
  problems on VM destruction, since then the CodeBlocks would still be alive but the
  whole heap would be destroyed.

This also ensures that CodeBlock::jettison() cannot cause a GC. This is safe since
that method doesn't really allocate objects, and it is likely necessary because
jettison() may be called from deep in the stack.

* bytecode/CodeBlock.cpp:
(JSC::CodeBlock::jettison):
* bytecode/Watchpoint.cpp:
(JSC::WatchpointSet::~WatchpointSet):
* bytecode/Watchpoint.h:

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/CodeBlock.cpp
Source/JavaScriptCore/bytecode/Watchpoint.cpp
Source/JavaScriptCore/bytecode/Watchpoint.h

index 8d795c6..3b68126 100644 (file)
@@ -1,3 +1,44 @@
+2013-10-30  Filip Pizlo  <fpizlo@apple.com>
+
+        Assertion failure in js/dom/global-constructors-attributes-dedicated-worker.html
+        https://bugs.webkit.org/show_bug.cgi?id=123551
+        <rdar://problem/15356238>
+
+        Reviewed by Mark Hahnenberg.
+        
+        WatchpointSets have always had this "fire everything on deletion" policy because it
+        seemed like a good fail-safe at the time I first implemented WatchpointSets. But
+        it's actually causing bugs rather than providing safety:
+        
+        - Everyone who registers Watchpoints with WatchpointSets have separate mechanisms
+          for either keeping the WatchpointSets alive or noticing when they are collected.
+          So this wasn't actually providing any safety.
+          
+          One example of this is Structures, where:
+          
+          - CodeBlocks that register Watchpoints on Structure's WatchpointSet will also
+            register weak references to the Structure, and the GC will jettison a CodeBlock
+            if the Structure(s) it cares about dies.
+          
+          - StructureStubInfos that register Watchpoints on Structure's WatchpointSet will
+            also be cleared by GC if the Structures die.
+        
+        - The WatchpointSet destructor would get invoked from finalization/destruction.
+          This would then cause CodeBlock::jettison() to be called on a CodeBlock, but that
+          method requires doing things that access heap objects. This would usually cause
+          problems on VM destruction, since then the CodeBlocks would still be alive but the
+          whole heap would be destroyed.
+        
+        This also ensures that CodeBlock::jettison() cannot cause a GC. This is safe since
+        that method doesn't really allocate objects, and it is likely necessary because
+        jettison() may be called from deep in the stack.
+
+        * bytecode/CodeBlock.cpp:
+        (JSC::CodeBlock::jettison):
+        * bytecode/Watchpoint.cpp:
+        (JSC::WatchpointSet::~WatchpointSet):
+        * bytecode/Watchpoint.h:
+
 2013-10-30  Mark Lam  <mark.lam@apple.com>
 
         Unreviewed, fix C Loop LLINT build.
index af2945d..c85395e 100644 (file)
@@ -2817,7 +2817,7 @@ void CodeBlock::jettison(ReoptimizationMode mode)
         dataLog(".\n");
     }
     
-    DeferGC deferGC(*m_heap);
+    DeferGCForAWhile deferGC(*m_heap);
     RELEASE_ASSERT(JITCode::isOptimizingJIT(jitType()));
     
     // We want to accomplish two things here:
index be8270b..1b5b030 100644 (file)
@@ -46,10 +46,12 @@ WatchpointSet::WatchpointSet(InitialWatchpointSetMode mode)
 
 WatchpointSet::~WatchpointSet()
 {
-    // Fire all watchpoints. This is necessary because it is possible, say with
-    // structure watchpoints, for the watchpoint set owner to die while the
-    // watchpoint owners are still live.
-    fireAllWatchpoints();
+    // Remove all watchpoints, so that they don't try to remove themselves. Note that we
+    // don't fire watchpoints on deletion. We assume that any code that is interested in
+    // watchpoints already also separately has a mechanism to make sure that the code is
+    // either keeping the watchpoint set's owner alive, or does some weak reference thing.
+    while (!m_set.isEmpty())
+        m_set.begin()->remove();
 }
 
 void WatchpointSet::add(Watchpoint* watchpoint)
index 69205a4..cd0a25e 100644 (file)
@@ -53,7 +53,7 @@ class WatchpointSet : public ThreadSafeRefCounted<WatchpointSet> {
     friend class LLIntOffsetsExtractor;
 public:
     WatchpointSet(InitialWatchpointSetMode);
-    ~WatchpointSet();
+    ~WatchpointSet(); // Note that this will not fire any of the watchpoints; if you need to know when a WatchpointSet dies then you need a separate mechanism for this.
     
     // It is safe to call this from another thread.  It may return true
     // even if the set actually had been invalidated, but that ought to happen