Summary: | Assertion failure !m_lastChunkBeforeScript in HTMLDocumentParser during inspector/debugger/pause-in-inline-script.html | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kenneth Russell <kbr> | ||||||||||||||||||
Component: | DOM | Assignee: | 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
Kenneth Russell
2013-03-14 11:22:01 PDT
This is likely from Eric's recent patch. Thanks. I'll make sure to fix this on Monday if not before. 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 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. Have we seen this since? I've not had any other reports of this. 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. 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. Created attachment 194006 [details]
backtrace of the bug
Turns out the debugger can cause us to re-enter didReceiveParsedChunkFromBackgroundParser on the main thread. :(
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. Adam points out that window.showModalDialog also runs a nested event loop and likely could be used to trigger this condition. (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 :) Created attachment 195659 [details]
Patch
Created attachment 195661 [details]
Patch
Created attachment 195662 [details]
Patch
Created attachment 195664 [details]
Patch
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
Created attachment 195927 [details]
Example patch
Comment on attachment 195927 [details]
Example patch
This is the approach I had in mind. I'm not sure whether it works, however. :)
Looks like it doesn't quite work because pumpPendingSpeculations takes the chunk out of m_speculations before processing it... i think you should review my original patch to fix the sec bug, then we should iterate :) 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. (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. Created attachment 196029 [details]
more work in progress
Created attachment 196030 [details]
Patch
Comment on attachment 196030 [details] Patch Clearing flags on attachment: 196030 Committed r147383: <http://trac.webkit.org/changeset/147383> All reviewed patches have been landed. Closing bug. 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. (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 Re-opened since this is blocked by bug 113765 The consensus is this was the correct patch to blame? (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 The related test(s) have been removed from trunk. |