Title: [158341] trunk/Source/_javascript_Core
- Revision
- 158341
- Author
- [email protected]
- Date
- 2013-10-30 20:52:23 -0700 (Wed, 30 Oct 2013)
Log Message
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:
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/ChangeLog (158340 => 158341)
--- trunk/Source/_javascript_Core/ChangeLog 2013-10-31 03:37:00 UTC (rev 158340)
+++ trunk/Source/_javascript_Core/ChangeLog 2013-10-31 03:52:23 UTC (rev 158341)
@@ -1,3 +1,44 @@
+2013-10-30 Filip Pizlo <[email protected]>
+
+ 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 <[email protected]>
Unreviewed, fix C Loop LLINT build.
Modified: trunk/Source/_javascript_Core/bytecode/CodeBlock.cpp (158340 => 158341)
--- trunk/Source/_javascript_Core/bytecode/CodeBlock.cpp 2013-10-31 03:37:00 UTC (rev 158340)
+++ trunk/Source/_javascript_Core/bytecode/CodeBlock.cpp 2013-10-31 03:52:23 UTC (rev 158341)
@@ -2817,7 +2817,7 @@
dataLog(".\n");
}
- DeferGC deferGC(*m_heap);
+ DeferGCForAWhile deferGC(*m_heap);
RELEASE_ASSERT(JITCode::isOptimizingJIT(jitType()));
// We want to accomplish two things here:
Modified: trunk/Source/_javascript_Core/bytecode/Watchpoint.cpp (158340 => 158341)
--- trunk/Source/_javascript_Core/bytecode/Watchpoint.cpp 2013-10-31 03:37:00 UTC (rev 158340)
+++ trunk/Source/_javascript_Core/bytecode/Watchpoint.cpp 2013-10-31 03:52:23 UTC (rev 158341)
@@ -46,10 +46,12 @@
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)
Modified: trunk/Source/_javascript_Core/bytecode/Watchpoint.h (158340 => 158341)
--- trunk/Source/_javascript_Core/bytecode/Watchpoint.h 2013-10-31 03:37:00 UTC (rev 158340)
+++ trunk/Source/_javascript_Core/bytecode/Watchpoint.h 2013-10-31 03:52:23 UTC (rev 158341)
@@ -53,7 +53,7 @@
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
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes
