Bug 121710 - REGRESSION(r153215): New iCloud site crashes
Summary: REGRESSION(r153215): New iCloud site crashes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Oliver Hunt
URL: http://icloud.com
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-09-20 13:29 PDT by Oliver Hunt
Modified: 2013-09-20 16:59 PDT (History)
4 users (show)

See Also:


Attachments
Dumperation (1.49 MB, text/plain)
2013-09-20 15:03 PDT, Oliver Hunt
no flags Details
Patch (4.17 KB, patch)
2013-09-20 16:48 PDT, Oliver Hunt
fpizlo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2013-09-20 13:29:55 PDT
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.
Comment 1 Oliver Hunt 2013-09-20 13:30:26 PDT
<rdar://problem/15017146>
Comment 2 Oliver Hunt 2013-09-20 14:39:22 PDT
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 *****
Comment 3 Oliver Hunt 2013-09-20 14:56:50 PDT
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)
Comment 4 Filip Pizlo 2013-09-20 14:59:36 PDT
(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.
Comment 5 Oliver Hunt 2013-09-20 15:02:21 PDT
(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)
Comment 6 Oliver Hunt 2013-09-20 15:03:47 PDT
Created attachment 212226 [details]
Dumperation

Here's a dump, ******** to mark the trapping instruction
Comment 7 Filip Pizlo 2013-09-20 15:09:42 PDT
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.
Comment 8 Oliver Hunt 2013-09-20 16:48:42 PDT
Created attachment 212241 [details]
Patch
Comment 9 Filip Pizlo 2013-09-20 16:55:39 PDT
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.
Comment 10 Oliver Hunt 2013-09-20 16:59:27 PDT
Committed r156211: <http://trac.webkit.org/changeset/156211>