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

@@ -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;

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;
 

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

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 {

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;

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;

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:

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<Document> 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,
+                       responsibleDocument, secFlags, contentPolicyType,
-                       nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST,
                        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,
+                       contentPolicyType, mCookieJarSettings,
-                       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,
+                       secFlags, contentPolicyType, mCookieJarSettings,
-                       mCookieJarSettings,
                        mPerformanceStorage,  // aPerformanceStorage
                        loadGroup,
                        nullptr,  // aCallbacks

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,

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<nsILoadInfo> loadInfo = channel->LoadInfo();
-  // Figure out if this is for an XHR, if we haven't done so already.
+  // Figure out if this is for an Async XHR, if we haven't done so already.
-  if (!mHasCheckedForXMLHttpRequest) {
+  // 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 =
+    mForAsyncXMLHttpRequest =
-        (contentType == nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST);
+        contentType == nsIContentPolicy::TYPE_INTERNAL_XMLHTTPREQUEST_ASYNC;
-    mHasCheckedForXMLHttpRequest = true;
+    mHasCheckedForAsyncXMLHttpRequest = true;
 
-    if (!mForXMLHttpRequest) {
+    if (!mForAsyncXMLHttpRequest) {
       return false;
     }
   }
 
-  // Suspend the queue if the associated document has suppressed event handling,
+  // 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).
   RefPtr<dom::Document> document;
   loadInfo->GetLoadingDocument(getter_AddRefs(document));
-  if (document && document->EventHandlingSuppressed() &&
+  if (document && document->EventHandlingSuppressed()) {
-      !document->IsInSyncOperation()) {
     document->AddSuspendedChannelEventQueue(this);
     SuspendInternal();
     return true;

netwerk/ipc/ChannelEventQueue.h

@@ -114,8 +114,8 @@ class ChannelEventQueue final {
         mSuspended(false),
         mForcedCount(0),
         mFlushing(false),
-        mHasCheckedForXMLHttpRequest(false),
+        mHasCheckedForAsyncXMLHttpRequest(false),
-        mForXMLHttpRequest(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
+  // Whether the queue is associated with an ssync XHR.
-  // the first time it is needed. These are MainThread-only.
+  // This is lazily instantiated the first time it is needed.
-  bool mHasCheckedForXMLHttpRequest;
+  // These are MainThread-only.
-  bool mForXMLHttpRequest;
+  bool mHasCheckedForAsyncXMLHttpRequest;
+  bool mForAsyncXMLHttpRequest;
 
   // Keep ptr to avoid refcount cycle: only grab ref during flushing.
   nsISupports* mOwner MOZ_GUARDED_BY(mMutex);

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");
   }
 

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<mozilla::dom::ClientInfo>(),
           Maybe<mozilla::dom::ServiceWorkerDescriptor>(), 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

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

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,
 }
 
 }