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
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.