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

Timestamp:

Mar 9, 2020, 1:17:21 PM (5 years ago)

Author:

Chris Dumez

Message:

Align garbage collection for XMLHttpRequest objects with the specification
​https://bugs.webkit.org/show_bug.cgi?id=208481

Reviewed by Ryosuke Niwa.

Align garbage collection for XMLHttpRequest objects with the specification:

• ​https://xhr.spec.whatwg.org/#garbage-collection

We now override ActiveDOMObject::hasPendingActivity() to match exactly the text
in the specification:
"""
An XMLHttpRequest object must not be garbage collected if its state is either
opened with the send() flag set, headers received, or loading, and it has one or
more event listeners registered whose type is one of readystatechange, progress,
abort, error, load, timeout, and loadend.
"""

Previously, we were trying to implement this behavior with ActiveDOMObject's
setPendingActivity() / unsetPendingActivity() but this was error and leak prone.
It was also keeping the JS wrapper alive too long in the cases where the JS
does not have any event listeners. If the JS has not event listeners, then we
can collect the JS wrapper, we just need to keep the implementation
XMLHttpRequest object for the duration of the load.

No new tests, covered by existing test such as:
fast/xmlhttprequest/xmlhttprequest-gc.html

• dom/EventTarget.cpp:

(WebCore::EventTarget::addEventListener):
(WebCore::EventTarget::removeEventListener):
(WebCore::EventTarget::removeAllEventListeners):

• dom/EventTarget.h:

(WebCore::EventTarget::eventListenersDidChange):

• xml/XMLHttpRequest.cpp:

(WebCore::XMLHttpRequest::XMLHttpRequest):
(WebCore::XMLHttpRequest::changeState):
(WebCore::XMLHttpRequest::open):
(WebCore::XMLHttpRequest::prepareToSend):
(WebCore::XMLHttpRequest::createRequest):
(WebCore::XMLHttpRequest::abort):
(WebCore::XMLHttpRequest::internalAbort):
(WebCore::XMLHttpRequest::networkError):
(WebCore::XMLHttpRequest::didFail):
(WebCore::XMLHttpRequest::didFinishLoading):
(WebCore::XMLHttpRequest::didReachTimeout):
(WebCore::XMLHttpRequest::contextDestroyed):
(WebCore::XMLHttpRequest::eventListenersDidChange):
(WebCore::XMLHttpRequest::hasPendingActivity const):

• xml/XMLHttpRequest.h:
• xml/XMLHttpRequestProgressEventThrottle.cpp:

(WebCore::XMLHttpRequestProgressEventThrottle::XMLHttpRequestProgressEventThrottle):
(WebCore::XMLHttpRequestProgressEventThrottle::dispatchEventWhenPossible):
(WebCore::XMLHttpRequestProgressEventThrottle::suspend):
(WebCore::XMLHttpRequestProgressEventThrottle::resume):

• xml/XMLHttpRequestProgressEventThrottle.h:

File:

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

trunk/Source/WebCore/xml/XMLHttpRequestProgressEventThrottle.cpp

@@ -30 +30 @@
 #include "EventNames.h"
 #include "EventTarget.h"
+#include "XMLHttpRequest.h"
 #include "XMLHttpRequestProgressEvent.h"
 
@@ -36 +37 @@
 const Seconds XMLHttpRequestProgressEventThrottle::minimumProgressEventDispatchingInterval { 50_ms }; // 50 ms per specification.
 
-XMLHttpRequestProgressEventThrottle::XMLHttpRequestProgressEventThrottle(EventTarget& target)
+XMLHttpRequestProgressEventThrottle::XMLHttpRequestProgressEventThrottle(XMLHttpRequest& target)
     : m_target(target)
     , m_dispatchThrottledProgressEventTimer(target.scriptExecutionContext(), *this, &XMLHttpRequestProgressEventThrottle::dispatchThrottledProgressEventTimerFired)
-    , m_dispatchDeferredEventsAfterResumingTimer(target.scriptExecutionContext(), *this, &XMLHttpRequestProgressEventThrottle::dispatchDeferredEventsAfterResuming)
 {
     m_dispatchThrottledProgressEventTimer.suspendIfNeeded();
-    m_dispatchDeferredEventsAfterResumingTimer.suspendIfNeeded();
 }
 
@@ -82 +81 @@
 void XMLHttpRequestProgressEventThrottle::dispatchEventWhenPossible(Event& event)
 {
-    if (m_shouldDeferEventsDueToSuspension) {
-        if (m_eventsDeferredDueToSuspension.size() > 1 && event.type() == eventNames().readystatechangeEvent && event.type() == m_eventsDeferredDueToSuspension.last()->type()) {
-            // Readystatechange events are state-less so avoid repeating two identical events in a row on resume.
-            return;
-        }
-        m_eventsDeferredDueToSuspension.append(event);
-    } else
+    if (m_shouldDeferEventsDueToSuspension)
+        m_target.queueTaskToDispatchEvent(m_target, TaskSource::Networking, makeRef(event));
+    else
         m_target.dispatchEvent(event);
 }
@@ -118 +113 @@
 }
 
-void XMLHttpRequestProgressEventThrottle::dispatchDeferredEventsAfterResuming()
-{
-    ASSERT(m_shouldDeferEventsDueToSuspension);
-    m_shouldDeferEventsDueToSuspension = false;
-
-    // Take over the deferred events before dispatching them which can potentially add more.
-    auto eventsDeferredDueToSuspension = WTFMove(m_eventsDeferredDueToSuspension);
-
-    flushProgressEvent();
-
-    for (auto& deferredEvent : eventsDeferredDueToSuspension)
-        dispatchEventWhenPossible(deferredEvent);
-}
-
 void XMLHttpRequestProgressEventThrottle::dispatchThrottledProgressEventTimerFired()
 {
@@ -148 +129 @@
 {
     m_shouldDeferEventsDueToSuspension = true;
+
+    if (m_hasPendingThrottledProgressEvent) {
+        m_target.queueTaskKeepingObjectAlive(m_target, TaskSource::Networking, [this] {
+            flushProgressEvent();
+        });
+    }
 }
 
 void XMLHttpRequestProgressEventThrottle::resume()
 {
-    if (m_eventsDeferredDueToSuspension.isEmpty() && !m_hasPendingThrottledProgressEvent) {
+    m_target.queueTaskKeepingObjectAlive(m_target, TaskSource::Networking, [this] {
         m_shouldDeferEventsDueToSuspension = false;
-        return;
-    }
-
-    m_dispatchDeferredEventsAfterResumingTimer.startOneShot(0_s);
+    });
 }