Changeset 114374 in webkit for trunk/Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp

Timestamp:

Apr 17, 2012, 7:02:07 AM (13 years ago)

Author:

[email protected]

Message:

Asserts in XMLHttpRequestProgressEventThrottle
​https://bugs.webkit.org/show_bug.cgi?id=81506

Patch by Allan Sandfeld Jensen <​[email protected]> on 2012-04-17
Reviewed by Julien Chaffraix.

Source/WebCore:

The asserts were incorrectly triggered because suspending active DOM objects
(which suspends the XMLHttpRequestProgressEventThrottle) doesn't stop JavaScript
from running or suspend any running loader we may have. The previous code would
assume those 2 cases were impossible.

When XmlHttpRequest::open is called or data is received while the XmlHttpRequest object
is suspended the object may attempt to dispatch events. This patch defers these events
until the object is resumed.

Progress events are coalesced similar to normal throttling, and readystate-change events
are coalesced to avoid identical events emitted right after eachother.

On resume the events are dispatched after a timer to avoid interfering with
ScriptExecutionContext which is iterating over suspended objects.

Test: fast/events/pagehide-xhr-open.html

• xml/XMLHttpRequestProgressEventThrottle.cpp:

(WebCore::XMLHttpRequestProgressEventThrottle::XMLHttpRequestProgressEventThrottle):
(WebCore::XMLHttpRequestProgressEventThrottle::dispatchProgressEvent):
(WebCore::XMLHttpRequestProgressEventThrottle::dispatchEvent):
(WebCore::XMLHttpRequestProgressEventThrottle::flushProgressEvent):
(WebCore::XMLHttpRequestProgressEventThrottle::dispatchDeferredEvents):
(WebCore::XMLHttpRequestProgressEventThrottle::fired):
(WebCore::XMLHttpRequestProgressEventThrottle::suspend):
(WebCore::XMLHttpRequestProgressEventThrottle::resume):

• xml/XMLHttpRequestProgressEventThrottle.h:

(XMLHttpRequestProgressEventThrottle):

LayoutTests:

Tests that xmlhttprequest.open does not assert in when called from onpagehide.

• fast/events/pagehide-xhr-open-expected.txt: Added.
• fast/events/pagehide-xhr-open.html: Added.
• fast/events/resources/subframe-xmlhttprequest.html: Added.
• platform/chromium/test_expectations.txt:

File:

• trunk/Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp (modified) (9 diffs)

trunk/Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp

@@ -1 +1 @@
 /*
- * Copyright (C) 2010 Julien Chaffraix <[email protected]>
- * All right reserved.
+ * Copyright (C) 2010 Julien Chaffraix <[email protected]>  All right reserved.
+ * Copyright (C) 2012 Nokia Corporation and/or its subsidiary(-ies)
  *
  * Redistribution and use in source and binary forms, with or without
@@ -39 +39 @@
     , m_loaded(0)
     , m_total(0)
-    , m_suspended(false)
+    , m_deferEvents(false)
+    , m_dispatchDeferredEventsTimer(this, &XMLHttpRequestProgressEventThrottle::dispatchDeferredEvents)
 {
     ASSERT(target);
@@ -50 +51 @@
 void XMLHttpRequestProgressEventThrottle::dispatchProgressEvent(bool lengthComputable, unsigned long long loaded, unsigned long long total)
 {
-    ASSERT(!suspended());
+    if (m_deferEvents) {
+        // Only store the latest progress event while suspended.
+        m_deferredProgressEvent = XMLHttpRequestProgressEvent::create(eventNames().progressEvent, lengthComputable, loaded, total);
+        return;
+    }
+
     if (!isActive()) {
         // The timer is not active so the least frequent event for now is every byte.
@@ -80 +86 @@
 void XMLHttpRequestProgressEventThrottle::dispatchEvent(PassRefPtr<Event> event)
 {
-    ASSERT(!suspended());
-    // We should not have any pending events from a previous resume.
-    ASSERT(!m_pausedEvent);
-
-    m_target->dispatchEvent(event);
+    ASSERT(event);
+    if (m_deferEvents) {
+        if (m_deferredEvents.size() > 1 && event->type() == eventNames().readystatechangeEvent && event->type() == m_deferredEvents.last()->type()) {
+            // Readystatechange events are state-less so avoid repeating two identical events in a row on resume.
+            return;
+        }
+        m_deferredEvents.append(event);
+    } else
+        m_target->dispatchEvent(event);
 }
 
@@ -97 +107 @@
 void XMLHttpRequestProgressEventThrottle::flushProgressEvent()
 {
+    if (m_deferEvents && m_deferredProgressEvent) {
+        // Move the progress event to the queue, to get it in the right order on resume.
+        m_deferredEvents.append(m_deferredProgressEvent);
+        m_deferredProgressEvent = 0;
+        return;
+    }
+
     if (!hasEventToDispatch())
         return;
@@ -110 +127 @@
 }
 
-void XMLHttpRequestProgressEventThrottle::dispatchPausedEvent()
-{
-    ASSERT(!suspended());
-    if (!m_pausedEvent)
-        return;
-
-    dispatchEvent(m_pausedEvent);
-    m_pausedEvent = 0;
+void XMLHttpRequestProgressEventThrottle::dispatchDeferredEvents(Timer<XMLHttpRequestProgressEventThrottle>* timer)
+{
+    ASSERT_UNUSED(timer, timer == &m_dispatchDeferredEventsTimer);
+    ASSERT(m_deferEvents);
+    m_deferEvents = false;
+
+    // Take over the deferred events before dispatching them which can potentially add more.
+    Vector<RefPtr<Event> > deferredEvents;
+    m_deferredEvents.swap(deferredEvents);
+
+    RefPtr<Event> deferredProgressEvent = m_deferredProgressEvent;
+    m_deferredProgressEvent = 0;
+
+    Vector<RefPtr<Event> >::const_iterator it = deferredEvents.begin();
+    const Vector<RefPtr<Event> >::const_iterator end = deferredEvents.end();
+    for (; it != end; ++it)
+        dispatchEvent(*it);
+
+    // The progress event will be in the m_deferredEvents vector if the load was finished while suspended.
+    // If not, just send the most up-to-date progress on resume.
+    if (deferredProgressEvent)
+        dispatchEvent(deferredProgressEvent);
 }
 
@@ -123 +154 @@
 {
     ASSERT(isActive());
-    ASSERT(!suspended());
-    ASSERT(!m_pausedEvent);
     if (!hasEventToDispatch()) {
         // No progress event was queued since the previous dispatch, we can safely stop the timer.
@@ -143 +172 @@
 void XMLHttpRequestProgressEventThrottle::suspend()
 {
-    ASSERT(!m_pausedEvent);
-
-    m_suspended = true;
+    // If re-suspended before deferred events have been dispatched, just stop the dispatch
+    // and continue the last suspend.
+    if (m_dispatchDeferredEventsTimer.isActive()) {
+        ASSERT(m_deferEvents);
+        m_dispatchDeferredEventsTimer.stop();
+        return;
+    }
+    ASSERT(!m_deferredProgressEvent);
+    ASSERT(m_deferredEvents.isEmpty());
+    ASSERT(!m_deferEvents);
+
+    m_deferEvents = true;
     // If we have a progress event waiting to be dispatched,
-    // just queue it.
+    // just defer it.
     if (hasEventToDispatch()) {
-        m_pausedEvent = XMLHttpRequestProgressEvent::create(eventNames().progressEvent, m_lengthComputable, m_loaded, m_total);
+        m_deferredProgressEvent = XMLHttpRequestProgressEvent::create(eventNames().progressEvent, m_lengthComputable, m_loaded, m_total);
         m_total = 0;
         m_loaded = 0;
@@ -161 +199 @@
     ASSERT(!m_total);
 
-    m_suspended = false;
-    dispatchPausedEvent();
+    // Do not dispatch events inline here, since ScriptExecutionContext is iterating over
+    // the list of active DOM objects to resume them, and any activated JS event-handler
+    // could insert new active DOM objects to the list.
+    // m_deferEvents is kept true until all deferred events have been dispatched.
+    m_dispatchDeferredEventsTimer.startOneShot(0);
 }