Changeset 278329 in webkit

Timestamp:

Jun 1, 2021, 3:03:35 PM (5 years ago)

Author:

Chris Dumez

Message:

Fix unsafe access to m_upload in XMLHttpRequest::virtualHasPendingActivity()
​https://bugs.webkit.org/show_bug.cgi?id=226508

Reviewed by Geoffrey Garen.

Source/WebCore:

Fix unsafe access to m_upload in XMLHttpRequest::virtualHasPendingActivity() as virtualHasPendingActivity()
may get called off the main thread and m_upload gets initialized lazily on the main thread.

Tests: fast/xmlhttprequest/xmlhttprequest-upload-sameobject.html

http/tests/xmlhttprequest/upload-progress-events-gc.html

• xml/XMLHttpRequest.cpp:

(WebCore::XMLHttpRequest::updateHasRelevantEventListener):
(WebCore::XMLHttpRequest::eventListenersDidChange):
(WebCore::XMLHttpRequest::virtualHasPendingActivity const):

• xml/XMLHttpRequest.h:
• xml/XMLHttpRequest.idl:
• xml/XMLHttpRequestUpload.cpp:

(WebCore::XMLHttpRequestUpload::eventListenersDidChange):
(WebCore::XMLHttpRequestUpload::hasRelevantEventListener const):

• xml/XMLHttpRequestUpload.h:

LayoutTests:

Improve layout test coverage to make sure that XMLHttpRequest.upload always returns
the same object and that progress events on XMLHttpRequest.upload still get fired
after GC.

• fast/xmlhttprequest/xmlhttprequest-upload-sameobject-expected.txt: Added.
• fast/xmlhttprequest/xmlhttprequest-upload-sameobject.html: Added.
• http/tests/xmlhttprequest/upload-progress-events-gc-expected.txt: Added.
• http/tests/xmlhttprequest/upload-progress-events-gc.html: Added.

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/xmlhttprequest/xmlhttprequest-upload-sameobject-expected.txt (added)
• LayoutTests/fast/xmlhttprequest/xmlhttprequest-upload-sameobject.html (added)
• LayoutTests/http/tests/xmlhttprequest/upload-progress-events-gc-expected.txt (added)
• LayoutTests/http/tests/xmlhttprequest/upload-progress-events-gc.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/xml/XMLHttpRequest.cpp (modified) (3 diffs)
• Source/WebCore/xml/XMLHttpRequest.h (modified) (2 diffs)
• Source/WebCore/xml/XMLHttpRequest.idl (modified) (1 diff)
• Source/WebCore/xml/XMLHttpRequestUpload.cpp (modified) (1 diff)
• Source/WebCore/xml/XMLHttpRequestUpload.h (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2021-06-01  Chris Dumez  <cdumez@apple.com>
+
+        Fix unsafe access to m_upload in XMLHttpRequest::virtualHasPendingActivity()
+        https://bugs.webkit.org/show_bug.cgi?id=226508
+
+        Reviewed by Geoffrey Garen.
+
+        Improve layout test coverage to make sure that XMLHttpRequest.upload always returns
+        the same object and that progress events on XMLHttpRequest.upload still get fired
+        after GC.
+
+        * fast/xmlhttprequest/xmlhttprequest-upload-sameobject-expected.txt: Added.
+        * fast/xmlhttprequest/xmlhttprequest-upload-sameobject.html: Added.
+        * http/tests/xmlhttprequest/upload-progress-events-gc-expected.txt: Added.
+        * http/tests/xmlhttprequest/upload-progress-events-gc.html: Added.
+
 2021-06-01  Sam Weinig  <weinig@apple.com>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-06-01  Chris Dumez  <cdumez@apple.com>
+
+        Fix unsafe access to m_upload in XMLHttpRequest::virtualHasPendingActivity()
+        https://bugs.webkit.org/show_bug.cgi?id=226508
+
+        Reviewed by Geoffrey Garen.
+
+        Fix unsafe access to m_upload in XMLHttpRequest::virtualHasPendingActivity() as virtualHasPendingActivity()
+        may get called off the main thread and m_upload gets initialized lazily on the main thread.
+
+        Tests: fast/xmlhttprequest/xmlhttprequest-upload-sameobject.html
+               http/tests/xmlhttprequest/upload-progress-events-gc.html
+
+        * xml/XMLHttpRequest.cpp:
+        (WebCore::XMLHttpRequest::updateHasRelevantEventListener):
+        (WebCore::XMLHttpRequest::eventListenersDidChange):
+        (WebCore::XMLHttpRequest::virtualHasPendingActivity const):
+        * xml/XMLHttpRequest.h:
+        * xml/XMLHttpRequest.idl:
+        * xml/XMLHttpRequestUpload.cpp:
+        (WebCore::XMLHttpRequestUpload::eventListenersDidChange):
+        (WebCore::XMLHttpRequestUpload::hasRelevantEventListener const):
+        * xml/XMLHttpRequestUpload.h:
+
 2021-06-01  Fujii Hironori  <Hironori.Fujii@sony.com>
 

trunk/Source/WebCore/xml/XMLHttpRequest.cpp

@@ -1162 +1162 @@
 }
 
-void XMLHttpRequest::eventListenersDidChange()
+void XMLHttpRequest::updateHasRelevantEventListener()
 {
     m_hasRelevantEventListener = hasEventListeners(eventNames().abortEvent)
@@ -1170 +1170 @@
         || hasEventListeners(eventNames().progressEvent)
         || hasEventListeners(eventNames().readystatechangeEvent)
-        || hasEventListeners(eventNames().timeoutEvent);
+        || hasEventListeners(eventNames().timeoutEvent)
+        || (m_upload && m_upload->hasRelevantEventListener());
+}
+
+void XMLHttpRequest::eventListenersDidChange()
+{
+    updateHasRelevantEventListener();
 }
 
@@ -1177 +1183 @@
 bool XMLHttpRequest::virtualHasPendingActivity() const
 {
-    if (!m_hasRelevantEventListener && !(m_upload && m_upload->hasRelevantEventListener()))
+    if (!m_hasRelevantEventListener)
         return false;
 

trunk/Source/WebCore/xml/XMLHttpRequest.h

@@ -134 +134 @@
 
 private:
+    friend class XMLHttpRequestUpload;
     explicit XMLHttpRequest(ScriptExecutionContext&);
+
+    void updateHasRelevantEventListener();
 
     // EventTarget.
@@ -248 +251 @@
     std::optional<ExceptionCode> m_exceptionCode;
     RefPtr<UserGestureToken> m_userGestureToken;
-    bool m_hasRelevantEventListener { false };
+    std::atomic<bool> m_hasRelevantEventListener;
 };
 

trunk/Source/WebCore/xml/XMLHttpRequest.idl

@@ -67 +67 @@
     attribute unsigned long timeout;
     attribute boolean withCredentials;
-    readonly attribute XMLHttpRequestUpload upload;
+    [SameObject] readonly attribute XMLHttpRequestUpload upload;
     undefined send(optional (Document or BodyInit)? body = null);
     undefined abort();

trunk/Source/WebCore/xml/XMLHttpRequestUpload.cpp

@@ -44 +44 @@
 void XMLHttpRequestUpload::eventListenersDidChange()
 {
-    m_hasRelevantEventListener = hasEventListeners(eventNames().abortEvent)
+    m_request.updateHasRelevantEventListener();
+}
+
+bool XMLHttpRequestUpload::hasRelevantEventListener() const
+{
+    return hasEventListeners(eventNames().abortEvent)
         || hasEventListeners(eventNames().errorEvent)
         || hasEventListeners(eventNames().loadEvent)

trunk/Source/WebCore/xml/XMLHttpRequestUpload.h

@@ -41 +41 @@
     void dispatchProgressEvent(const AtomString& type, unsigned long long loaded, unsigned long long total);
 
-    bool hasRelevantEventListener() const { return m_hasRelevantEventListener; }
+    bool hasRelevantEventListener() const;
 
 private:
@@ -53 +53 @@
 
     XMLHttpRequest& m_request;
-    bool m_hasRelevantEventListener { false };
 };