Bug 918703 - Part 1: Remove MaybeDispatchProgressEvents() and have its callsites only do what they need to do, to make the XHR code more readable. r=baku
authorThomas Wisniewski <wisniewskit@gmail.com>
Wed, 03 Aug 2016 21:58:17 -0400
changeset 308656 647a26ff50125c5ac3c45dfa78dfc96add0d0f1e
parent 308655 e2bb8f329f102546bf9981e5a6a3d4975f384468
child 308657 3b2b06d9c908ddbc48a4e3e0e3c35b45b1998c8e
push id30547
push usercbook@mozilla.com
push dateTue, 09 Aug 2016 13:45:15 +0000
treeherdermozilla-central@6cf0089510fa [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbaku
bugs918703
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 918703 - Part 1: Remove MaybeDispatchProgressEvents() and have its callsites only do what they need to do, to make the XHR code more readable. r=baku
dom/xhr/XMLHttpRequestMainThread.cpp
dom/xhr/XMLHttpRequestMainThread.h
--- a/dom/xhr/XMLHttpRequestMainThread.cpp
+++ b/dom/xhr/XMLHttpRequestMainThread.cpp
@@ -1301,30 +1301,47 @@ XMLHttpRequestMainThread::DispatchProgre
 {
   NS_ASSERTION(aTarget, "null target");
 
   if (NS_FAILED(CheckInnerWindowCorrectness()) ||
       (!AllowUploadProgress() && aTarget == mUpload)) {
     return;
   }
 
+  if (aType == ProgressEventType::progress) {
+    mInLoadProgressEvent = true;
+  }
+
   ProgressEventInit init;
   init.mBubbles = false;
   init.mCancelable = false;
   init.mLengthComputable = aLengthComputable;
   init.mLoaded = aLoaded;
   init.mTotal = (aTotal == -1) ? 0 : aTotal;
 
   const nsAString& typeString = ProgressEventTypeStrings[(uint8_t)aType];
   RefPtr<ProgressEvent> event =
     ProgressEvent::Constructor(aTarget, typeString, init);
   event->SetTrusted(true);
 
   aTarget->DispatchDOMEvent(nullptr, event, nullptr, nullptr);
 
+  if (aType == ProgressEventType::progress) {
+    mInLoadProgressEvent = false;
+
+    // clear chunked responses after every progress event
+    if (mResponseType == XMLHttpRequestResponseType::Moz_chunked_text ||
+        mResponseType == XMLHttpRequestResponseType::Moz_chunked_arraybuffer) {
+      mResponseBody.Truncate();
+      mResponseText.Truncate();
+      mResultArrayBuffer = nullptr;
+      mArrayBufferBuilder.reset();
+    }
+  }
+
   // If we're sending a load, error, timeout or abort event, then
   // also dispatch the subsequent loadend event.
   if (aType == ProgressEventType::load || aType == ProgressEventType::error ||
       aType == ProgressEventType::timeout || aType == ProgressEventType::abort) {
     DispatchProgressEvent(aTarget, ProgressEventType::loadend,
                           aLengthComputable, aLoaded, aTotal);
   }
 }
@@ -1343,16 +1360,23 @@ XMLHttpRequestMainThread::GetCurrentJARC
   return appChannel.forget();
 }
 
 bool
 XMLHttpRequestMainThread::IsSystemXHR() const
 {
   return mIsSystem || nsContentUtils::IsSystemPrincipal(mPrincipal);
 }
+ 
+bool
+XMLHttpRequestMainThread::InUploadPhase() const
+{
+  // We're in the upload phase while our state is State::opened.
+  return mState == State::opened;
+}
 
 NS_IMETHODIMP
 XMLHttpRequestMainThread::Open(const nsACString& aMethod, const nsACString& aUrl,
                                bool aAsync, const nsAString& aUsername,
                                const nsAString& aPassword, uint8_t optional_argc)
 {
   Optional<bool> async;
   if (!optional_argc) {
@@ -1675,17 +1699,19 @@ XMLHttpRequestMainThread::OnDataAvailabl
     ChangeState(State::loading);
     return request->Cancel(NS_OK);
   }
 
   mDataAvailable += totalRead;
 
   ChangeState(State::loading);
 
-  MaybeDispatchProgressEvents(false);
+  if (!mFlagSynchronous && !mProgressTimerIsActive) {
+    StartProgressEventTimer();
+  }
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 XMLHttpRequestMainThread::OnStartRequest(nsIRequest *request, nsISupports *ctxt)
 {
   PROFILER_LABEL("XMLHttpRequestMainThread", "OnStartRequest",
@@ -1722,27 +1748,30 @@ XMLHttpRequestMainThread::OnStartRequest
 
   nsCOMPtr<nsIChannel> channel(do_QueryInterface(request));
   NS_ENSURE_TRUE(channel, NS_ERROR_UNEXPECTED);
 
   nsresult status;
   request->GetStatus(&status);
   mErrorLoad = mErrorLoad || NS_FAILED(status);
 
+  // Upload phase is now over. If we were uploading anything,
+  // stop the timer and fire any final progress events.
   if (mUpload && !mUploadComplete && !mErrorLoad && !mFlagSynchronous) {
-    if (mProgressTimerIsActive) {
-      mProgressTimerIsActive = false;
-      mProgressNotifier->Cancel();
+    StopProgressEventTimer();
+
+    mUploadTransferred = mUploadTotal;
+
+    if (mProgressSinceLastProgressEvent) {
+      DispatchProgressEvent(mUpload, ProgressEventType::progress,
+                            mUploadLengthComputable, mUploadTransferred,
+                            mUploadTotal);
+      mProgressSinceLastProgressEvent = false;
     }
-    if (mUploadTransferred < mUploadTotal) {
-      mUploadTransferred = mUploadTotal;
-      mProgressSinceLastProgressEvent = true;
-      mUploadLengthComputable = true;
-      MaybeDispatchProgressEvents(true);
-    }
+
     mUploadComplete = true;
     DispatchProgressEvent(mUpload, ProgressEventType::load,
                           true, mUploadTotal, mUploadTotal);
   }
 
   mContext = ctxt;
   mFlagParseBody = true;
   ChangeState(State::headers_received);
@@ -1919,20 +1948,18 @@ XMLHttpRequestMainThread::OnStartRequest
     // the spec requires the response document.referrer to be the empty string
     mResponseXML->SetReferrer(NS_LITERAL_CSTRING(""));
 
     mXMLParserStreamListener = listener;
     rv = mXMLParserStreamListener->OnStartRequest(request, ctxt);
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
-  // We won't get any progress events anyway if we didn't have progress
-  // events when starting the request - so maybe no need to start timer here.
-  if (NS_SUCCEEDED(rv) && !mFlagSynchronous &&
-      HasListenersFor(nsGkAtoms::onprogress)) {
+  // Download phase beginning; start the progress event timer if necessary.
+  if (NS_SUCCEEDED(rv) && HasListenersFor(nsGkAtoms::onprogress)) {
     StartProgressEventTimer();
   }
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 XMLHttpRequestMainThread::OnStopRequest(nsIRequest *request, nsISupports *ctxt, nsresult status)
@@ -1965,23 +1992,16 @@ XMLHttpRequestMainThread::OnStopRequest(
   // Is this good enough here?
   if (mXMLParserStreamListener && mFlagParseBody) {
     mXMLParserStreamListener->OnStopRequest(request, ctxt, status);
   }
 
   mXMLParserStreamListener = nullptr;
   mContext = nullptr;
 
-  // If we're received data since the last progress event, make sure to fire
-  // an event for it, except in the HTML case, defer the last progress event
-  // until the parser is done.
-  if (!mIsHtml) {
-    MaybeDispatchProgressEvents(true);
-  }
-
   if (NS_SUCCEEDED(status) &&
       (mResponseType == XMLHttpRequestResponseType::Blob ||
        mResponseType == XMLHttpRequestResponseType::Moz_blob)) {
     if (!mDOMBlob) {
       CreateDOMBlob(request);
     }
     if (mDOMBlob) {
       mResponseBlob = mDOMBlob;
@@ -2068,20 +2088,28 @@ XMLHttpRequestMainThread::OnStopRequest(
   }
   ChangeStateToDone();
   return NS_OK;
 }
 
 void
 XMLHttpRequestMainThread::ChangeStateToDone()
 {
-  if (mIsHtml) {
-    // In the HTML case, this has to be deferred, because the parser doesn't
-    // do it's job synchronously.
-    MaybeDispatchProgressEvents(true);
+  StopProgressEventTimer();
+
+  MOZ_ASSERT(!mFlagParseBody,
+             "ChangeStateToDone() called before async HTML parsing is done.");
+
+  // Fire the final progress event required by spec, if we haven't already.
+  if (mProgressSinceLastProgressEvent && !mErrorLoad && !mFlagSynchronous) {
+    mLoadTotal = mLoadTransferred;
+    DispatchProgressEvent(this, ProgressEventType::progress,
+                          mLoadLengthComputable, mLoadTransferred,
+                          mLoadTotal);
+    mProgressSinceLastProgressEvent = false;
   }
 
   ChangeState(State::done, true);
 
   mFlagSend = false;
 
   if (mTimeoutTimer) {
     mTimeoutTimer->Cancel();
@@ -2805,20 +2833,17 @@ XMLHttpRequestMainThread::SendInternal(c
             suspendedDoc->SuppressEventHandling(nsIDocument::eEvents);
           }
           topWindow->SuspendTimeouts(1, false);
           resumeTimeoutRunnable = new nsResumeTimeoutsEvent(topInner);
         }
       }
     }
 
-    if (mProgressNotifier) {
-      mProgressTimerIsActive = false;
-      mProgressNotifier->Cancel();
-    }
+    StopProgressEventTimer();
 
     {
       nsAutoSyncOperation sync(suspendedDoc);
       nsIThread *thread = NS_GetCurrentThread();
       while (mFlagSyncLooping) {
         if (!NS_ProcessNextEvent(thread)) {
           rv = NS_ERROR_UNEXPECTED;
           break;
@@ -2834,21 +2859,19 @@ XMLHttpRequestMainThread::SendInternal(c
     if (resumeTimeoutRunnable) {
       NS_DispatchToCurrentThread(resumeTimeoutRunnable);
     }
   } else {
     // Now that we've successfully opened the channel, we can change state.  Note
     // that this needs to come after the AsyncOpen() and rv check, because this
     // can run script that would try to restart this request, and that could end
     // up doing our AsyncOpen on a null channel if the reentered AsyncOpen fails.
-    if (mProgressNotifier) {
-      mProgressTimerIsActive = false;
-      mProgressNotifier->Cancel();
-    }
-
+    StopProgressEventTimer();
+
+    // Upload phase beginning; start the progress event timer if necessary.
     if (mUpload && mUpload->HasListenersFor(nsGkAtoms::onprogress)) {
       StartProgressEventTimer();
     }
     DispatchProgressEvent(this, ProgressEventType::loadstart, false, 0, 0);
     if (mUpload && !mUploadComplete) {
       DispatchProgressEvent(mUpload, ProgressEventType::loadstart, true,
                             0, mUploadTotal);
     }
@@ -3116,20 +3139,18 @@ XMLHttpRequestMainThread::SetWithCredent
 }
 
 nsresult
 XMLHttpRequestMainThread::ChangeState(State aState, bool aBroadcast)
 {
   mState = aState;
   nsresult rv = NS_OK;
 
-  if (mProgressNotifier &&
-      aState != State::headers_received && aState != State::loading) {
-    mProgressTimerIsActive = false;
-    mProgressNotifier->Cancel();
+  if (aState != State::headers_received && aState != State::loading) {
+    StopProgressEventTimer();
   }
 
 
   if (aBroadcast && (!mFlagSynchronous ||
                      aState == State::opened ||
                      aState == State::done)) {
     rv = FireReadystatechangeEvent();
   }
@@ -3220,88 +3241,41 @@ XMLHttpRequestMainThread::OnRedirectVeri
 
   return result;
 }
 
 /////////////////////////////////////////////////////
 // nsIProgressEventSink methods:
 //
 
-void
-XMLHttpRequestMainThread::MaybeDispatchProgressEvents(bool aFinalProgress)
-{
-  if (aFinalProgress && mProgressTimerIsActive) {
-    mProgressTimerIsActive = false;
-    mProgressNotifier->Cancel();
-  }
-
-  if (mProgressTimerIsActive ||
-      !mProgressSinceLastProgressEvent ||
-      mErrorLoad ||
-      mFlagSynchronous) {
-    return;
-  }
-
-  if (!aFinalProgress) {
-    StartProgressEventTimer();
-  }
-
-  // We're in the upload phase while our state is State::opened.
-  if (mState == State::opened) {
-    if (mUpload && !mUploadComplete) {
-      DispatchProgressEvent(mUpload, ProgressEventType::progress,
-                            mUploadLengthComputable, mUploadTransferred,
-                            mUploadTotal);
-    }
-  } else {
-    if (aFinalProgress) {
-      mLoadTotal = mLoadTransferred;
-    }
-    mInLoadProgressEvent = true;
-    DispatchProgressEvent(this, ProgressEventType::progress,
-                          mLoadLengthComputable, mLoadTransferred,
-                          mLoadTotal);
-    mInLoadProgressEvent = false;
-    if (mResponseType == XMLHttpRequestResponseType::Moz_chunked_text ||
-        mResponseType == XMLHttpRequestResponseType::Moz_chunked_arraybuffer) {
-      mResponseBody.Truncate();
-      mResponseText.Truncate();
-      mResultArrayBuffer = nullptr;
-      mArrayBufferBuilder.reset();
-    }
-  }
-
-  mProgressSinceLastProgressEvent = false;
-}
-
 NS_IMETHODIMP
 XMLHttpRequestMainThread::OnProgress(nsIRequest *aRequest, nsISupports *aContext, int64_t aProgress, int64_t aProgressMax)
 {
-  // We're in the upload phase while our state is State::opened.
-  bool upload = mState == State::opened;
   // When uploading, OnProgress reports also headers in aProgress and aProgressMax.
   // So, try to remove the headers, if possible.
   bool lengthComputable = (aProgressMax != -1);
-  if (upload) {
+  if (InUploadPhase()) {
     int64_t loaded = aProgress;
     if (lengthComputable) {
       int64_t headerSize = aProgressMax - mUploadTotal;
       loaded -= headerSize;
     }
     mUploadLengthComputable = lengthComputable;
     mUploadTransferred = loaded;
     mProgressSinceLastProgressEvent = true;
 
-    MaybeDispatchProgressEvents((mUploadTransferred == mUploadTotal));
+    if (!mFlagSynchronous && !mProgressTimerIsActive) {
+      StartProgressEventTimer();
+    }
   } else {
     mLoadLengthComputable = lengthComputable;
     mLoadTotal = lengthComputable ? aProgressMax : 0;
     mLoadTransferred = aProgress;
-    // Don't dispatch progress events here. OnDataAvailable will take care
-    // of that.
+    // OnDataAvailable() handles mProgressSinceLastProgressEvent
+    // for the download phase.
   }
 
   if (mProgressEventSink) {
     mProgressEventSink->OnProgress(aRequest, aContext, aProgress,
                                    aProgressMax);
   }
 
   return NS_OK;
@@ -3485,17 +3459,45 @@ XMLHttpRequestMainThread::Notify(nsITime
   NS_WARNING("Unexpected timer!");
   return NS_ERROR_INVALID_POINTER;
 }
 
 void
 XMLHttpRequestMainThread::HandleProgressTimerCallback()
 {
   mProgressTimerIsActive = false;
-  MaybeDispatchProgressEvents(false);
+
+  if (!mProgressSinceLastProgressEvent || mErrorLoad) {
+    return;
+  }
+
+  if (InUploadPhase()) {
+    if (mUpload && !mUploadComplete) {
+      DispatchProgressEvent(mUpload, ProgressEventType::progress,
+                            mUploadLengthComputable, mUploadTransferred,
+                            mUploadTotal);
+    }
+  } else {
+    DispatchProgressEvent(this, ProgressEventType::progress,
+                          mLoadLengthComputable, mLoadTransferred,
+                          mLoadTotal);
+  }
+
+  mProgressSinceLastProgressEvent = false;
+
+  StartProgressEventTimer();
+}
+
+void
+XMLHttpRequestMainThread::StopProgressEventTimer()
+{
+  if (mProgressNotifier) {
+    mProgressTimerIsActive = false;
+    mProgressNotifier->Cancel();
+  }
 }
 
 void
 XMLHttpRequestMainThread::StartProgressEventTimer()
 {
   if (!mProgressNotifier) {
     mProgressNotifier = do_CreateInstance(NS_TIMER_CONTRACTID);
   }
--- a/dom/xhr/XMLHttpRequestMainThread.h
+++ b/dom/xhr/XMLHttpRequestMainThread.h
@@ -470,21 +470,16 @@ public:
   // This fires a trusted readystatechange event, which is not cancelable and
   // doesn't bubble.
   nsresult FireReadystatechangeEvent();
   void DispatchProgressEvent(DOMEventTargetHelper* aTarget,
                              const ProgressEventType aType,
                              bool aLengthComputable,
                              int64_t aLoaded, int64_t aTotal);
 
-  // Dispatch the "progress" event on the XHR or XHR.upload object if we've
-  // received data since the last "progress" event. Also dispatches
-  // "uploadprogress" as needed.
-  void MaybeDispatchProgressEvents(bool aFinalProgress);
-
   // This is called by the factory constructor.
   nsresult Init();
 
   nsresult init(nsIPrincipal* principal,
                 nsPIDOMWindowInner* globalObject,
                 nsIURI* baseURI);
 
   void SetRequestObserver(nsIRequestObserver* aObserver);
@@ -531,20 +526,22 @@ protected:
   nsresult ChangeState(State aState, bool aBroadcast = true);
   already_AddRefed<nsILoadGroup> GetLoadGroup() const;
   nsIURI *GetBaseURI();
 
   already_AddRefed<nsIHttpChannel> GetCurrentHttpChannel();
   already_AddRefed<nsIJARChannel> GetCurrentJARChannel();
 
   bool IsSystemXHR() const;
+  bool InUploadPhase() const;
 
   void ChangeStateToDone();
 
   void StartProgressEventTimer();
+  void StopProgressEventTimer();
 
   nsresult OnRedirectVerifyCallback(nsresult result);
 
   nsresult OpenInternal(const nsACString& aMethod,
                         const nsACString& aUrl,
                         const Optional<bool>& aAsync,
                         const Optional<nsAString>& aUsername,
                         const Optional<nsAString>& aPassword);