Bug 121710 – REGRESSION(r153215): New iCloud site crashes

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Oliver Hunt]

Debug builds of r153215 crash reasonably reliably with:
 JSC_enableConcurrentJIT=0 JSC_bytecodeRangeToDFGCompile=47:49 JSC_maximumInliningDepth=2 run-safari


Release builds crash ~100% of the time.

[Oliver Hunt]

<rdar://problem/15017146>

[Oliver Hunt]

My bad, it's actually the prior node:
        66:           <!0:->	CheckStructure(Cell:@32<Arguments>, MustGen|CanExit, struct(0x11cba9700: NonArray), bc#19)
            0x2af4da9393c0: mov $0x11cba9700, %r11
            0x2af4da9393ca: cmp %r11, (%rcx)
            0x2af4da9393cd: jnz 0x2af4da9396e4
int3 here *****

[Oliver Hunt]

If we bring back the assertion here:
    for (m_indexInBlock = 0; m_indexInBlock < block.size(); ++m_indexInBlock) {
        m_currentNode = block[m_indexInBlock];
        
        // We may have his a contradiction that the CFA was aware of but that the JIT
        // didn't cause directly.
        if (!m_state.isValid()) {
            RELEASE_ASSERT_NOT_REACHED(); <---
            bail();
            return;
        }

We hit it, implying the CFA is deciding there's a contradiction:

  --> capitalize#AzCeyu:<0x117848e70, bc#39, Call, known callee: Cell: 0x117c18430 (0x10a93f270: Function, NonArray), numArgs+this = 3, stack >= r12>
    34:           <!0:->	InlineStart(MustGen, bc#0)
    35:  skipped  < 0:->	MovHint(@9<String>, r15(M~<String>), bc#1)
    36:           <!0:->	CheckStructure(Cell:@9<String>, MustGen|CanExit, struct(0x10a93d2f0: NonArray), bc#4)

[Filip Pizlo]

(In reply to comment #3)
> If we bring back the assertion here:
>     for (m_indexInBlock = 0; m_indexInBlock < block.size(); ++m_indexInBlock) {
>         m_currentNode = block[m_indexInBlock];
> 
>         // We may have his a contradiction that the CFA was aware of but that the JIT
>         // didn't cause directly.
>         if (!m_state.isValid()) {
>             RELEASE_ASSERT_NOT_REACHED(); <---
>             bail();
>             return;
>         }
> 
> We hit it, implying the CFA is deciding there's a contradiction:
> 
>   --> capitalize#AzCeyu:<0x117848e70, bc#39, Call, known callee: Cell: 0x117c18430 (0x10a93f270: Function, NonArray), numArgs+this = 3, stack >= r12>
>     34:           <!0:->    InlineStart(MustGen, bc#0)
>     35:  skipped  < 0:->    MovHint(@9<String>, r15(M~<String>), bc#1)
>     36:           <!0:->    CheckStructure(Cell:@9<String>, MustGen|CanExit, struct(0x10a93d2f0: NonArray), bc#4)

Can you post the whole IR?  I have no idea, from looking at a CheckStructure node in isolation from everything else, why there's a contradiction.

Also, to be clear, putting a RELEASE_ASSERT_NOT_REACHED() when we bail at the top of a basic block is not correct.  It's fine if you're using it for your testing but it's totally OK for the CFA to decide that a basic block is unreachable.  It happens a lot.

[Oliver Hunt]

(In reply to comment #4)
> (In reply to comment #3)
> > If we bring back the assertion here:
> >     for (m_indexInBlock = 0; m_indexInBlock < block.size(); ++m_indexInBlock) {
> >         m_currentNode = block[m_indexInBlock];
> > 
> >         // We may have his a contradiction that the CFA was aware of but that the JIT
> >         // didn't cause directly.
> >         if (!m_state.isValid()) {
> >             RELEASE_ASSERT_NOT_REACHED(); <---
> >             bail();
> >             return;
> >         }
> > 
> > We hit it, implying the CFA is deciding there's a contradiction:
> > 
> >   --> capitalize#AzCeyu:<0x117848e70, bc#39, Call, known callee: Cell: 0x117c18430 (0x10a93f270: Function, NonArray), numArgs+this = 3, stack >= r12>
> >     34:           <!0:->    InlineStart(MustGen, bc#0)
> >     35:  skipped  < 0:->    MovHint(@9<String>, r15(M~<String>), bc#1)
> >     36:           <!0:->    CheckStructure(Cell:@9<String>, MustGen|CanExit, struct(0x10a93d2f0: NonArray), bc#4)
> 
> Can you post the whole IR?  I have no idea, from looking at a CheckStructure node in isolation from everything else, why there's a contradiction.
> 
> Also, to be clear, putting a RELEASE_ASSERT_NOT_REACHED() when we bail at the top of a basic block is not correct.  It's fine if you're using it for your testing but it's totally OK for the CFA to decide that a basic block is unreachable.  It happens a lot.

I was using it for testing (i'm currently on r153215)

[Oliver Hunt]

Created attachment 212226 [details]
Dumperation

Here's a dump, ******** to mark the trapping instruction

[Filip Pizlo]

Lololololo.  That's hilarious.  We have:

12: CreateArguments(...)
// bunch of code that stores properties into the arguments, resulting in @12 having structure S
32: CreateArguments(@12)
// stuff
66: CheckStructure(@32, structure S)

The CFA for CreateArguments claims that it always returns an object with the arguments structure.  That's totally wrong, if we have this CreateArguments(CreateArguments) - in that case it will return an object that has whatever structure the previous CreateArguments had.

The correct solution is to make:

    case CreateArguments:
        forNode(node).set(
            m_graph, m_codeBlock->globalObjectFor(node->codeOrigin)->argumentsStructure());
        m_state.setHaveStructures(true);
        break;

instead be:

    case CreateArguments:
        forNode(node).setType(SpecArguments);
        break;

That's somewhat pessimistic but it'll fix the bug.  It's unlikely to hurt performance, but I would test first.

[Oliver Hunt]

Created attachment 212241 [details]
Patch

[Filip Pizlo]

Comment on attachment 212241 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=212241&action=review

r=me if you fix the test.

> LayoutTests/js/script-tests/dfg-arguments-mutated-structure.js:12
> +while(!dfgCompiled({f:foo}))
> +    foo();
> +
> +shouldBeTrue("foo()");

This is wrong.  You're forgetting to say noInline(foo).

The more canonical way to write a test like this is:

dfgShouldBe(foo, "foo()", "true");

dfgShouldBe() will automatically noInline and will automatically run enough times.

[Oliver Hunt]

Committed r156211: <http://trac.webkit.org/changeset/156211>