Bug 918719 - Only fire one loading readystatechange per XHR, but keep the old behavior available behind the preference dom.send_multiple_xhr_loading_readystatechanges. r=smaug a=smaug
☠☠ backed out by af1a3c41d8c1 ☠ ☠
authorThomas Wisniewski <wisniewskit@gmail.com>
Thu, 08 Sep 2016 12:01:58 -0400
changeset 354641 ec584a62ec2689226457b1679c3a5802637dcd3c
parent 354640 6e5c2daac4d39c1394dad3089f35c3432e4580bf
child 354642 c526dd176ca348e632559715bad25ae0380df04e
push id6570
push userraliiev@mozilla.com
push dateMon, 14 Nov 2016 12:26:13 +0000
treeherdermozilla-beta@f455459b2ae5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug, smaug
bugs918719
milestone51.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 918719 - Only fire one loading readystatechange per XHR, but keep the old behavior available behind the preference dom.send_multiple_xhr_loading_readystatechanges. r=smaug a=smaug
dom/tests/mochitest/bugs/bug918719.sjs
dom/tests/mochitest/bugs/mochitest.ini
dom/tests/mochitest/bugs/test_bug918719.html
dom/xhr/XMLHttpRequestMainThread.cpp
dom/xhr/XMLHttpRequestMainThread.h
modules/libpref/init/all.js
testing/web-platform/meta/XMLHttpRequest/event-readystatechange-loaded.htm.ini
new file mode 100644
--- /dev/null
+++ b/dom/tests/mochitest/bugs/bug918719.sjs
@@ -0,0 +1,13 @@
+// Send a payload that's over 32k in size, which for a plain response should be
+// large enough to ensure that OnDataAvailable is called more than once (and so
+// the XHR will be triggered to send more than one "loading" event if desired).
+
+function handleRequest(request, response)
+{
+  let data = (new Array(1 << 18)).join("A");
+  response.processAsync();
+  response.setHeader("Content-Type", "text/plain", false);
+  response.setHeader("Content-Length", "" + data.length, false);
+  response.write(data, data.length);
+  response.finish();
+}
--- a/dom/tests/mochitest/bugs/mochitest.ini
+++ b/dom/tests/mochitest/bugs/mochitest.ini
@@ -2,16 +2,17 @@
 support-files =
   bug289714.sjs
   bug346659-echoer.html
   bug346659-opener-echoer.html
   bug346659-opener.html
   bug346659-parent-echoer.html
   bug346659-parent.html
   bug458091_child.html
+  bug918719.sjs
   child_bug260264.html
   devicemotion_inner.html
   devicemotion_outer.html
   file_bug291653.html
   file_bug406375.html
   file_bug504862.html
   file_bug593174_1.html
   file_bug593174_2.html
@@ -150,16 +151,17 @@ skip-if = (buildapp == 'b2g' && toolkit 
 [test_bug809290.html]
 [test_bug817476.html]
 [test_bug823173.html]
 [test_bug848088.html]
 [test_bug850517.html]
 [test_bug857555.html]
 [test_bug862540.html]
 [test_bug876098.html]
+[test_bug918719.html]
 [test_bug927901.html]
 [test_devicemotion_multiple_listeners.html]
 skip-if = toolkit == 'android' #bug 775227
 [test_domparser_after_blank.html]
 [test_errorReporting.html]
 [test_onerror_message.html]
 [test_protochains.html]
 [test_resize_move_windows.html]
new file mode 100644
--- /dev/null
+++ b/dom/tests/mochitest/bugs/test_bug918719.html
@@ -0,0 +1,62 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=918719
+-->
+<head>
+  <title>Test for Bug 918719</title>
+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=918719">Mozilla Bug 918719</a>
+<p id="display"></p>
+<div id="content" style="display: none">
+
+</div>
+<pre id="test">
+<script class="testbody" type="text/javascript">
+
+SimpleTest.waitForExplicitFinish();
+
+function runTest() {
+  return new Promise(function(resolve) {
+    let loadingEventCount = 0,
+        xhr = new XMLHttpRequest();
+    xhr.open("GET", "bug918719.sjs");
+    xhr.onreadystatechange = function() {
+      if (xhr.readyState == 3) {
+        loadingEventCount++;
+      } else if (xhr.readyState == 4) {
+        resolve(loadingEventCount);
+      }
+    }
+    xhr.send();
+  });
+}
+
+function prefChangePromise(args) {
+  return new Promise(function(resolve) {
+    SpecialPowers.pushPrefEnv(args, resolve);
+  });
+}
+
+runTest().then(function(count) {
+  ok(count === 1, "Only one loading readystatechange event should have been fired with the pref off.");
+}).then(function() {
+  return prefChangePromise({"set": [["dom.fire_extra_xhr_loading_readystatechanges", true]]});
+}).then(function() {
+  return runTest();
+}).then(function(count) {
+  // Note this case is tricky to test, as there is no hard guarantee that the
+  // server will send the data in more than one chunk, even with the trick
+  // we're using in the sjs. As such this case *may* intermittently fail.
+  ok(count > 1, "Multiple loading readystatechange events should have been fired with the pref on.");
+  SimpleTest.finish();
+});
+
+</script>
+</pre>
+</body>
+</html>
+
--- a/dom/xhr/XMLHttpRequestMainThread.cpp
+++ b/dom/xhr/XMLHttpRequestMainThread.cpp
@@ -164,16 +164,17 @@ XMLHttpRequestMainThread::XMLHttpRequest
   : mResponseBodyDecodedPos(0),
     mResponseType(XMLHttpRequestResponseType::_empty),
     mRequestObserver(nullptr),
     mState(State::unsent),
     mFlagSynchronous(false), mFlagAborted(false), mFlagParseBody(false),
     mFlagSyncLooping(false), mFlagBackgroundRequest(false),
     mFlagHadUploadListenersOnSend(false), mFlagACwithCredentials(false),
     mFlagTimedOut(false), mFlagDeleted(false), mFlagSend(false),
+    mSendExtraLoadingEvents(Preferences::GetBool("dom.fire_extra_xhr_loading_readystatechanges", true)),
     mUploadTransferred(0), mUploadTotal(0), mUploadComplete(true),
     mProgressSinceLastProgressEvent(false),
     mRequestSentTime(0), mTimeoutMilliseconds(0),
     mErrorLoad(false), mErrorParsingXML(false),
     mWaitingForOnStopRequest(false),
     mProgressTimerIsActive(false),
     mIsHtml(false),
     mWarnAboutSyncHtml(false),
@@ -1714,17 +1715,19 @@ XMLHttpRequestMainThread::OnDataAvailabl
     }
 
     ChangeState(State::loading);
     return request->Cancel(NS_OK);
   }
 
   mDataAvailable += totalRead;
 
-  ChangeState(State::loading);
+  if (mState == State::headers_received || mSendExtraLoadingEvents) {
+    ChangeState(State::loading);
+  }
 
   if (!mFlagSynchronous && !mProgressTimerIsActive) {
     StartProgressEventTimer();
   }
 
   return NS_OK;
 }
 
--- a/dom/xhr/XMLHttpRequestMainThread.h
+++ b/dom/xhr/XMLHttpRequestMainThread.h
@@ -691,16 +691,24 @@ protected:
   bool mFlagDeleted;
 
   // The XHR2 spec's send() flag. Set when the XHR begins uploading, until it
   // finishes downloading (or an error/abort has occurred during either phase).
   // Used to guard against the user trying to alter headers/etc when it's too
   // late, and ensure the XHR only handles one in-flight request at once.
   bool mFlagSend;
 
+  // Before ProgressEvents were a thing, multiple readystatechange events were
+  // fired during the loading state to give sites a way to monitor XHR progress.
+  // The XHR spec now has proper progress events and dictates that only one
+  // "loading" readystatechange should be fired per send. However, it's possible
+  // that some content still relies on this old behavior, so we're keeping it
+  // (behind a preference) for now. See bug 918719.
+  bool mSendExtraLoadingEvents;
+
   RefPtr<XMLHttpRequestUpload> mUpload;
   int64_t mUploadTransferred;
   int64_t mUploadTotal;
   bool mUploadComplete;
   bool mProgressSinceLastProgressEvent;
 
   // Timeout support
   PRTime mRequestSentTime;
--- a/modules/libpref/init/all.js
+++ b/modules/libpref/init/all.js
@@ -5075,16 +5075,20 @@ pref("dom.voicemail.enabled", false);
 #endif
 // Numeric default service id for Voice Mail API calls with |serviceId|
 // parameter omitted.
 pref("dom.voicemail.defaultServiceId", 0);
 
 // Enable mapped array buffer by default.
 pref("dom.mapped_arraybuffer.enabled", true);
 
+// Whether to send more than one "loading" readystatechange during XHRs to
+// simulate progress events for sites still not using modern progress events.
+pref("dom.fire_extra_xhr_loading_readystatechanges", false);
+
 // The tables used for Safebrowsing phishing and malware checks.
 pref("urlclassifier.malwareTable", "goog-malware-shavar,goog-unwanted-shavar,test-malware-simple,test-unwanted-simple");
 
 #ifdef MOZILLA_OFFICIAL
 // In the official build, we are allowed to use google's private
 // phishing list "goog-phish-shavar". See Bug 1288840.
 pref("urlclassifier.phishTable", "goog-phish-shavar,test-phish-simple");
 #else
deleted file mode 100644
--- a/testing/web-platform/meta/XMLHttpRequest/event-readystatechange-loaded.htm.ini
+++ /dev/null
@@ -1,5 +0,0 @@
-[event-readystatechange-loaded.htm]
-  type: testharness
-  [XMLHttpRequest: the LOADING state change should only happen once]
-    expected: FAIL
-