Changeset 147383 in webkit

Timestamp:

Apr 1, 2013 10:34:38 PM (11 years ago)

Author:

abarth@webkit.org

Message:

Assertion failure !m_lastChunkBeforeScript in HTMLDocumentParser during inspector/debugger/pause-in-inline-script.html
​https://bugs.webkit.org/show_bug.cgi?id=112369

Reviewed by Eric Seidel.

Source/WebCore:

The threaded HTML parser wasn't correctly handling the nested event
loops that can arise from the JavaScript debugger and from
showModalDialog. When the parser received a chunk from the background
parser, it was always processing it immediately, which lead to
re-entrancy. Now, we'll queue the chunk in the speculation buffer and
process it once the stack unwinds.

• html/parser/HTMLDocumentParser.cpp:

(WebCore::HTMLDocumentParser::~HTMLDocumentParser):
(WebCore::HTMLDocumentParser::didReceiveParsedChunkFromBackgroundParser):
(WebCore::HTMLDocumentParser::processParsedChunkFromBackgroundParser):
(WebCore::HTMLDocumentParser::pumpPendingSpeculations):
(WebCore::HTMLDocumentParser::insert):

• html/parser/HTMLParserScheduler.cpp:

(WebCore::PumpSession::PumpSession):

• html/parser/HTMLParserScheduler.h:

LayoutTests:

Unskip test that is now passing.

• platform/chromium/TestExpectations:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/platform/chromium/TestExpectations (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/html/parser/HTMLDocumentParser.cpp (modified) (9 diffs)
• Source/WebCore/html/parser/HTMLParserScheduler.cpp (modified) (1 diff)
• Source/WebCore/html/parser/HTMLParserScheduler.h (modified) (1 diff)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2013-04-01  Adam Barth  <abarth@webkit.org>
+
+        Assertion failure !m_lastChunkBeforeScript in HTMLDocumentParser during inspector/debugger/pause-in-inline-script.html
+        https://bugs.webkit.org/show_bug.cgi?id=112369
+
+        Reviewed by Eric Seidel.
+
+        Unskip test that is now passing.
+
+        * platform/chromium/TestExpectations:
+
 2013-04-01  Michael Pruett  <michael@68k.org>
 

trunk/LayoutTests/platform/chromium/TestExpectations

@@ -3784 +3784 @@
 webkit.org/b/91611 media/media-higher-prio-audio-stream.html [ Skip ]
 
-# Flaky assertion failures on debug bots
-webkit.org/b/112369 [ Debug ] inspector/debugger/pause-in-inline-script.html [ Pass Crash Timeout ]
-
 # Needs rebaseline on all platforms
 Bug(leviw) fast/text/shaping/shaping-selection-rect.html [ Failure ]

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2013-04-01  Adam Barth  <abarth@webkit.org>
+
+        Assertion failure !m_lastChunkBeforeScript in HTMLDocumentParser during inspector/debugger/pause-in-inline-script.html
+        https://bugs.webkit.org/show_bug.cgi?id=112369
+
+        Reviewed by Eric Seidel.
+
+        The threaded HTML parser wasn't correctly handling the nested event
+        loops that can arise from the JavaScript debugger and from
+        showModalDialog. When the parser received a chunk from the background
+        parser, it was always processing it immediately, which lead to
+        re-entrancy. Now, we'll queue the chunk in the speculation buffer and
+        process it once the stack unwinds.
+
+        * html/parser/HTMLDocumentParser.cpp:
+        (WebCore::HTMLDocumentParser::~HTMLDocumentParser):
+        (WebCore::HTMLDocumentParser::didReceiveParsedChunkFromBackgroundParser):
+        (WebCore::HTMLDocumentParser::processParsedChunkFromBackgroundParser):
+        (WebCore::HTMLDocumentParser::pumpPendingSpeculations):
+        (WebCore::HTMLDocumentParser::insert):
+        * html/parser/HTMLParserScheduler.cpp:
+        (WebCore::PumpSession::PumpSession):
+        * html/parser/HTMLParserScheduler.h:
+
 2013-04-01  Michael Pruett  <michael@68k.org>
 

trunk/Source/WebCore/html/parser/HTMLDocumentParser.cpp

@@ -128 +128 @@
     ASSERT(!m_insertionPreloadScanner);
     ASSERT(!m_haveBackgroundParser);
+    // FIXME: We should be able to ASSERT(m_speculations.isEmpty()),
+    // but there are cases where that's not true currently. For example,
+    // we we're told to stop parsing before we've consumed all the input.
 }
 
@@ -312 +315 @@
 void HTMLDocumentParser::didReceiveParsedChunkFromBackgroundParser(PassOwnPtr<ParsedChunk> chunk)
 {
-    if (isWaitingForScripts() || !m_speculations.isEmpty()) {
+    // alert(), runModalDialog, and the JavaScript Debugger all run nested event loops
+    // which can cause this method to be re-entered. We detect re-entry using
+    // inPumpSession(), save the chunk as a speculation, and return.
+    if (isWaitingForScripts() || !m_speculations.isEmpty() || inPumpSession()) {
         m_preloader->takeAndPreload(chunk->preloads);
         m_speculations.append(chunk);
@@ -322 +328 @@
     RefPtr<HTMLDocumentParser> protect(this);
 
-    InspectorInstrumentationCookie cookie = InspectorInstrumentation::willWriteHTML(document(), lineNumber().zeroBasedInt());
-
     ASSERT(m_speculations.isEmpty());
     chunk->preloads.clear(); // We don't need to preload because we're going to parse immediately.
-    processParsedChunkFromBackgroundParser(chunk);
-
-    InspectorInstrumentation::didWriteHTML(cookie, lineNumber().zeroBasedInt());
+    m_speculations.append(chunk);
+    pumpPendingSpeculations();
 }
 
@@ -391 +394 @@
 void HTMLDocumentParser::processParsedChunkFromBackgroundParser(PassOwnPtr<ParsedChunk> popChunk)
 {
+    ASSERT_WITH_SECURITY_IMPLICATION(!inPumpSession());
+    ASSERT(!isParsingFragment());
+    ASSERT(!isWaitingForScripts());
+    ASSERT(!isStopped());
     // ASSERT that this object is both attached to the Document and protected.
     ASSERT(refCount() >= 2);
@@ -398 +405 @@
     ASSERT(!m_lastChunkBeforeScript);
 
-    ActiveParserSession session(contextForParsingSession());
+    PumpSession session(m_pumpSessionNestingLevel, contextForParsingSession());
+
     OwnPtr<ParsedChunk> chunk(popChunk);
     OwnPtr<CompactHTMLTokenStream> tokens = chunk->tokens.release();
@@ -427 +435 @@
             // we peek to see if this chunk has an EOF and process it anyway.
             if (tokens->last().type() == HTMLToken::EndOfFile) {
-                ASSERT(m_speculations.isEmpty());
+                ASSERT(m_speculations.isEmpty()); // There should never be any chunks after the EOF.
                 prepareToStopParsing();
             }
@@ -442 +450 @@
         if (it->type() == HTMLToken::EndOfFile) {
             ASSERT(it + 1 == tokens->end()); // The EOF is assumed to be the last token of this bunch.
-            ASSERT(m_speculations.isEmpty());
+            ASSERT(m_speculations.isEmpty()); // There should never be any chunks after the EOF.
             prepareToStopParsing();
             break;
@@ -464 +472 @@
     ASSERT(!m_token);
     ASSERT(!m_lastChunkBeforeScript);
+    ASSERT(!isWaitingForScripts());
+    ASSERT(!isStopped());
 
     // FIXME: Pass in current input length.
@@ -630 +640 @@
 #if ENABLE(THREADED_HTML_PARSER)
     if (!m_tokenizer) {
-        ASSERT(!inPumpSession());
         ASSERT(m_haveBackgroundParser || wasCreatedByScript());
         m_token = adoptPtr(new HTMLToken);

trunk/Source/WebCore/html/parser/HTMLParserScheduler.cpp

@@ -78 +78 @@
 
 PumpSession::PumpSession(unsigned& nestingLevel, Document* document)
-    : NestingLevelIncrementer(nestingLevel)
-    , ActiveParserSession(document)
+    : ActiveParserSession(document)
+    , NestingLevelIncrementer(nestingLevel)
     // Setting processedTokens to INT_MAX causes us to check for yields
     // after any token during any parse where yielding is allowed.

trunk/Source/WebCore/html/parser/HTMLParserScheduler.h

@@ -48 +48 @@
 };
 
-class PumpSession : public NestingLevelIncrementer, public ActiveParserSession {
+// In C++, base classes are destructed from right to left, which means
+// ~NestingLevelIncrementer will run before ~ActiveParserSession. This
+// is important because ~ActiveParserSession calls Document::decrementActiveParserCount,
+// which cares about the pump nesting level of the parser.
+class PumpSession : public ActiveParserSession, public NestingLevelIncrementer {
 public:
     PumpSession(unsigned& nestingLevel, Document*);