Bug 112369 – Assertion failure !m_lastChunkBeforeScript in HTMLDocumentParser during inspector/debugger/pause-in-inline-script.html

• Component: DOM
• Status: RESOLVED INVALID

[Kenneth Russell]

Layout test inspector/debugger/pause-in-inline-script.html is crashing on chromium.webkit debug bots (all OSs) with the following assertion failure:

crash log for DumpRenderTree (pid 1073):
STDOUT: <empty>
STDERR: ASSERTION FAILED: !m_lastChunkBeforeScript
STDERR: ../../third_party/WebKit/Source/WebCore/html/parser/HTMLDocumentParser.cpp(339) : void WebCore::HTMLDocumentParser::validateSpeculations(WTF::PassOwnPtr<WebCore::HTMLDocumentParser::ParsedChunk>)
STDERR: 1   0xe00051
STDERR: 2   0xe00c76
STDERR: 3   0xdfff20
STDERR: 4   0xe979d3
STDERR: 5   0xe9790d
STDERR: 6   0x46082e
STDERR: 7   0x460530
STDERR: 8   0x1863e94
STDERR: 9   0x1863dc4
STDERR: 10  0x1863bb3
STDERR: 11  0x208955f
STDERR: 12  0x2764993
STDERR: 13  0x2764aaa
STDERR: 14  0x2765311
STDERR: 15  0x27cfb72
STDERR: 16  0x27cfff4
STDERR: 17  0x276459b
STDERR: 18  0x2764456
STDERR: 19  0x278f80c
STDERR: 20  0x2763d8e
STDERR: 21  0x18605bb
STDERR: 22  0x42bc55
STDERR: 23  0x425e92
STDERR: 24  0x41f4d0
STDERR: 25  0x42007b
STDERR: 26  0x7f436588b76d __libc_start_main
STDERR: 27  0x41dbc9
STDERR: Received signal 11 SEGV_MAPERR 0000bbadbeef
STDERR:  [0x000002744c18] base::debug::StackTrace::StackTrace()
STDERR:  [0x00000274451f] base::debug::(anonymous namespace)::StackDumpSignalHandler()
STDERR:  [0x7f4366861cb0] <unknown>
STDERR:  [0x000000e0005b] WebCore::HTMLDocumentParser::validateSpeculations()
STDERR:  [0x000000e00c76] WebCore::HTMLDocumentParser::processParsedChunkFromBackgroundParser()
STDERR:  [0x000000dfff20] WebCore::HTMLDocumentParser::didReceiveParsedChunkFromBackgroundParser()
STDERR:  [0x000000e979d3] WTF::FunctionWrapper<>::operator()()
STDERR:  [0x000000e9790d] WTF::BoundFunctionImpl<>::operator()()
STDERR:  [0x00000046082e] WTF::Function<>::operator()()
STDERR:  [0x000000460530] WTF::callFunctionObject()
STDERR:  [0x000001863e94] base::internal::RunnableAdapter<>::Run()
STDERR:  [0x000001863dc4] base::internal::InvokeHelper<>::MakeItSo()
STDERR:  [0x000001863bb3] base::internal::Invoker<>::Run()
STDERR:  [0x00000208955f] base::Callback<>::Run()
STDERR:  [0x000002764993] MessageLoop::RunTask()
STDERR:  [0x000002764aaa] MessageLoop::DeferOrRunPendingTask()
STDERR:  [0x000002765311] MessageLoop::DoWork()
STDERR:  [0x0000027cfb72] base::MessagePumpGlib::RunWithDispatcher()
STDERR:  [0x0000027cfff4] base::MessagePumpGlib::Run()
STDERR:  [0x00000276459b] MessageLoop::RunInternal()
STDERR:  [0x000002764456] MessageLoop::RunHandler()
STDERR:  [0x00000278f80c] base::RunLoop::Run()
STDERR:  [0x000002763d8e] MessageLoop::Run()
STDERR:  [0x0000018605bb] webkit_support::RunMessageLoop()
STDERR:  [0x00000042bc55] TestShell::waitTestFinished()
STDERR:  [0x000000425e92] TestShell::runFileTest()
STDERR:  [0x00000041f4d0] runTest()
STDERR:  [0x00000042007b] main
STDERR:  [0x7f436588b76d] __libc_start_main
STDERR:  [0x00000041dbc9] <unknown>
STDERR:   r8: 00007f4369e5c980  r9: 00000000004002d0 r10: 0000000002a8146e r11: 0000000000000000
STDERR:  r12: 00007fffa4dead28 r13: 00007fffa4deb8c0 r14: 0000000000000000 r15: 0000000000000000
STDERR:   di: 0000000000000000  si: 00000000efcdab90  bp: 00007fffa4de9fc0  bx: 0000351bd5acf900
STDERR:   dx: 00007f4365c24aa0  ax: 00000000bbadbeef  cx: 00007f436595091d  sp: 00007fffa4de9f10
STDERR:   ip: 0000000000e0005b efl: 0000000000010246 cgf: 0000000000000033 erf: 0000000000000006
STDERR:  trp: 000000000000000e msk: 0000000000000000 cr2: 00000000bbadbeef


Example failures:
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7%20%28dbg%29%282%29/builds/2395/steps/webkit_tests
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20%28dbg%29/builds/7136/steps/webkit_tests

[Adam Barth]

This is likely from Eric's recent patch.

[Eric Seidel (no email)]

Thanks.  I'll make sure to fix this on Monday if not before.

[Eric Seidel (no email)]

I suspect this may also be that we were doing something wonky here before, and now we're just catching it with ASSERTs.
http://trac.webkit.org/browser/trunk/Source/WebCore/html/parser/HTMLDocumentParser.cpp#L331

[Eric Seidel (no email)]

Hmm...  I have not been able to repro this yet:

run-webkit-tests --debug --chromium inspector/debugger
Found 74 tests; running 74, skipping 0.
Running 1 DumpRenderTree.       

All 74 tests ran as expected.

On r146004.

[Eric Seidel (no email)]

Have we seen this since?  I've not had any other reports of this.

[Kenneth Russell]

Yes, it still seems to be crashing regularly:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=inspector%2Fdebugger%2Fpause-in-inline-script.html

I think you'll need to remove the Crash/Timeout suppression in LayoutTests/platform/chromium/TestExpectations in order to have DRT report the problem.

[Eric Seidel (no email)]

I'm able to repro the ASSERT.

We're hitting the ASSERT when trying to set m_lastChunkBeforeScript when it's already set.  That means that we're handling this chunk on the main thread, before we were told that script execution was complete.

[Eric Seidel (no email)]

Created attachment 194006 [details]
backtrace of the bug

Turns out the debugger can cause us to re-enter didReceiveParsedChunkFromBackgroundParser on the main thread. :(

[Eric Seidel (no email)]

We're going to want to add some ASSERTs to catch reentrancy here, as we thought we had avoided any reentrancy possibly with the threaded parser design.  If we're hitting this in cases other than the inspector, we could be seeing badness.  As far as we know this test only ASSERTs in Debug.  I suspect the output changes slightly in Release, but there should be no crashes there.

[Eric Seidel (no email)]

Adam points out that window.showModalDialog also runs a nested event loop and likely could be used to trigger this condition.

[Abhishek Arya]

(In reply to comment #9)
> We're going to want to add some ASSERTs to catch reentrancy here, as we thought we had avoided any reentrancy possibly with the threaded parser design.  If we're hitting this in cases other than the inspector, we could be seeing badness.  As far as we know this test only ASSERTs in Debug.  I suspect the output changes slightly in Release, but there should be no crashes there.

If you use our new ASSERT_WITH_SECURITY_IMPLICATION, we can catch that badness on our fuzzing bots with a reproducible testcase :)

[Eric Seidel (no email)]

Created attachment 195659 [details]
Patch

[Eric Seidel (no email)]

Created attachment 195661 [details]
Patch

[Eric Seidel (no email)]

Created attachment 195662 [details]
Patch

[Eric Seidel (no email)]

Created attachment 195664 [details]
Patch

[Adam Barth]

Comment on attachment 195664 [details]
Patch

I've asked Eric to try another approach where we always push the current chunk onto the speculation queue and call pumpPendingSpeculations.l

[Adam Barth]

Created attachment 195927 [details]
Example patch

[Adam Barth]

Comment on attachment 195927 [details]
Example patch

This is the approach I had in mind.  I'm not sure whether it works, however.  :)

[Adam Barth]

Looks like it doesn't quite work because pumpPendingSpeculations takes the chunk out of m_speculations before processing it...

[Eric Seidel (no email)]

i think you should review my original patch to fix the sec bug, then we should iterate :)

[Adam Barth]

Comment on attachment 195664 [details]
Patch

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

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:320
> +    bool calledFromNestedEventLoop = (document()->activeParserCount() > 0);

The problem with using activeParserCount is that you'll get cross-talk from other parsers associated with the current document.  Maybe that's ok because only the first parser can be a threaded parser (since all other parsers will be script-created)?

It seems better to use state that's associated with this this specific parser rather than using state associated wit the document.

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:346
> +    if (!m_speculations.isEmpty() && !isWaitingForScripts() && !isStopped())
> +        pumpPendingSpeculations();

This is just duplicating logic from pumpPendingSpeculations.  IMHO, we should call pumpPendingSpeculations instead of processParsedChunkFromBackgroundParser so that we measure the parserTimeLimit accurately.

[Eric Seidel (no email)]

(In reply to comment #21)
> (From update of attachment 195664 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=195664&action=review
> 
> > Source/WebCore/html/parser/HTMLDocumentParser.cpp:320
> > +    bool calledFromNestedEventLoop = (document()->activeParserCount() > 0);
> 
> The problem with using activeParserCount is that you'll get cross-talk from other parsers associated with the current document.  Maybe that's ok because only the first parser can be a threaded parser (since all other parsers will be script-created)?

correct.  the method which matters is always the bottom of the stack.

> It seems better to use state that's associated with this this specific parser rather than using state associated wit the document.

that's fine too.

> > Source/WebCore/html/parser/HTMLDocumentParser.cpp:346
> > +    if (!m_speculations.isEmpty() && !isWaitingForScripts() && !isStopped())
> > +        pumpPendingSpeculations();
> 
> This is just duplicating logic from pumpPendingSpeculations.  IMHO, we should call pumpPendingSpeculations instead of processParsedChunkFromBackgroundParser so that we measure the parserTimeLimit accurately.

that's fine.  we can discuss tomorrow.

[Adam Barth]

Created attachment 196029 [details]
more work in progress

[Adam Barth]

Created attachment 196030 [details]
Patch

[WebKit Review Bot]

Comment on attachment 196030 [details]
Patch

Clearing flags on attachment: 196030

Committed r147383: <http://trac.webkit.org/changeset/147383>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Zan Dobersek]

This patch is the prime candidate as a cause for regressions on a much limited set of builders:
http://build.webkit.org/builders/Apple%20MountainLion%20Debug%20WK1%20%28Tests%29/builds/6855
http://build.webkit.org/builders/Apple%20Lion%20Debug%20WK1%20%28Tests%29/builds/7837
http://build.webkit.org/builders/Apple%20Win%207%20Release%20%28Tests%29/builds/34376
http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release/builds/11411
http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Release/builds/36312

The minimal revision range consists of commits 147383 and 147384:
http://trac.webkit.org/log/?verbose=on&rev=147384&stop_rev=147383

I'll consult other port maintainers and try to delay the rollout until I can confirm what patch caused this, but will reland any potential rollout of this patch if it turns out it wasn't the one to blame.

[Zan Dobersek]

(In reply to comment #27)
> http://build.webkit.org/builders/Apple%20Win%207%20Release%20%28Tests%29/builds/34376

Rather http://build.webkit.org/builders/Apple%20Win%207%20Release%20%28Tests%29/builds/34377

[WebKit Review Bot]

Re-opened since this is blocked by bug 113765

[Eric Seidel (no email)]

The consensus is this was the correct patch to blame?

[Zan Dobersek]

(In reply to comment #30)
> The consensus is this was the correct patch to blame?

Yes, reverting this patch fixed these problems:
http://build.webkit.org/builders/Apple%20MountainLion%20Debug%20WK1%20%28Tests%29/builds/6865
http://build.webkit.org/builders/Apple%20MountainLion%20Debug%20WK1%20%28Tests%29/builds/6865
http://build.webkit.org/builders/Apple%20Win%207%20Release%20%28Tests%29/builds/34390
http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Release/builds/11416

[Brian Burg]

The related test(s) have been removed from trunk.