<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[153407] trunk/Source/WebCore</title>
</head>
<body>

<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt;  }
#msg dl a { font-weight: bold}
#msg dl a:link    { color:#fc3; }
#msg dl a:active  { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff  {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/153407">153407</a></dd>
<dt>Author</dt> <dd>ap@apple.com</dd>
<dt>Date</dt> <dd>2013-07-27 17:05:11 -0700 (Sat, 27 Jul 2013)</dd>
</dl>

<h3>Log Message</h3>
<pre>HTMLParserScheduler gets into an inconsistent state when suspended for reasons
other than WillDeferLoading
https://bugs.webkit.org/show_bug.cgi?id=119172

Reviewed by Sam Weinig.

When loading is not deferred, even a suspended parser will be processing new data
from network, potentially starting its next chunk timer.

Limit suspending to when we can actually enforce it.

Here is what happens for each ReasonForSuspension:
- JavaScriptDebuggerPaused: continuing to parse is probably wrong, but in practice,
this is unlikely to happen while debugging, and wasn't properly prevented before
this patch anyway.
- WillDeferLoading: No change in behavior.
- DocumentWillBecomeInactive: This is about page cache, and documents are only allowed
to be cached when fully loaded.
- PageWillBeSuspended: This appears to be part of Frame::suspendActiveDOMObjectsAndAnimations()
implementation, I'm guessing that it is appropriate to continue loading.

* dom/Document.cpp:
(WebCore::Document::suspendScheduledTasks):
(WebCore::Document::resumeScheduledTasks):
Only suspend/resume parsing when loading is deferred. This is not expressed directly,
but it's important to do this to avoid executing JS behind alerts and other modal dialogs.

* html/parser/HTMLParserScheduler.h: Added m_suspended member variable for assertions.

* html/parser/HTMLParserScheduler.cpp:
(WebCore::HTMLParserScheduler::HTMLParserScheduler):
(WebCore::HTMLParserScheduler::continueNextChunkTimerFired):
(WebCore::HTMLParserScheduler::scheduleForResume):
(WebCore::HTMLParserScheduler::suspend):
(WebCore::HTMLParserScheduler::resume):
Update m_suspended and assert as appropriate. No behavior changes for release mode.

* page/Frame.cpp: (WebCore::Frame::suspendActiveDOMObjectsAndAnimations):
Added a FIXME.</pre>

<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceWebCoreChangeLog">trunk/Source/WebCore/ChangeLog</a></li>
<li><a href="#trunkSourceWebCoredomDocumentcpp">trunk/Source/WebCore/dom/Document.cpp</a></li>
<li><a href="#trunkSourceWebCorehtmlparserHTMLParserSchedulercpp">trunk/Source/WebCore/html/parser/HTMLParserScheduler.cpp</a></li>
<li><a href="#trunkSourceWebCorehtmlparserHTMLParserSchedulerh">trunk/Source/WebCore/html/parser/HTMLParserScheduler.h</a></li>
<li><a href="#trunkSourceWebCorepageFramecpp">trunk/Source/WebCore/page/Frame.cpp</a></li>
</ul>

</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceWebCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/ChangeLog (153406 => 153407)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/ChangeLog        2013-07-28 00:04:14 UTC (rev 153406)
+++ trunk/Source/WebCore/ChangeLog        2013-07-28 00:05:11 UTC (rev 153407)
</span><span class="lines">@@ -1,5 +1,47 @@
</span><span class="cx"> 2013-07-27  Alexey Proskuryakov  &lt;ap@apple.com&gt;
</span><span class="cx"> 
</span><ins>+        HTMLParserScheduler gets into an inconsistent state when suspended for reasons
+        other than WillDeferLoading
+        https://bugs.webkit.org/show_bug.cgi?id=119172
+
+        Reviewed by Sam Weinig.
+
+        When loading is not deferred, even a suspended parser will be processing new data
+        from network, potentially starting its next chunk timer.
+
+        Limit suspending to when we can actually enforce it.
+
+        Here is what happens for each ReasonForSuspension:
+        - JavaScriptDebuggerPaused: continuing to parse is probably wrong, but in practice,
+        this is unlikely to happen while debugging, and wasn't properly prevented before
+        this patch anyway.
+        - WillDeferLoading: No change in behavior.
+        - DocumentWillBecomeInactive: This is about page cache, and documents are only allowed
+        to be cached when fully loaded.
+        - PageWillBeSuspended: This appears to be part of Frame::suspendActiveDOMObjectsAndAnimations()
+        implementation, I'm guessing that it is appropriate to continue loading.
+
+        * dom/Document.cpp:
+        (WebCore::Document::suspendScheduledTasks):
+        (WebCore::Document::resumeScheduledTasks):
+        Only suspend/resume parsing when loading is deferred. This is not expressed directly,
+        but it's important to do this to avoid executing JS behind alerts and other modal dialogs.
+
+        * html/parser/HTMLParserScheduler.h: Added m_suspended member variable for assertions.
+
+        * html/parser/HTMLParserScheduler.cpp:
+        (WebCore::HTMLParserScheduler::HTMLParserScheduler):
+        (WebCore::HTMLParserScheduler::continueNextChunkTimerFired):
+        (WebCore::HTMLParserScheduler::scheduleForResume):
+        (WebCore::HTMLParserScheduler::suspend):
+        (WebCore::HTMLParserScheduler::resume):
+        Update m_suspended and assert as appropriate. No behavior changes for release mode.
+
+        * page/Frame.cpp: (WebCore::Frame::suspendActiveDOMObjectsAndAnimations):
+        Added a FIXME.
+
+2013-07-27  Alexey Proskuryakov  &lt;ap@apple.com&gt;
+
</ins><span class="cx">         Make SuspendableTimer safer
</span><span class="cx">         https://bugs.webkit.org/show_bug.cgi?id=119127
</span><span class="cx"> 
</span></span></pre></div>
<a id="trunkSourceWebCoredomDocumentcpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/dom/Document.cpp (153406 => 153407)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/dom/Document.cpp        2013-07-28 00:04:14 UTC (rev 153406)
+++ trunk/Source/WebCore/dom/Document.cpp        2013-07-28 00:05:11 UTC (rev 153407)
</span><span class="lines">@@ -4816,7 +4816,12 @@
</span><span class="cx">     suspendActiveDOMObjects(reason);
</span><span class="cx">     scriptRunner()-&gt;suspend();
</span><span class="cx">     m_pendingTasksTimer.stop();
</span><del>-    if (m_parser)
</del><ins>+
+    // Deferring loading and suspending parser is necessary when we need to prevent re-entrant JavaScript execution
+    // (e.g. while displaying an alert).
+    // It is not currently possible to suspend parser unless loading is deferred, because new data arriving from network
+    // will trigger parsing, and leave the scheduler in an inconsistent state where it doesn't know whether it's suspended or not.
+    if (reason == ActiveDOMObject::WillDeferLoading &amp;&amp; m_parser)
</ins><span class="cx">         m_parser-&gt;suspendScheduledTasks();
</span><span class="cx"> 
</span><span class="cx">     m_scheduledTasksAreSuspended = true;
</span><span class="lines">@@ -4829,7 +4834,7 @@
</span><span class="cx"> 
</span><span class="cx">     ASSERT(m_scheduledTasksAreSuspended);
</span><span class="cx"> 
</span><del>-    if (m_parser)
</del><ins>+    if (reason == ActiveDOMObject::WillDeferLoading &amp;&amp; m_parser)
</ins><span class="cx">         m_parser-&gt;resumeScheduledTasks();
</span><span class="cx">     if (!m_pendingTasks.isEmpty())
</span><span class="cx">         m_pendingTasksTimer.startOneShot(0);
</span></span></pre></div>
<a id="trunkSourceWebCorehtmlparserHTMLParserSchedulercpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/html/parser/HTMLParserScheduler.cpp (153406 => 153407)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/html/parser/HTMLParserScheduler.cpp        2013-07-28 00:04:14 UTC (rev 153406)
+++ trunk/Source/WebCore/html/parser/HTMLParserScheduler.cpp        2013-07-28 00:05:11 UTC (rev 153407)
</span><span class="lines">@@ -1,5 +1,6 @@
</span><span class="cx"> /*
</span><span class="cx">  * Copyright (C) 2010 Google, Inc. All Rights Reserved.
</span><ins>+ * Copyright (C) 2013 Apple, Inc. All Rights Reserved.
</ins><span class="cx">  *
</span><span class="cx">  * Redistribution and use in source and binary forms, with or without
</span><span class="cx">  * modification, are permitted provided that the following conditions
</span><span class="lines">@@ -99,6 +100,9 @@
</span><span class="cx">     , m_parserChunkSize(parserChunkSize(m_parser-&gt;document()-&gt;page()))
</span><span class="cx">     , m_continueNextChunkTimer(this, &amp;HTMLParserScheduler::continueNextChunkTimerFired)
</span><span class="cx">     , m_isSuspendedWithActiveTimer(false)
</span><ins>+#if !ASSERT_DISABLED
+    , m_suspended(false)
+#endif
</ins><span class="cx"> {
</span><span class="cx"> }
</span><span class="cx"> 
</span><span class="lines">@@ -109,6 +113,7 @@
</span><span class="cx"> 
</span><span class="cx"> void HTMLParserScheduler::continueNextChunkTimerFired(Timer&lt;HTMLParserScheduler&gt;* timer)
</span><span class="cx"> {
</span><ins>+    ASSERT(!m_suspended);
</ins><span class="cx">     ASSERT_UNUSED(timer, timer == &amp;m_continueNextChunkTimer);
</span><span class="cx">     // FIXME: The timer class should handle timer priorities instead of this code.
</span><span class="cx">     // If a layout is scheduled, wait again to let the layout timer run first.
</span><span class="lines">@@ -132,13 +137,18 @@
</span><span class="cx"> 
</span><span class="cx"> void HTMLParserScheduler::scheduleForResume()
</span><span class="cx"> {
</span><ins>+    ASSERT(!m_suspended);
</ins><span class="cx">     m_continueNextChunkTimer.startOneShot(0);
</span><span class="cx"> }
</span><span class="cx"> 
</span><del>-
</del><span class="cx"> void HTMLParserScheduler::suspend()
</span><span class="cx"> {
</span><ins>+    ASSERT(!m_suspended);
</ins><span class="cx">     ASSERT(!m_isSuspendedWithActiveTimer);
</span><ins>+#if !ASSERT_DISABLED
+    m_suspended = true;
+#endif
+
</ins><span class="cx">     if (!m_continueNextChunkTimer.isActive())
</span><span class="cx">         return;
</span><span class="cx">     m_isSuspendedWithActiveTimer = true;
</span><span class="lines">@@ -147,7 +157,12 @@
</span><span class="cx"> 
</span><span class="cx"> void HTMLParserScheduler::resume()
</span><span class="cx"> {
</span><ins>+    ASSERT(m_suspended);
</ins><span class="cx">     ASSERT(!m_continueNextChunkTimer.isActive());
</span><ins>+#if !ASSERT_DISABLED
+    m_suspended = false;
+#endif
+
</ins><span class="cx">     if (!m_isSuspendedWithActiveTimer)
</span><span class="cx">         return;
</span><span class="cx">     m_isSuspendedWithActiveTimer = false;
</span></span></pre></div>
<a id="trunkSourceWebCorehtmlparserHTMLParserSchedulerh"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/html/parser/HTMLParserScheduler.h (153406 => 153407)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/html/parser/HTMLParserScheduler.h        2013-07-28 00:04:14 UTC (rev 153406)
+++ trunk/Source/WebCore/html/parser/HTMLParserScheduler.h        2013-07-28 00:05:11 UTC (rev 153407)
</span><span class="lines">@@ -103,6 +103,9 @@
</span><span class="cx">     int m_parserChunkSize;
</span><span class="cx">     Timer&lt;HTMLParserScheduler&gt; m_continueNextChunkTimer;
</span><span class="cx">     bool m_isSuspendedWithActiveTimer;
</span><ins>+#if !ASSERT_DISABLED
+    bool m_suspended;
+#endif
</ins><span class="cx"> };
</span><span class="cx"> 
</span><span class="cx"> }
</span></span></pre></div>
<a id="trunkSourceWebCorepageFramecpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebCore/page/Frame.cpp (153406 => 153407)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebCore/page/Frame.cpp        2013-07-28 00:04:14 UTC (rev 153406)
+++ trunk/Source/WebCore/page/Frame.cpp        2013-07-28 00:05:11 UTC (rev 153407)
</span><span class="lines">@@ -963,6 +963,7 @@
</span><span class="cx">     if (wasSuspended)
</span><span class="cx">         return;
</span><span class="cx"> 
</span><ins>+    // FIXME: Suspend/resume calls will not match if the frame is navigated, and gets a new document.
</ins><span class="cx">     if (document()) {
</span><span class="cx">         document()-&gt;suspendScriptedAnimationControllerCallbacks();
</span><span class="cx">         animation()-&gt;suspendAnimationsForDocument(document());
</span></span></pre>
</div>
</div>

</body>
</html>