Prevent sync XHR from being fired in a child frame when the unload handler was...

Prevent sync XHR from being fired in a child frame when the unload handler was attached from a parent document.

As of M80, sync XHR during page dismissal is not allowed by default. However, it was still possible to send a sync XHR request during dismissal by attaching an unload handler defined in the parent document to a child frame from the parent's script. This change prevents that from happening. (Note: It is still possible to schedule a sync XHR during a page dismissal, ie doing a SetTimeout in a beforeUnload handler that will do the synchronous XHR.)

This change also removes some use counters that are no longer needed.

Bug: 1044348
Change-Id: Ieab8fd018d61aecc75241c6d7d01ad6c08eff7da
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2032426
Commit-Queue: Katie Dillon <kdillon@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org>
Cr-Commit-Position: refs/heads/master@{#742684}

third_party/blink/public/mojom/web_feature/web_feature.mojom

@@ -1581,8 +1581,6 @@ enum WebFeature {
   kSuppressHistoryEntryWithoutUserGesture = 2146,
   kPerformanceServerTiming = 2157,
   kFileReaderResultBeforeCompletion = 2158,
-  kSyncXhrInPageDismissal = 2159,
-  kAsyncXhrInPageDismissal = 2160,
   kAnimationSetPlaybackRateCompensatorySeek = 2162,
   kDeepCombinatorInStaticProfile = 2163,
   kPseudoShadowInStaticProfile = 2164,
@@ -2095,7 +2093,6 @@ enum WebFeature {
   kV8HTMLMediaElement_CaptureStream_Method = 2729,
   kQuirkyLineBoxBackgroundSize = 2730,
   kDirectlyCompositedImage = 2731,
-  kForbiddenSyncXhrInPageDismissal = 2732,
   kV8HTMLVideoElement_AutoPictureInPicture_AttributeGetter = 2733,
   kV8HTMLVideoElement_AutoPictureInPicture_AttributeSetter = 2734,
   kAutoPictureInPictureAttribute = 2735,

third_party/blink/renderer/core/dom/document.cc

@@ -285,6 +285,7 @@
 #include "third_party/blink/renderer/core/trustedtypes/trusted_html.h"
 #include "third_party/blink/renderer/core/xml/parser/xml_document_parser.h"
 #include "third_party/blink/renderer/core/xml_names.h"
+#include "third_party/blink/renderer/core/xmlhttprequest/main_thread_disallow_synchronous_xhr_scope.h"
 #include "third_party/blink/renderer/core/xmlns_names.h"
 #include "third_party/blink/renderer/platform/bindings/dom_data_store.h"
 #include "third_party/blink/renderer/platform/bindings/exception_messages.h"
@@ -3999,6 +4000,7 @@ bool Document::DispatchBeforeUnloadEvent(ChromeClient* chrome_client,
   if (ProcessingBeforeUnload())
     return false;
 
+  MainThreadDisallowSynchronousXHRScope disallow_synchronous_xhr;
   auto& before_unload_event = *MakeGarbageCollected<BeforeUnloadEvent>();
   before_unload_event.initEvent(event_type_names::kBeforeunload, false, true);
   load_event_progress_ = kBeforeUnloadEventInProgress;
@@ -4082,6 +4084,7 @@ void Document::DispatchUnloadEvents(
     SecurityOrigin* committing_origin,
     base::Optional<Document::UnloadEventTiming>* unload_timing) {
   PluginScriptForbiddenScope forbid_plugin_destructor_scripting;
+  MainThreadDisallowSynchronousXHRScope disallow_synchronous_xhr;
   if (parser_)
     parser_->StopParsing();
 

third_party/blink/renderer/core/frame/frame.cc

@@ -48,6 +48,7 @@
 #include "third_party/blink/renderer/core/page/focus_controller.h"
 #include "third_party/blink/renderer/core/page/page.h"
 #include "third_party/blink/renderer/core/probe/core_probes.h"
+#include "third_party/blink/renderer/core/xmlhttprequest/main_thread_disallow_synchronous_xhr_scope.h"
 #include "third_party/blink/renderer/platform/instrumentation/instance_counters.h"
 #include "third_party/blink/renderer/platform/instrumentation/use_counter.h"
 #include "third_party/blink/renderer/platform/loader/fetch/resource_error.h"
@@ -77,6 +78,7 @@ void Frame::Detach(FrameDetachType type) {
   // Detach() can be re-entered, so this can't simply DCHECK(IsAttached()).
   DCHECK(!IsDetached());
   lifecycle_.AdvanceTo(FrameLifecycle::kDetaching);
+  MainThreadDisallowSynchronousXHRScope disallow_synchronous_xhr;
 
   DetachImpl(type);
 

third_party/blink/renderer/core/xmlhttprequest/BUILD.gn

@@ -6,6 +6,8 @@ import("//third_party/blink/renderer/core/core.gni")
 
 blink_core_sources("xmlhttprequest") {
   sources = [
+    "main_thread_disallow_synchronous_xhr_scope.cc",
+    "main_thread_disallow_synchronous_xhr_scope.h",
     "xml_http_request.cc",
     "xml_http_request.h",
     "xml_http_request_event_target.h",

third_party/blink/renderer/core/xmlhttprequest/main_thread_disallow_synchronous_xhr_scope.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "third_party/blink/renderer/core/xmlhttprequest/main_thread_disallow_synchronous_xhr_scope.h"
+
+#include "base/logging.h"
+#include "third_party/blink/renderer/platform/wtf/wtf.h"
+
+namespace blink {
+
+static unsigned g_disallow_synchronous_xhr_count = 0;
+
+MainThreadDisallowSynchronousXHRScope::MainThreadDisallowSynchronousXHRScope() {
+  DCHECK(IsMainThread());
+  ++g_disallow_synchronous_xhr_count;
+}
+
+MainThreadDisallowSynchronousXHRScope::
+    ~MainThreadDisallowSynchronousXHRScope() {
+  DCHECK(IsMainThread());
+  --g_disallow_synchronous_xhr_count;
+}
+
+bool MainThreadDisallowSynchronousXHRScope::ShouldDisallowSynchronousXHR() {
+  DCHECK(IsMainThread());
+  return g_disallow_synchronous_xhr_count > 0;
+}
+
+}  // namespace blink

third_party/blink/renderer/core/xmlhttprequest/main_thread_disallow_synchronous_xhr_scope.h

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef THIRD_PARTY_BLINK_RENDERER_CORE_XMLHTTPREQUEST_MAIN_THREAD_DISALLOW_SYNCHRONOUS_XHR_SCOPE_H_
+#define THIRD_PARTY_BLINK_RENDERER_CORE_XMLHTTPREQUEST_MAIN_THREAD_DISALLOW_SYNCHRONOUS_XHR_SCOPE_H_
+
+#include "base/macros.h"
+#include "third_party/blink/renderer/core/core_export.h"
+#include "third_party/blink/renderer/platform/wtf/allocator/allocator.h"
+
+namespace blink {
+
+class CORE_EXPORT MainThreadDisallowSynchronousXHRScope final {
+  STACK_ALLOCATED();
+
+ public:
+  MainThreadDisallowSynchronousXHRScope();
+  ~MainThreadDisallowSynchronousXHRScope();
+
+  static bool ShouldDisallowSynchronousXHR();
+
+  DISALLOW_COPY_AND_ASSIGN(MainThreadDisallowSynchronousXHRScope);
+};
+
+}  // namespace blink
+
+#endif  // THIRD_PARTY_BLINK_RENDERER_CORE_XMLHTTPREQUEST_MAIN_THREAD_DISALLOW_SYNCHRONOUS_XHR_SCOPE_H_

third_party/blink/renderer/core/xmlhttprequest/xml_http_request.cc

@@ -68,6 +68,7 @@
 #include "third_party/blink/renderer/core/typed_arrays/dom_array_buffer_view.h"
 #include "third_party/blink/renderer/core/typed_arrays/dom_typed_array.h"
 #include "third_party/blink/renderer/core/url/url_search_params.h"
+#include "third_party/blink/renderer/core/xmlhttprequest/main_thread_disallow_synchronous_xhr_scope.h"
 #include "third_party/blink/renderer/core/xmlhttprequest/xml_http_request_upload.h"
 #include "third_party/blink/renderer/platform/bindings/dom_wrapper_world.h"
 #include "third_party/blink/renderer/platform/bindings/exception_state.h"
@@ -1100,18 +1101,6 @@ void XMLHttpRequest::CreateRequest(scoped_refptr<EncodedFormData> http_body,
   if (async_) {
     UseCounter::Count(&execution_context,
                       WebFeature::kXMLHttpRequestAsynchronous);
-    if (execution_context.IsDocument()) {
-      // Update histogram for usage of async xhr within pagedismissal.
-      auto pagedismissal = GetDocument()->PageDismissalEventBeingDispatched();
-      if (pagedismissal != Document::kNoDismissal) {
-        UseCounter::Count(&execution_context,
-                          WebFeature::kAsyncXhrInPageDismissal);
-        DEFINE_STATIC_LOCAL(EnumerationHistogram,
-                            asyncxhr_pagedismissal_histogram,
-                            ("XHR.Async.PageDismissal", 5));
-        asyncxhr_pagedismissal_histogram.Count(pagedismissal);
-      }
-    }
     if (upload_)
       request.SetReportUploadProgress(true);
 
@@ -1136,33 +1125,16 @@ void XMLHttpRequest::CreateRequest(scoped_refptr<EncodedFormData> http_body,
               WebFeature::kXMLHttpRequestSynchronousInSameOriginSubframe);
         }
       }
-      // Update histogram for usage of sync xhr within pagedismissal.
-      auto pagedismissal = GetDocument()->PageDismissalEventBeingDispatched();
-      if (pagedismissal != Document::kNoDismissal) {
-        // Disallow synchronous requests on page dismissal unless enabled by
-        // origin trial or enterprise policy.
-        if (!RuntimeEnabledFeatures::AllowSyncXHRInPageDismissalEnabled(
+      if (MainThreadDisallowSynchronousXHRScope::
+              ShouldDisallowSynchronousXHR() &&
+          !RuntimeEnabledFeatures::AllowSyncXHRInPageDismissalEnabled(
               &execution_context)) {
-          UseCounter::Count(&execution_context,
-                            WebFeature::kForbiddenSyncXhrInPageDismissal);
-          DEFINE_STATIC_LOCAL(EnumerationHistogram,
-                              forbidden_syncxhr_pagedismissal_histogram,
-                              ("XHR.Sync.PageDismissal_forbidden", 5));
-          forbidden_syncxhr_pagedismissal_histogram.Count(pagedismissal);
         HandleNetworkError();
         ThrowForLoadFailureIfNeeded(exception_state,
                                     "Synchronous XHR in page dismissal. See "
                                     "https://www.chromestatus.com/feature/"
                                     "4664843055398912 for more details.");
         return;
-        } else {
-          UseCounter::Count(&execution_context,
-                            WebFeature::kSyncXhrInPageDismissal);
-          DEFINE_STATIC_LOCAL(EnumerationHistogram,
-                              syncxhr_pagedismissal_histogram,
-                              ("XHR.Sync.PageDismissal", 5));
-          syncxhr_pagedismissal_histogram.Count(pagedismissal);
-        }
       }
     } else {
       DCHECK(execution_context.IsWorkerGlobalScope());

third_party/blink/renderer/platform/runtime_enabled_features.json5

@@ -147,7 +147,6 @@
     {
       name: "AllowSyncXHRInPageDismissal",
       origin_trial_feature_name: "AllowSyncXHRInPageDismissal",
-      status: "experimental",
     },
     {
       name: "AnimationWorklet",

third_party/blink/web_tests/http/tests/xmlhttprequest/resources/xmlhttprequest-in-unload-sync.html

 <html>
 <head>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
 <script>
-window.onunload = function () {
+
+window.onunload = parent.t.step_func(() => {
   try {
-        var url = '1251.html';
+    console.log("trying the synx xhr");
     var xhr = new XMLHttpRequest();
-        xhr.open('GET', url, false);
+    xhr.open("GET", "resources/1251.html", false);
     xhr.send(null);
-        window.parent.completed("PASS: sync XHR completed successfully");
+    assert_unreached('Sync XHR must throw during an unload event');
   } catch (e) {
-        window.parent.completed("FAIL: sync XHR during unload failed: " + e.message);
+    parent.t.done();
   }
-};
-window.onload = function () {
-    window.parent.subframeLoaded();
-};
+});
+
 </script>
 </head>
 <body>

third_party/blink/web_tests/http/tests/xmlhttprequest/xmlhttprequest-unload-sync-expected.txt

-CONSOLE WARNING: line 8: Synchronous XMLHttpRequest on the main thread is deprecated because of its detrimental effects to the end user's experience. For more help, check https://xhr.spec.whatwg.org/.
-Test behavior of sync XHR during unload
-
-PASS: sync XHR completed successfully

third_party/blink/web_tests/http/tests/xmlhttprequest/xmlhttprequest-unload-sync-handler-attached-from-parent.html

+<html>
+<body>
+<p>Test behavior of sync XHR during unload when the unload handler is attached
+   to a child iframe from the parent (crbug.com/1044348).</p>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<script>
+var t = async_test();
+
+onload = () => {
+  t.step_timeout(() => {
+    var iframe = document.getElementById("ifr");
+    iframe.contentWindow.onunload = t.step_func(() => {
+      try {
+        console.log("trying the synx xhr");
+        var xhr = new XMLHttpRequest();
+        xhr.open("GET", "resources/1251.html", false);
+        xhr.send(null);
+        assert_unreached('Sync XHR must throw during an unload event');
+      } catch (e) {
+        t.done();
+      }
+    });
+    iframe.src = "about:blank";
+  }, 1000);
+};
+
+</script>
+<iframe id="ifr" name="ifr" src="resources/test.html"></iframe>
+</body>
+</html>

third_party/blink/web_tests/http/tests/xmlhttprequest/xmlhttprequest-unload-sync.html

 <html>
 <body>
 <p>Test behavior of sync XHR during unload</p>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<iframe id="ifr" name="ifr" src="resources/xmlhttprequest-in-unload-sync.html"></iframe>
 <script>
-var consoleMessages = document.createElement("ul");
-document.body.appendChild(consoleMessages);
-
-if (window.testRunner) {
-    testRunner.waitUntilDone();
-    testRunner.dumpAsText();
-}
+var t = async_test();
 
-function subframeLoaded()
-{
-    document.getElementById('ifr').contentWindow.location.assign("data:text/html,DONE");
-}
+t.step_timeout(() => {
+  var iframe = document.getElementById("ifr");
+  iframe.src = "about:blank";
+}, 1000);
 
-function completed(msg)
-{
-    log(msg);
-    if (window.testRunner)
-        testRunner.notifyDone();
-}
-
-function log(message)
-{
-    var item = document.createElement("li");
-    item.appendChild(document.createTextNode(message));
-    consoleMessages.appendChild(item);
-}
 </script>
-<div id="framediv">
-<iframe id="ifr" name="ifr" src="resources/xmlhttprequest-in-unload-sync.html"></iframe>
-</div>
 </body>
 </html>