- Revision
- 197040
- Author
- rn...@webkit.org
- Date
- 2016-02-24 12:26:01 -0800 (Wed, 24 Feb 2016)
Log Message
A function named canTakeNextToken executing blocking scripts is misleading
https://bugs.webkit.org/show_bug.cgi?id=154636
Reviewed by Darin Adler.
Merged canTakeNextToken into pumpTokenizer and extracted pumpTokenizerLoop out of pumpTokenizer.
Inlined m_parserChunkSize in HTMLParserScheduler into checkForYieldBeforeToken, and removed needsYield
from PumpSession in favor of making checkForYieldBeforeToken and checkForYieldBeforeScript return a bool.
No new tests since this is a pure refactoring.
* html/parser/HTMLDocumentParser.cpp:
(WebCore::HTMLDocumentParser::canTakeNextToken): Deleted.
(WebCore::HTMLDocumentParser::pumpTokenizerLoop): Extracted from pumpTokenizer. We don't have to check
isStopped() at the beginning since pumpTokenizer asserts that. Return true when session.needsYield would
have been set to true in the old code and return false elsewhere (for stopping or incomplete token).
(WebCore::HTMLDocumentParser::pumpTokenizer):
* html/parser/HTMLDocumentParser.h:
* html/parser/HTMLParserScheduler.cpp:
(WebCore::PumpSession::PumpSession):
(WebCore::HTMLParserScheduler::HTMLParserScheduler):
(WebCore::HTMLParserScheduler::shouldYieldBeforeExecutingScript): Renamed from checkForYieldBeforeScript.
* html/parser/HTMLParserScheduler.h:
(WebCore::HTMLParserScheduler::shouldYieldBeforeToken): Renamed from checkForYieldBeforeToken.
(WebCore::HTMLParserScheduler::isScheduledForResume):
(WebCore::HTMLParserScheduler::checkForYield): Extracted from checkForYieldBeforeToken. Reset
processedTokens to 1 instead of setting it to 0 here and incrementing it later as done in the old code.
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (197039 => 197040)
--- trunk/Source/WebCore/ChangeLog 2016-02-24 20:19:47 UTC (rev 197039)
+++ trunk/Source/WebCore/ChangeLog 2016-02-24 20:26:01 UTC (rev 197040)
@@ -1,3 +1,34 @@
+2016-02-24 Ryosuke Niwa <rn...@webkit.org>
+
+ A function named canTakeNextToken executing blocking scripts is misleading
+ https://bugs.webkit.org/show_bug.cgi?id=154636
+
+ Reviewed by Darin Adler.
+
+ Merged canTakeNextToken into pumpTokenizer and extracted pumpTokenizerLoop out of pumpTokenizer.
+
+ Inlined m_parserChunkSize in HTMLParserScheduler into checkForYieldBeforeToken, and removed needsYield
+ from PumpSession in favor of making checkForYieldBeforeToken and checkForYieldBeforeScript return a bool.
+
+ No new tests since this is a pure refactoring.
+
+ * html/parser/HTMLDocumentParser.cpp:
+ (WebCore::HTMLDocumentParser::canTakeNextToken): Deleted.
+ (WebCore::HTMLDocumentParser::pumpTokenizerLoop): Extracted from pumpTokenizer. We don't have to check
+ isStopped() at the beginning since pumpTokenizer asserts that. Return true when session.needsYield would
+ have been set to true in the old code and return false elsewhere (for stopping or incomplete token).
+ (WebCore::HTMLDocumentParser::pumpTokenizer):
+ * html/parser/HTMLDocumentParser.h:
+ * html/parser/HTMLParserScheduler.cpp:
+ (WebCore::PumpSession::PumpSession):
+ (WebCore::HTMLParserScheduler::HTMLParserScheduler):
+ (WebCore::HTMLParserScheduler::shouldYieldBeforeExecutingScript): Renamed from checkForYieldBeforeScript.
+ * html/parser/HTMLParserScheduler.h:
+ (WebCore::HTMLParserScheduler::shouldYieldBeforeToken): Renamed from checkForYieldBeforeToken.
+ (WebCore::HTMLParserScheduler::isScheduledForResume):
+ (WebCore::HTMLParserScheduler::checkForYield): Extracted from checkForYieldBeforeToken. Reset
+ processedTokens to 1 instead of setting it to 0 here and incrementing it later as done in the old code.
+
2016-02-24 Daniel Bates <daba...@apple.com>
CSP: Enable plugin-types directive by default
Modified: trunk/Source/WebCore/html/parser/HTMLDocumentParser.cpp (197039 => 197040)
--- trunk/Source/WebCore/html/parser/HTMLDocumentParser.cpp 2016-02-24 20:19:47 UTC (rev 197039)
+++ trunk/Source/WebCore/html/parser/HTMLDocumentParser.cpp 2016-02-24 20:26:01 UTC (rev 197040)
@@ -196,38 +196,6 @@
}
}
-bool HTMLDocumentParser::canTakeNextToken(SynchronousMode mode, PumpSession& session)
-{
- if (isStopped())
- return false;
-
- if (isWaitingForScripts()) {
- if (mode == AllowYield)
- m_parserScheduler->checkForYieldBeforeScript(session);
-
- // If we don't run the script, we cannot allow the next token to be taken.
- if (session.needsYield)
- return false;
-
- // If we're paused waiting for a script, we try to execute scripts before continuing.
- runScriptsForPausedTreeBuilder();
- if (isWaitingForScripts() || isStopped())
- return false;
- }
-
- // FIXME: It's wrong for the HTMLDocumentParser to reach back to the Frame, but this approach is
- // how the parser has always handled stopping when the page assigns window.location. What should
- // happen instead is that assigning window.location causes the parser to stop parsing cleanly.
- // The problem is we're not prepared to do that at every point where we run _javascript_.
- if (!isParsingFragment() && document()->frame() && document()->frame()->navigationScheduler().locationChangePending())
- return false;
-
- if (mode == AllowYield)
- m_parserScheduler->checkForYieldBeforeToken(session);
-
- return true;
-}
-
Document* HTMLDocumentParser::contextForParsingSession()
{
// The parsing session should interact with the document only when parsing
@@ -237,27 +205,36 @@
return document();
}
-void HTMLDocumentParser::pumpTokenizer(SynchronousMode mode)
+bool HTMLDocumentParser::pumpTokenizerLoop(SynchronousMode mode, bool parsingFragment, PumpSession& session)
{
- ASSERT(!isStopped());
- ASSERT(!isScheduledForResume());
+ do {
+ if (UNLIKELY(isWaitingForScripts())) {
+ if (mode == AllowYield && m_parserScheduler->shouldYieldBeforeExecutingScript(session))
+ return true;
+ runScriptsForPausedTreeBuilder();
+ // If we're paused waiting for a script, we try to execute scripts before continuing.
+ if (isWaitingForScripts() || isStopped())
+ return false;
+ }
- // This is an attempt to check that this object is both attached to the Document and protected by something.
- ASSERT(refCount() >= 2);
+ // FIXME: It's wrong for the HTMLDocumentParser to reach back to the Frame, but this approach is
+ // how the parser has always handled stopping when the page assigns window.location. What should
+ // happen instead is that assigning window.location causes the parser to stop parsing cleanly.
+ // The problem is we're not prepared to do that at every point where we run _javascript_.
+ if (UNLIKELY(!parsingFragment && document()->frame() && document()->frame()->navigationScheduler().locationChangePending()))
+ return false;
- PumpSession session(m_pumpSessionNestingLevel, contextForParsingSession());
+ if (UNLIKELY(mode == AllowYield && m_parserScheduler->shouldYieldBeforeToken(session)))
+ return true;
- m_xssAuditor.init(document(), &m_xssAuditorDelegate);
-
- while (canTakeNextToken(mode, session) && !session.needsYield) {
- if (!isParsingFragment())
+ if (!parsingFragment)
m_sourceTracker.startToken(m_input.current(), m_tokenizer);
auto token = m_tokenizer.nextToken(m_input.current());
if (!token)
- break;
+ return false;
- if (!isParsingFragment()) {
+ if (!parsingFragment) {
m_sourceTracker.endToken(m_input.current(), m_tokenizer);
// We do not XSS filter innerHTML, which means we (intentionally) fail
@@ -267,8 +244,25 @@
}
constructTreeFromHTMLToken(token);
- }
+ } while (!isStopped());
+ return false;
+}
+
+void HTMLDocumentParser::pumpTokenizer(SynchronousMode mode)
+{
+ ASSERT(!isStopped());
+ ASSERT(!isScheduledForResume());
+
+ // This is an attempt to check that this object is both attached to the Document and protected by something.
+ ASSERT(refCount() >= 2);
+
+ PumpSession session(m_pumpSessionNestingLevel, contextForParsingSession());
+
+ m_xssAuditor.init(document(), &m_xssAuditorDelegate);
+
+ bool shouldResume = pumpTokenizerLoop(mode, isParsingFragment(), session);
+
// Ensure we haven't been totally deref'ed after pumping. Any caller of this
// function should be holding a RefPtr to this to ensure we weren't deleted.
ASSERT(refCount() >= 1);
@@ -276,7 +270,7 @@
if (isStopped())
return;
- if (session.needsYield)
+ if (shouldResume)
m_parserScheduler->scheduleForResume();
if (isWaitingForScripts()) {
Modified: trunk/Source/WebCore/html/parser/HTMLDocumentParser.h (197039 => 197040)
--- trunk/Source/WebCore/html/parser/HTMLDocumentParser.h 2016-02-24 20:19:47 UTC (rev 197039)
+++ trunk/Source/WebCore/html/parser/HTMLDocumentParser.h 2016-02-24 20:26:01 UTC (rev 197040)
@@ -102,8 +102,8 @@
Document* contextForParsingSession();
enum SynchronousMode { AllowYield, ForceSynchronous };
- bool canTakeNextToken(SynchronousMode, PumpSession&);
void pumpTokenizer(SynchronousMode);
+ bool pumpTokenizerLoop(SynchronousMode, bool parsingFragment, PumpSession&);
void pumpTokenizerIfPossible(SynchronousMode);
void constructTreeFromHTMLToken(HTMLTokenizer::TokenPtr&);
Modified: trunk/Source/WebCore/html/parser/HTMLParserScheduler.cpp (197039 => 197040)
--- trunk/Source/WebCore/html/parser/HTMLParserScheduler.cpp 2016-02-24 20:19:47 UTC (rev 197039)
+++ trunk/Source/WebCore/html/parser/HTMLParserScheduler.cpp 2016-02-24 20:26:01 UTC (rev 197040)
@@ -32,11 +32,6 @@
#include "HTMLDocumentParser.h"
#include "Page.h"
-// defaultParserChunkSize is used to define how many tokens the parser will
-// process before checking against parserTimeLimit and possibly yielding.
-// This is a performance optimization to prevent checking after every token.
-static const int defaultParserChunkSize = 4096;
-
// defaultParserTimeLimit is the seconds the parser will run in one write() call
// before yielding. Inline <script> execution can cause it to exceed the limit.
// FIXME: We would like this value to be 0.2.
@@ -75,7 +70,6 @@
// At that time we'll initialize startTime.
, processedTokens(INT_MAX)
, startTime(0)
- , needsYield(false)
, didSeeScript(false)
{
}
@@ -87,7 +81,6 @@
HTMLParserScheduler::HTMLParserScheduler(HTMLDocumentParser& parser)
: m_parser(parser)
, m_parserTimeLimit(parserTimeLimit(m_parser.document()->page()))
- , m_parserChunkSize(defaultParserChunkSize)
, m_continueNextChunkTimer(*this, &HTMLParserScheduler::continueNextChunkTimerFired)
, m_isSuspendedWithActiveTimer(false)
#if !ASSERT_DISABLED
@@ -114,15 +107,14 @@
m_parser.resumeParsingAfterYield();
}
-void HTMLParserScheduler::checkForYieldBeforeScript(PumpSession& session)
+bool HTMLParserScheduler::shouldYieldBeforeExecutingScript(PumpSession& session)
{
// If we've never painted before and a layout is pending, yield prior to running
// scripts to give the page a chance to paint earlier.
Document* document = m_parser.document();
bool needsFirstPaint = document->view() && !document->view()->hasEverPainted();
- if (needsFirstPaint && document->isLayoutTimerActive())
- session.needsYield = true;
session.didSeeScript = true;
+ return needsFirstPaint && document->isLayoutTimerActive();
}
void HTMLParserScheduler::scheduleForResume()
Modified: trunk/Source/WebCore/html/parser/HTMLParserScheduler.h (197039 => 197040)
--- trunk/Source/WebCore/html/parser/HTMLParserScheduler.h 2016-02-24 20:19:47 UTC (rev 197039)
+++ trunk/Source/WebCore/html/parser/HTMLParserScheduler.h 2016-02-24 20:26:01 UTC (rev 197040)
@@ -54,9 +54,8 @@
PumpSession(unsigned& nestingLevel, Document*);
~PumpSession();
- int processedTokens;
+ unsigned processedTokens;
double startTime;
- bool needsYield;
bool didSeeScript;
};
@@ -66,29 +65,19 @@
explicit HTMLParserScheduler(HTMLDocumentParser&);
~HTMLParserScheduler();
- // Inline as this is called after every token in the parser.
- void checkForYieldBeforeToken(PumpSession& session)
+ bool shouldYieldBeforeToken(PumpSession& session)
{
#if PLATFORM(IOS)
if (WebThreadShouldYield())
- session.needsYield = true;
+ return true;
#endif
- if (session.processedTokens > m_parserChunkSize || session.didSeeScript) {
- // monotonicallyIncreasingTime() can be expensive. By delaying, we avoided calling
- // monotonicallyIncreasingTime() when constructing non-yielding PumpSessions.
- if (!session.startTime)
- session.startTime = monotonicallyIncreasingTime();
+ if (UNLIKELY(session.processedTokens > numberOfTokensBeforeCheckingForYield || session.didSeeScript))
+ return checkForYield(session);
- session.processedTokens = 0;
- session.didSeeScript = false;
-
- double elapsedTime = monotonicallyIncreasingTime() - session.startTime;
- if (elapsedTime > m_parserTimeLimit)
- session.needsYield = true;
- }
++session.processedTokens;
+ return false;
}
- void checkForYieldBeforeScript(PumpSession&);
+ bool shouldYieldBeforeExecutingScript(PumpSession&);
void scheduleForResume();
bool isScheduledForResume() const { return m_isSuspendedWithActiveTimer || m_continueNextChunkTimer.isActive(); }
@@ -97,12 +86,29 @@
void resume();
private:
+ static const unsigned numberOfTokensBeforeCheckingForYield = 4096; // Performance optimization
+
void continueNextChunkTimerFired();
+ bool checkForYield(PumpSession& session)
+ {
+ session.processedTokens = 1;
+ session.didSeeScript = false;
+
+ // monotonicallyIncreasingTime() can be expensive. By delaying, we avoided calling
+ // monotonicallyIncreasingTime() when constructing non-yielding PumpSessions.
+ if (!session.startTime) {
+ session.startTime = monotonicallyIncreasingTime();
+ return false;
+ }
+
+ double elapsedTime = monotonicallyIncreasingTime() - session.startTime;
+ return elapsedTime > m_parserTimeLimit;
+ }
+
HTMLDocumentParser& m_parser;
double m_parserTimeLimit;
- int m_parserChunkSize;
Timer m_continueNextChunkTimer;
bool m_isSuspendedWithActiveTimer;
#if !ASSERT_DISABLED
