HTMLParserScheduler gets into an inconsistent state when suspended for reasons
authorap@apple.com <ap@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 28 Jul 2013 00:05:11 +0000 (00:05 +0000)
committerap@apple.com <ap@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 28 Jul 2013 00:05:11 +0000 (00:05 +0000)
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.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@153407 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/dom/Document.cpp
Source/WebCore/html/parser/HTMLParserScheduler.cpp
Source/WebCore/html/parser/HTMLParserScheduler.h
Source/WebCore/page/Frame.cpp

index c342894..b95412c 100644 (file)
@@ -1,5 +1,47 @@
 2013-07-27  Alexey Proskuryakov  <ap@apple.com>
 
+        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  <ap@apple.com>
+
         Make SuspendableTimer safer
         https://bugs.webkit.org/show_bug.cgi?id=119127
 
index a15b681..9349334 100644 (file)
@@ -4816,7 +4816,12 @@ void Document::suspendScheduledTasks(ActiveDOMObject::ReasonForSuspension reason
     suspendActiveDOMObjects(reason);
     scriptRunner()->suspend();
     m_pendingTasksTimer.stop();
-    if (m_parser)
+
+    // 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 && m_parser)
         m_parser->suspendScheduledTasks();
 
     m_scheduledTasksAreSuspended = true;
@@ -4829,7 +4834,7 @@ void Document::resumeScheduledTasks(ActiveDOMObject::ReasonForSuspension reason)
 
     ASSERT(m_scheduledTasksAreSuspended);
 
-    if (m_parser)
+    if (reason == ActiveDOMObject::WillDeferLoading && m_parser)
         m_parser->resumeScheduledTasks();
     if (!m_pendingTasks.isEmpty())
         m_pendingTasksTimer.startOneShot(0);
index cd6f3df..d203d77 100644 (file)
@@ -1,5 +1,6 @@
 /*
  * Copyright (C) 2010 Google, Inc. All Rights Reserved.
+ * Copyright (C) 2013 Apple, Inc. All Rights Reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -99,6 +100,9 @@ HTMLParserScheduler::HTMLParserScheduler(HTMLDocumentParser* parser)
     , m_parserChunkSize(parserChunkSize(m_parser->document()->page()))
     , m_continueNextChunkTimer(this, &HTMLParserScheduler::continueNextChunkTimerFired)
     , m_isSuspendedWithActiveTimer(false)
+#if !ASSERT_DISABLED
+    , m_suspended(false)
+#endif
 {
 }
 
@@ -109,6 +113,7 @@ HTMLParserScheduler::~HTMLParserScheduler()
 
 void HTMLParserScheduler::continueNextChunkTimerFired(Timer<HTMLParserScheduler>* timer)
 {
+    ASSERT(!m_suspended);
     ASSERT_UNUSED(timer, timer == &m_continueNextChunkTimer);
     // FIXME: The timer class should handle timer priorities instead of this code.
     // If a layout is scheduled, wait again to let the layout timer run first.
@@ -132,13 +137,18 @@ void HTMLParserScheduler::checkForYieldBeforeScript(PumpSession& session)
 
 void HTMLParserScheduler::scheduleForResume()
 {
+    ASSERT(!m_suspended);
     m_continueNextChunkTimer.startOneShot(0);
 }
 
-
 void HTMLParserScheduler::suspend()
 {
+    ASSERT(!m_suspended);
     ASSERT(!m_isSuspendedWithActiveTimer);
+#if !ASSERT_DISABLED
+    m_suspended = true;
+#endif
+
     if (!m_continueNextChunkTimer.isActive())
         return;
     m_isSuspendedWithActiveTimer = true;
@@ -147,7 +157,12 @@ void HTMLParserScheduler::suspend()
 
 void HTMLParserScheduler::resume()
 {
+    ASSERT(m_suspended);
     ASSERT(!m_continueNextChunkTimer.isActive());
+#if !ASSERT_DISABLED
+    m_suspended = false;
+#endif
+
     if (!m_isSuspendedWithActiveTimer)
         return;
     m_isSuspendedWithActiveTimer = false;
index 3c4ea38..8256cf4 100644 (file)
@@ -103,6 +103,9 @@ private:
     int m_parserChunkSize;
     Timer<HTMLParserScheduler> m_continueNextChunkTimer;
     bool m_isSuspendedWithActiveTimer;
+#if !ASSERT_DISABLED
+    bool m_suspended;
+#endif
 };
 
 }
index f673846..587c67c 100644 (file)
@@ -963,6 +963,7 @@ void Frame::suspendActiveDOMObjectsAndAnimations()
     if (wasSuspended)
         return;
 
+    // FIXME: Suspend/resume calls will not match if the frame is navigated, and gets a new document.
     if (document()) {
         document()->suspendScriptedAnimationControllerCallbacks();
         animation()->suspendAnimationsForDocument(document());