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
authorThomas Wisniewski <wisniewskit@gmail.com>
Mon, 12 Sep 2016 22:39:01 -0400
changeset 313749 4bdbbae12cb345de1d0dd52759a1824d031c7f91
parent 313748 facf5812faffa05e6037e96c92a5835d12937966
child 313750 876a4319c262bff844529614b5343e634d0e3947
push id81703
push userryanvm@gmail.com
push dateWed, 14 Sep 2016 02:22:54 +0000
treeherdermozilla-inbound@8639daa019b6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
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
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,47 @@
+// Keep track of one in-flight XHR by allowing secondary ones to
+// tell us when the client has received a Progress event and wants
+// more data or to stop the in-flight XHR.
+
+const STATE_NAME = "bug918719_loading_event_test";
+const CHUNK_DATA = "chunk";
+
+function setReq(req) {
+  setObjectState(STATE_NAME, req);
+}
+
+function getReq() {
+  let req;
+  getObjectState(STATE_NAME, function(v) {
+    req = v;
+  });
+  return req;
+}
+
+function handleRequest(request, response)
+{
+  var pairs = request.queryString.split("&");
+  var command = pairs.shift();
+
+  response.setHeader("Content-Type", "text/plain");
+  response.setHeader("Cache-Control", "no-cache", false);
+
+  switch(command) {
+    case "more":
+      getReq().write(CHUNK_DATA);
+      break;
+
+    case "done":
+      getReq().finish();
+      setReq(null);
+      break;
+
+    default:
+      response.processAsync();
+      response.write(CHUNK_DATA);
+      setReq(response);
+      return;
+  }
+
+  response.setHeader("Content-Length", "2", false);
+  response.write("ok");
+}
--- 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,89 @@
+<!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();
+
+const SERVER_URL = "bug918719.sjs";
+
+function sendCommand(cmd) {
+  let xhr = new XMLHttpRequest();
+  xhr.open("get", SERVER_URL + "?" + cmd);
+  xhr.send();
+}
+
+function runTest() {
+  // Manipulate one in-flight XHR using secondary command XHRs, to guarantee
+  // that multiple OnDataAvailable events are triggered (which are where
+  // LOADING readystatechanges are triggered). We return a promise that will
+  // resolve with a count of the number of LOADING events that were detected.
+
+  return new Promise((resolve, reject) => {
+    let xhr = new XMLHttpRequest();
+    let numProgressEvents = 0;
+    let numLoadingEvents = 0;
+
+    xhr.onreadystatechange = e => {
+      if (xhr.readyState === xhr.LOADING) {
+        ++numLoadingEvents;
+      }
+    };
+
+    xhr.onprogress = e => {
+      if (++numProgressEvents < 2) {
+        sendCommand("more");
+      } else {
+        sendCommand("done");
+      }
+    };
+
+    xhr.onerror = e => {
+      reject(e);
+    };
+
+    xhr.onloadend = e => {
+      resolve(numLoadingEvents);
+    };
+
+    xhr.open("GET", SERVER_URL);
+    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) {
+  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),
@@ -1726,17 +1727,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
@@ -698,16 +698,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
@@ -5071,16 +5071,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
-