From 79e4b8e5761d62623072afaa7e417324378ab6a0 Mon Sep 17 00:00:00 2001 From: Thomas Wisniewski Date: Tue, 11 Feb 2025 14:44:28 +0000 Subject: [PATCH] Bug 697151 - distinguish between SYNC and ASYNC XMLHttpRequests in nsIContentPolicy types, and have ChannelEventQueue::MaybeSuspendIfEventsAreSuppressed only suspend async ones; a=dmeehan Original Revision: https://phabricator.services.mozilla.com/D213516 Differential Revision: https://phabricator.services.mozilla.com/D237613 --- dom/base/nsContentPolicyUtils.h | 3 ++- dom/base/nsContentUtils.h | 3 ++- dom/base/nsIContentPolicy.idl | 13 ++++++++--- dom/cache/DBSchema.cpp | 5 ++-- dom/fetch/InternalRequest.cpp | 3 ++- dom/security/SecFetch.cpp | 3 ++- dom/security/nsCSPUtils.cpp | 3 ++- dom/xhr/XMLHttpRequestMainThread.cpp | 12 +++++----- dom/xslt/xml/txXMLParser.cpp | 2 +- netwerk/ipc/ChannelEventQueue.cpp | 23 +++++++++---------- netwerk/ipc/ChannelEventQueue.h | 13 ++++++----- netwerk/protocol/http/nsHttpChannel.cpp | 4 +++- netwerk/test/fuzz/TestHttpFuzzing.cpp | 4 ++-- .../meta/xhr/send-sync-blocks-async.htm.ini | 2 -- tools/@types/lib.gecko.xpcom.d.ts | 5 ++-- 15 files changed, 56 insertions(+), 42 deletions(-) diff --git a/dom/base/nsContentPolicyUtils.h b/dom/base/nsContentPolicyUtils.h index 058180096673..a8eb8ebbd44b 100644 --- a/dom/base/nsContentPolicyUtils.h +++ b/dom/base/nsContentPolicyUtils.h @@ -121,7 +121,7 @@ inline const char* NS_CP_ContentTypeName(nsContentPolicyType contentType) { CASE_RETURN(TYPE_INTERNAL_AUDIO); CASE_RETURN(TYPE_INTERNAL_VIDEO); CASE_RETURN(TYPE_INTERNAL_TRACK); - CASE_RETURN(TYPE_INTERNAL_XMLHTTPREQUEST); + CASE_RETURN(TYPE_INTERNAL_XMLHTTPREQUEST_ASYNC); CASE_RETURN(TYPE_INTERNAL_EVENTSOURCE); CASE_RETURN(TYPE_INTERNAL_SERVICE_WORKER); CASE_RETURN(TYPE_INTERNAL_SCRIPT_PRELOAD); @@ -148,6 +148,7 @@ inline const char* NS_CP_ContentTypeName(nsContentPolicyType contentType) { CASE_RETURN(TYPE_PROXIED_WEBRTC_MEDIA); CASE_RETURN(TYPE_WEB_IDENTITY); CASE_RETURN(TYPE_WEB_TRANSPORT); + CASE_RETURN(TYPE_INTERNAL_XMLHTTPREQUEST_SYNC); CASE_RETURN(TYPE_END); case nsIContentPolicy::TYPE_INVALID: break; diff --git a/dom/base/nsContentUtils.h b/dom/base/nsContentUtils.h index 795f13f5ceaa..344d65c159ac 100644 --- a/dom/base/nsContentUtils.h +++ b/dom/base/nsContentUtils.h @@ -3718,7 +3718,8 @@ nsContentUtils::InternalContentPolicyTypeToExternal(nsContentPolicyType aType) { case nsIContentPolicy::TYPE_INTERNAL_TRACK: return ExtContentPolicy::TYPE_MEDIA; - case nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST: + case nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST_ASYNC: + case nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST_SYNC: case nsIContentPolicy::TYPE_INTERNAL_EVENTSOURCE: return ExtContentPolicy::TYPE_XMLHTTPREQUEST; diff --git a/dom/base/nsIContentPolicy.idl b/dom/base/nsIContentPolicy.idl index bb86e39103dd..f82f112c18ba 100644 --- a/dom/base/nsIContentPolicy.idl +++ b/dom/base/nsIContentPolicy.idl @@ -232,11 +232,11 @@ interface nsIContentPolicy : nsISupports TYPE_INTERNAL_TRACK = 32, /** - * Indicates an internal constant for an XMLHttpRequest. + * Indicates an internal constant for an asynchronous XMLHttpRequest. * * This will be mapped to TYPE_XMLHTTPREQUEST. */ - TYPE_INTERNAL_XMLHTTPREQUEST = 33, + TYPE_INTERNAL_XMLHTTPREQUEST_ASYNC = 33, /** * Indicates an internal constant for EventSource. @@ -434,13 +434,20 @@ interface nsIContentPolicy : nsISupports */ TYPE_WEB_TRANSPORT = 59, + /** + * Indicates an internal constant for a synchronous XMLHttpRequest. + * + * This will be mapped to TYPE_XMLHTTPREQUEST. + */ + TYPE_INTERNAL_XMLHTTPREQUEST_SYNC = 60, + /** * Used to indicate the end of this list, not a content policy. If you want * to add a new content policy type, place it before this sentinel value * TYPE_END, have it use TYPE_END's current value, and increment TYPE_END by * one. (TYPE_END should always have the highest numerical value.) */ - TYPE_END = 60, + TYPE_END = 61, /* When adding new content types, please update diff --git a/dom/cache/DBSchema.cpp b/dom/cache/DBSchema.cpp index 390c1a9fd6d5..0833744077e5 100644 --- a/dom/cache/DBSchema.cpp +++ b/dom/cache/DBSchema.cpp @@ -365,7 +365,7 @@ static_assert( nsIContentPolicy::TYPE_INTERNAL_AUDIO == 30 && nsIContentPolicy::TYPE_INTERNAL_VIDEO == 31 && nsIContentPolicy::TYPE_INTERNAL_TRACK == 32 && - nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST == 33 && + nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST_ASYNC == 33 && nsIContentPolicy::TYPE_INTERNAL_EVENTSOURCE == 34 && nsIContentPolicy::TYPE_INTERNAL_SERVICE_WORKER == 35 && nsIContentPolicy::TYPE_INTERNAL_SCRIPT_PRELOAD == 36 && @@ -391,7 +391,8 @@ static_assert( nsIContentPolicy::TYPE_WEB_IDENTITY == 57 && nsIContentPolicy::TYPE_INTERNAL_WORKER_STATIC_MODULE == 58 && nsIContentPolicy::TYPE_WEB_TRANSPORT == 59 && - nsIContentPolicy::TYPE_END == 60, + nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST_SYNC == 60 && + nsIContentPolicy::TYPE_END == 61, "nsContentPolicyType values are as expected"); namespace { diff --git a/dom/fetch/InternalRequest.cpp b/dom/fetch/InternalRequest.cpp index d2086e58ebd1..eb50ed5fd3fd 100644 --- a/dom/fetch/InternalRequest.cpp +++ b/dom/fetch/InternalRequest.cpp @@ -305,7 +305,8 @@ RequestDestination InternalRequest::MapContentPolicyTypeToRequestDestination( case nsIContentPolicy::TYPE_PING: return RequestDestination::_empty; case nsIContentPolicy::TYPE_XMLHTTPREQUEST: - case nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST: + case nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST_ASYNC: + case nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST_SYNC: return RequestDestination::_empty; case nsIContentPolicy::TYPE_INTERNAL_EVENTSOURCE: return RequestDestination::_empty; diff --git a/dom/security/SecFetch.cpp b/dom/security/SecFetch.cpp index 17f4a23e0e2c..1d23dfd4c186 100644 --- a/dom/security/SecFetch.cpp +++ b/dom/security/SecFetch.cpp @@ -66,7 +66,8 @@ nsCString MapInternalContentPolicyTypeToDest(nsContentPolicyType aType) { case nsIContentPolicy::TYPE_PING: return "empty"_ns; case nsIContentPolicy::TYPE_XMLHTTPREQUEST: - case nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST: + case nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST_ASYNC: + case nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST_SYNC: return "empty"_ns; case nsIContentPolicy::TYPE_INTERNAL_EVENTSOURCE: return "empty"_ns; diff --git a/dom/security/nsCSPUtils.cpp b/dom/security/nsCSPUtils.cpp index f4aecfaf44d4..1cdd9b342f14 100644 --- a/dom/security/nsCSPUtils.cpp +++ b/dom/security/nsCSPUtils.cpp @@ -339,7 +339,8 @@ CSPDirective CSP_ContentTypeToDirective(nsContentPolicyType aType) { case nsIContentPolicy::TYPE_BEACON: case nsIContentPolicy::TYPE_PING: case nsIContentPolicy::TYPE_FETCH: - case nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST: + case nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST_ASYNC: + case nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST_SYNC: case nsIContentPolicy::TYPE_INTERNAL_EVENTSOURCE: case nsIContentPolicy::TYPE_INTERNAL_FETCH_PRELOAD: case nsIContentPolicy::TYPE_WEB_IDENTITY: diff --git a/dom/xhr/XMLHttpRequestMainThread.cpp b/dom/xhr/XMLHttpRequestMainThread.cpp index ab296a42ad99..f0ce577211d1 100644 --- a/dom/xhr/XMLHttpRequestMainThread.cpp +++ b/dom/xhr/XMLHttpRequestMainThread.cpp @@ -2591,11 +2591,13 @@ nsresult XMLHttpRequestMainThread::CreateChannel() { // where it will be the parent document, which is not the one we want to use. nsresult rv; nsCOMPtr responsibleDocument = GetDocumentIfCurrent(); + auto contentPolicyType = + mFlagSynchronous ? nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST_SYNC + : nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST_ASYNC; if (responsibleDocument && responsibleDocument->NodePrincipal() == mPrincipal) { rv = NS_NewChannel(getter_AddRefs(mChannel), mRequestURL, - responsibleDocument, secFlags, - nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST, + responsibleDocument, secFlags, contentPolicyType, nullptr, // aPerformanceStorage loadGroup, nullptr, // aCallbacks @@ -2603,8 +2605,7 @@ nsresult XMLHttpRequestMainThread::CreateChannel() { } else if (mClientInfo.isSome()) { rv = NS_NewChannel(getter_AddRefs(mChannel), mRequestURL, mPrincipal, mClientInfo.ref(), mController, secFlags, - nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST, - mCookieJarSettings, + contentPolicyType, mCookieJarSettings, mPerformanceStorage, // aPerformanceStorage loadGroup, nullptr, // aCallbacks @@ -2612,8 +2613,7 @@ nsresult XMLHttpRequestMainThread::CreateChannel() { } else { // Otherwise use the principal. rv = NS_NewChannel(getter_AddRefs(mChannel), mRequestURL, mPrincipal, - secFlags, nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST, - mCookieJarSettings, + secFlags, contentPolicyType, mCookieJarSettings, mPerformanceStorage, // aPerformanceStorage loadGroup, nullptr, // aCallbacks diff --git a/dom/xslt/xml/txXMLParser.cpp b/dom/xslt/xml/txXMLParser.cpp index b769d6f17e42..779e198fb1da 100644 --- a/dom/xslt/xml/txXMLParser.cpp +++ b/dom/xslt/xml/txXMLParser.cpp @@ -36,7 +36,7 @@ nsresult txParseDocumentFromURI(const nsAString& aHref, nsAutoSyncOperation sync(loaderDocument, SyncOperationBehavior::eSuspendInput); rv = nsSyncLoadService::LoadDocument( - documentURI, nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST, + documentURI, nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST_SYNC, loaderDocument->NodePrincipal(), nsILoadInfo::SEC_REQUIRE_CORS_INHERITS_SEC_CONTEXT, loadGroup, loaderDocument->CookieJarSettings(), true, diff --git a/netwerk/ipc/ChannelEventQueue.cpp b/netwerk/ipc/ChannelEventQueue.cpp index 911deaa7d9a1..089b130d2d01 100644 --- a/netwerk/ipc/ChannelEventQueue.cpp +++ b/netwerk/ipc/ChannelEventQueue.cpp @@ -185,7 +185,7 @@ bool ChannelEventQueue::MaybeSuspendIfEventsAreSuppressed() { // Only suppress events for queues associated with XHRs, as these can cause // content scripts to run. - if (mHasCheckedForXMLHttpRequest && !mForXMLHttpRequest) { + if (mHasCheckedForAsyncXMLHttpRequest && !mForAsyncXMLHttpRequest) { return false; } @@ -196,25 +196,24 @@ bool ChannelEventQueue::MaybeSuspendIfEventsAreSuppressed() { } nsCOMPtr loadInfo = channel->LoadInfo(); - // Figure out if this is for an XHR, if we haven't done so already. - if (!mHasCheckedForXMLHttpRequest) { + // Figure out if this is for an Async XHR, if we haven't done so already. + // We don't want to suspend Sync XHRs, as they'll always suspend event + // handling on the document, but we still need to process events for them. + if (!mHasCheckedForAsyncXMLHttpRequest) { nsContentPolicyType contentType = loadInfo->InternalContentPolicyType(); - mForXMLHttpRequest = - (contentType == nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST); - mHasCheckedForXMLHttpRequest = true; + mForAsyncXMLHttpRequest = + contentType == nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST_ASYNC; + mHasCheckedForAsyncXMLHttpRequest = true; - if (!mForXMLHttpRequest) { + if (!mForAsyncXMLHttpRequest) { return false; } } - // Suspend the queue if the associated document has suppressed event handling, - // *and* it is not in the middle of a synchronous operation that might require - // XHR events to be processed (such as a synchronous XHR). + // Suspend the queue if the associated document has suppressed event handling. RefPtr document; loadInfo->GetLoadingDocument(getter_AddRefs(document)); - if (document && document->EventHandlingSuppressed() && - !document->IsInSyncOperation()) { + if (document && document->EventHandlingSuppressed()) { document->AddSuspendedChannelEventQueue(this); SuspendInternal(); return true; diff --git a/netwerk/ipc/ChannelEventQueue.h b/netwerk/ipc/ChannelEventQueue.h index 1b61088e91bd..7790fdf3430c 100644 --- a/netwerk/ipc/ChannelEventQueue.h +++ b/netwerk/ipc/ChannelEventQueue.h @@ -114,8 +114,8 @@ class ChannelEventQueue final { mSuspended(false), mForcedCount(0), mFlushing(false), - mHasCheckedForXMLHttpRequest(false), - mForXMLHttpRequest(false), + mHasCheckedForAsyncXMLHttpRequest(false), + mForAsyncXMLHttpRequest(false), mOwner(owner), mMutex("ChannelEventQueue::mMutex"), mRunningMutex("ChannelEventQueue::mRunningMutex") {} @@ -184,10 +184,11 @@ class ChannelEventQueue final { MOZ_GUARDED_BY(mMutex); bool mFlushing MOZ_GUARDED_BY(mMutex); - // Whether the queue is associated with an XHR. This is lazily instantiated - // the first time it is needed. These are MainThread-only. - bool mHasCheckedForXMLHttpRequest; - bool mForXMLHttpRequest; + // Whether the queue is associated with an ssync XHR. + // This is lazily instantiated the first time it is needed. + // These are MainThread-only. + bool mHasCheckedForAsyncXMLHttpRequest; + bool mForAsyncXMLHttpRequest; // Keep ptr to avoid refcount cycle: only grab ref during flushing. nsISupports* mOwner MOZ_GUARDED_BY(mMutex); diff --git a/netwerk/protocol/http/nsHttpChannel.cpp b/netwerk/protocol/http/nsHttpChannel.cpp index 8d8e7419fa64..03b351297f83 100644 --- a/netwerk/protocol/http/nsHttpChannel.cpp +++ b/netwerk/protocol/http/nsHttpChannel.cpp @@ -3991,7 +3991,9 @@ nsresult nsHttpChannel::OpenCacheEntryInternal(bool isHttps) { mLoadInfo->InternalContentPolicyType() == nsIContentPolicy::TYPE_XMLHTTPREQUEST || mLoadInfo->InternalContentPolicyType() == - nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST)) { + nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST_ASYNC || + mLoadInfo->InternalContentPolicyType() == + nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST_SYNC)) { mCacheIdExtension.Append("FETCH"); } diff --git a/netwerk/test/fuzz/TestHttpFuzzing.cpp b/netwerk/test/fuzz/TestHttpFuzzing.cpp index e717b608e768..5724035cdd74 100644 --- a/netwerk/test/fuzz/TestHttpFuzzing.cpp +++ b/netwerk/test/fuzz/TestHttpFuzzing.cpp @@ -171,7 +171,7 @@ static int FuzzingRunNetworkHttp(const uint8_t* data, size_t size) { nsContentUtils::GetSystemPrincipal(), // loading principal nsContentUtils::GetSystemPrincipal(), // triggering principal nullptr, // Context - secFlags, nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST, + secFlags, nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST_ASYNC, Maybe(), Maybe(), sandboxFlags); @@ -186,7 +186,7 @@ static int FuzzingRunNetworkHttp(const uint8_t* data, size_t size) { } else { rv = NS_NewChannel(getter_AddRefs(channel), url, nsContentUtils::GetSystemPrincipal(), secFlags, - nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST, + nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST_ASYNC, nullptr, // aCookieJarSettings nullptr, // aPerformanceStorage nullptr, // loadGroup diff --git a/testing/web-platform/meta/xhr/send-sync-blocks-async.htm.ini b/testing/web-platform/meta/xhr/send-sync-blocks-async.htm.ini index 0076f71b059f..9285254d5656 100644 --- a/testing/web-platform/meta/xhr/send-sync-blocks-async.htm.ini +++ b/testing/web-platform/meta/xhr/send-sync-blocks-async.htm.ini @@ -1,5 +1,3 @@ [send-sync-blocks-async.htm] expected: if (os == "android") and fission: [OK, TIMEOUT] - [XMLHttpRequest: sync requests should block events on pending async requests] - expected: FAIL diff --git a/tools/@types/lib.gecko.xpcom.d.ts b/tools/@types/lib.gecko.xpcom.d.ts index 891093525a8e..f7bbe0ecc554 100644 --- a/tools/@types/lib.gecko.xpcom.d.ts +++ b/tools/@types/lib.gecko.xpcom.d.ts @@ -2054,7 +2054,7 @@ enum nsContentPolicyType { TYPE_INTERNAL_AUDIO = 30, TYPE_INTERNAL_VIDEO = 31, TYPE_INTERNAL_TRACK = 32, - TYPE_INTERNAL_XMLHTTPREQUEST = 33, + TYPE_INTERNAL_XMLHTTPREQUEST_ASYNC = 33, TYPE_INTERNAL_EVENTSOURCE = 34, TYPE_INTERNAL_SERVICE_WORKER = 35, TYPE_INTERNAL_SCRIPT_PRELOAD = 36, @@ -2081,7 +2081,8 @@ enum nsContentPolicyType { TYPE_WEB_IDENTITY = 57, TYPE_INTERNAL_WORKER_STATIC_MODULE = 58, TYPE_WEB_TRANSPORT = 59, - TYPE_END = 60, + TYPE_INTERNAL_XMLHTTPREQUEST_SYNC = 60, + TYPE_END = 61, } }