Changeset 153407 in webkit

Timestamp:

Jul 27, 2013, 5:05:11 PM (12 years ago)

Author:

ap@apple.com

Message:

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.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• dom/Document.cpp (modified) (2 diffs)
• html/parser/HTMLParserScheduler.cpp (modified) (5 diffs)
• html/parser/HTMLParserScheduler.h (modified) (1 diff)
• page/Frame.cpp (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/dom/Document.cpp

@@ -4817 +4817 @@
     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();
 
@@ -4830 +4835 @@
     ASSERT(m_scheduledTasksAreSuspended);
 
-    if (m_parser)
+    if (reason == ActiveDOMObject::WillDeferLoading && m_parser)
         m_parser->resumeScheduledTasks();
     if (!m_pendingTasks.isEmpty())

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

@@ -1 +1 @@
 /*
  * 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
@@ -100 +101 @@
     , m_continueNextChunkTimer(this, &HTMLParserScheduler::continueNextChunkTimerFired)
     , m_isSuspendedWithActiveTimer(false)
+#if !ASSERT_DISABLED
+    , m_suspended(false)
+#endif
 {
 }
@@ -110 +114 @@
 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.
@@ -133 +138 @@
 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;
@@ -148 +158 @@
 void HTMLParserScheduler::resume()
 {
+    ASSERT(m_suspended);
     ASSERT(!m_continueNextChunkTimer.isActive());
+#if !ASSERT_DISABLED
+    m_suspended = false;
+#endif
+
     if (!m_isSuspendedWithActiveTimer)
         return;

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

@@ -104 +104 @@
     Timer<HTMLParserScheduler> m_continueNextChunkTimer;
     bool m_isSuspendedWithActiveTimer;
+#if !ASSERT_DISABLED
+    bool m_suspended;
+#endif
 };
 

trunk/Source/WebCore/page/Frame.cpp

@@ -964 +964 @@
         return;
 
+    // FIXME: Suspend/resume calls will not match if the frame is navigated, and gets a new document.
     if (document()) {
         document()->suspendScriptedAnimationControllerCallbacks();