Bug 112369

Summary: Assertion failure !m_lastChunkBeforeScript in HTMLDocumentParser during inspector/debugger/pause-in-inline-script.html
Product: WebKit Reporter: Kenneth Russell <kbr>
Component: DOMAssignee: Eric Seidel (no email) <eric>
Status: RESOLVED INVALID    
Severity: Normal CC: abarth, eric, esprehn+autocc, inferno, ojan.autocc, pfeldman, tonyg, webkit.review.bot, zan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 113765    
Bug Blocks:    
Attachments:
Description Flags
backtrace of the bug
none
Patch
none
Patch
none
Patch
none
Patch
abarth: review-
Example patch
none
more work in progress
none
Patch none

Description Kenneth Russell 2013-03-14 11:22:01 PDT
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
Comment 1 Adam Barth 2013-03-15 12:29:06 PDT
This is likely from Eric's recent patch.
Comment 2 Eric Seidel (no email) 2013-03-15 12:45:11 PDT
Thanks.  I'll make sure to fix this on Monday if not before.
Comment 3 Eric Seidel (no email) 2013-03-15 13:23:50 PDT
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
Comment 4 Eric Seidel (no email) 2013-03-17 00:09:17 PDT
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.
Comment 5 Eric Seidel (no email) 2013-03-19 13:51:22 PDT
Have we seen this since?  I've not had any other reports of this.
Comment 6 Kenneth Russell 2013-03-19 17:33:59 PDT
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.
Comment 7 Eric Seidel (no email) 2013-03-20 01:31:03 PDT
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.
Comment 8 Eric Seidel (no email) 2013-03-20 02:31:40 PDT
Created attachment 194006 [details]
backtrace of the bug

Turns out the debugger can cause us to re-enter didReceiveParsedChunkFromBackgroundParser on the main thread. :(
Comment 9 Eric Seidel (no email) 2013-03-20 02:57:16 PDT
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.
Comment 10 Eric Seidel (no email) 2013-03-20 11:41:54 PDT
Adam points out that window.showModalDialog also runs a nested event loop and likely could be used to trigger this condition.
Comment 11 Abhishek Arya 2013-03-21 08:42:13 PDT
(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 :)
Comment 12 Eric Seidel (no email) 2013-03-28 15:36:55 PDT
Created attachment 195659 [details]
Patch
Comment 13 Eric Seidel (no email) 2013-03-28 15:39:06 PDT
Created attachment 195661 [details]
Patch
Comment 14 Eric Seidel (no email) 2013-03-28 15:44:06 PDT
Created attachment 195662 [details]
Patch
Comment 15 Eric Seidel (no email) 2013-03-28 15:53:47 PDT
Created attachment 195664 [details]
Patch
Comment 16 Adam Barth 2013-03-30 10:59:28 PDT
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
Comment 17 Adam Barth 2013-03-31 23:36:27 PDT
Created attachment 195927 [details]
Example patch
Comment 18 Adam Barth 2013-03-31 23:37:20 PDT
Comment on attachment 195927 [details]
Example patch

This is the approach I had in mind.  I'm not sure whether it works, however.  :)
Comment 19 Adam Barth 2013-03-31 23:39:00 PDT
Looks like it doesn't quite work because pumpPendingSpeculations takes the chunk out of m_speculations before processing it...
Comment 20 Eric Seidel (no email) 2013-03-31 23:44:00 PDT
i think you should review my original patch to fix the sec bug, then we should iterate :)
Comment 21 Adam Barth 2013-03-31 23:53:10 PDT
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.
Comment 22 Eric Seidel (no email) 2013-03-31 23:58:20 PDT
(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.
Comment 23 Adam Barth 2013-04-01 15:24:46 PDT
Created attachment 196029 [details]
more work in progress
Comment 24 Adam Barth 2013-04-01 15:48:20 PDT
Created attachment 196030 [details]
Patch
Comment 25 WebKit Review Bot 2013-04-01 22:33:01 PDT
Comment on attachment 196030 [details]
Patch

Clearing flags on attachment: 196030

Committed r147383: <http://trac.webkit.org/changeset/147383>
Comment 26 WebKit Review Bot 2013-04-01 22:33:06 PDT
All reviewed patches have been landed.  Closing bug.
Comment 27 Zan Dobersek 2013-04-02 02:06:23 PDT
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.
Comment 29 WebKit Review Bot 2013-04-02 03:11:33 PDT
Re-opened since this is blocked by bug 113765
Comment 30 Eric Seidel (no email) 2013-04-02 11:02:04 PDT
The consensus is this was the correct patch to blame?
Comment 32 Brian Burg 2014-12-17 17:48:18 PST
The related test(s) have been removed from trunk.