[Webkit-unassigned] [Bug 87994] ASSERTION FAILED: m_refCount in DFG::Node:deref with patch from 87158

Tue Jun 5 13:43:59 PDT 2012

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

--- Comment #7 from Filip Pizlo <fpizlo at apple.com>  2012-06-05 13:43:57 PST ---
(In reply to comment #6)
> If my analysis is right, I'm not sure what the right fix is.  One can easily have arbitrarily long cycles that could exhibit similar behavior.

Here's one answer, which is correct in general but incorrect in this particular case (see below, for the correct, but less general, answer): you call call Graph::collectGarbage(), which will reset all ref counts based on a tracing GC over the graph.

> 
> This patch fixes the immediate symptom:
> 
> diff --git a/Source/JavaScriptCore/dfg/DFGCFGSimplificationPhase.cpp b/Source/JavaScriptCore/dfg/DFGCFGSimplificationPhase.cpp
> index 0f0a225..84286d3 100644
> --- a/Source/JavaScriptCore/dfg/DFGCFGSimplificationPhase.cpp
> +++ b/Source/JavaScriptCore/dfg/DFGCFGSimplificationPhase.cpp
> @@ -365,6 +365,10 @@ private:
> 
>      void fixPhis(BlockIndex sourceBlockIndex, BlockIndex destinationBlockIndex)
>      {
> +        if (sourceBlockIndex == destinationBlockIndex) {
> +            // No need to kill off phis referenced from our own block.
> +            return;
> +        }
>          BasicBlock* sourceBlock = m_graph.m_blocks[sourceBlockIndex].get();
>          BasicBlock* destinationBlock = m_graph.m_blocks[destinationBlockIndex].get();
>          if (!destinationBlock) {

That's probably wrong, since you'll end up with Phi references to code that was deleted, which ought to almost certainly lead to hilarity.

> 
> However I get other problems on paperjs.org, including a segfault in meta balls:
> 
> 
> Program received signal SIGSEGV, Segmentation fault.
> 0x00007ffff1facd86 in JSC::JSCell::classInfo (this=0x0) at ../../Source/JavaScriptCore/runtime/JSCell.h:195
> 195            return m_classInfo;
> (gdb) bt
> #0  0x00007ffff1facd86 in JSC::JSCell::classInfo (this=0x0) at ../../Source/JavaScriptCore/runtime/JSCell.h:195
> #1  0x00007ffff1faed96 in JSC::JSCell::methodTable (this=0x0) at ../../Source/JavaScriptCore/runtime/JSObject.h:536
> #2  0x00007ffff20ad801 in JSC::JSValue::get (this=0x7fffffffcde0, exec=0x7fff96f963b0, propertyName=0, slot=...) at ../../Source/JavaScriptCore/runtime/JSObject.h:843
> #3  0x00007ffff20ad755 in JSC::JSValue::get (this=0x7fffffffcde0, exec=0x7fff96f963b0, propertyName=0) at ../../Source/JavaScriptCore/runtime/JSObject.h:830
> #4  0x00007ffff20aae9b in JSC::DFG::operationGetArgumentByVal (exec=0x7fff96f963b0, argumentsRegister=1, index=0) at ../../Source/JavaScriptCore/dfg/DFGOperations.cpp:1108
> #5  0x00007fffa3a3107f in ?? ()
> 
> And voronoi prints out this on the console, many times:
> 
> ** Message: console message: http://jonathanpuckey.com/static/rhill-voronoi-core.js @284: TypeError: 'null' is not an object

That may well be a different bug.  I'll look into it.

> 
> Michael, can you reproduce any of these?  Filip, do you have any thoughts here?

Now for the (hopefully) correct answer.  The problem was that fixPhis() was being used in two subtly different cases, but was assuming that it was only being used in one of them and did wrong things for the other case.

Case #1, or the Jettisoned Block case:

Consider the control flow graph consisting of blocks A, B, C.  A initially has a branch to B and C based on some predicate (B if true, C if false).  But constant folding proves this predicate to be true, leading to C being jettisoned.  We then call fixPhis() with A as the source block and C as the destination block.  In this case, A is a reachable block, and C may or may not be reachable (note that other reachable blocks could still jump to C).  Regardless of whether or not C is reachable at this point (we don't need to know), we need to ensure that any Phis in C no longer refer to A's nodes, since A is no longer a predecessor of C.  In the process of removing those Phi references, we must ensure that the thing that the Phi points to gets deref'd.

Note it's also possible to have blocks A and B, where A branches to either A or B - i.e. a loop.  Then A will potentially have Phi loops.  But it will only have *live* Phi loops if the variables for those Phis are used outside of the loop.  Hence, we will not encounter this infinite deref'ing because the Phi's ref counts will never hit zero.

Case #2, or the Unreachable Block case:

Consider the control flow graph consisting of blocks A, B, C, D.  A initially had a branch to B and C based on some predicate (B if true, C if false).  But constant folding proves this predicate to be true, leading to C being jettisoned.  Initially we do case #1 above, but then we have more work to do: assume that there are no longer any other jumps to C, making C unreachable.  This means that D will have Phi functions that refer into C; these must now be fixed up, since C is no longer a predecessor of D since C is unreachable.

In this case we call fixPhis() with C as the source block and D as the destination block.  But unlike case #1, the source block (C) is unreachable.  Hence, although we need to remove references into C from D, we don't need to do any deref's.  This is because all of C is going away anyway.  It will cease to exist.  Its ref counts don't matter.  There's no point in getting them right.

In this case it is certainly possible for there to be a dead cycle, and deref'ing this dead cycle will lead to infinite recursion and horror and badness.  (Though the infinite recursion would be caught by an ASSERT.)    It is possible to fix that by calling collectGarbage().  But we don't need to do any of that, because the ref counts of C's nodes are irrelevant - all that matters is that D just doesn't refer into C anymore.

So, in short: the fix is to make fixPhis() deref only if the destination Phi is shouldGenerate() *and* if the source block is reachable.

I'm testing this fix right now.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.