Bug 1402689 - Clear tab's "busy" attribute on LOCATION_CHANGE_ERROR_PAGE. r?dao draft
authorSamael Wang <freesamael@gmail.com>
Mon, 30 Oct 2017 17:30:56 +0800
changeset 688585 7d1a3294ada226e5ce7c715afeb0b7e877014488
parent 687186 aa958b29c149a67fce772f8473e9586e71fbdb46
child 738109 cc9b4553d84f6a7942bd97e3dfd74e9c2d4218a0
push id86793
push userbmo:sawang@mozilla.com
push dateMon, 30 Oct 2017 09:42:47 +0000
reviewersdao
bugs1402689
milestone58.0a1
Bug 1402689 - Clear tab's "busy" attribute on LOCATION_CHANGE_ERROR_PAGE. r?dao If the busy attribute was not set by (STATE_IS_NETWORK & STATE_START), and the loading results in an error, it's possible that the busy flag never gets cleared. In this case we can clear it when receiving location change with LOCATION_CHANGE_ERROR_PAGE. When digging into this bug I found nsBrowserStatusFilter::mFinishedRequests and mTotalRequests counters can be incorrect on parent side since we've applied the filter on both child and parent processes. This hasn't seem to be causing a bug yet but it's potentially problematic, so adding a warning in nsBrowserStatusFilter. MozReview-Commit-ID: H25R60ozGtM
browser/base/content/tabbrowser.xml
toolkit/components/statusfilter/nsBrowserStatusFilter.cpp
--- a/browser/base/content/tabbrowser.xml
+++ b/browser/base/content/tabbrowser.xml
@@ -862,16 +862,28 @@
                 // navigation initiated by the user.
                 if (this.mBrowser.didStartLoadSinceLastUserTyping() ||
                     ((aFlags & Ci.nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE) &&
                      aLocation.spec != "about:blank") ||
                     (isSameDocument && this.mBrowser.inLoadURI)) {
                   this.mBrowser.userTypedValue = null;
                 }
 
+                // If the tab has been set to "busy" outside the statChange
+                // handler below (e.g. by sessionStore.navigateAndRestore), and
+                // the load results in an error page, it's possible that there
+                // isn't any (STATE_IS_NETWORK & STATE_STOP) state to cause busy
+                // attribute being removed. In this case we should remove the
+                // attribute here.
+                if ((aFlags & Ci.nsIWebProgressListener.LOCATION_CHANGE_ERROR_PAGE) &&
+                    this.mTab.hasAttribute("busy")) {
+                  this.mTab.removeAttribute("busy");
+                  this.mTabBrowser._tabAttrModified(this.mTab, ["busy"]);
+                }
+
                 // If the browser was playing audio, we should remove the playing state.
                 if (this.mTab.hasAttribute("soundplaying") && !isSameDocument) {
                   clearTimeout(this.mTab._soundPlayingAttrRemovalTimer);
                   this.mTab._soundPlayingAttrRemovalTimer = 0;
                   this.mTab.removeAttribute("soundplaying");
                   this.mTabBrowser._tabAttrModified(this.mTab, ["soundplaying"]);
                 }
 
--- a/toolkit/components/statusfilter/nsBrowserStatusFilter.cpp
+++ b/toolkit/components/statusfilter/nsBrowserStatusFilter.cpp
@@ -177,16 +177,22 @@ nsBrowserStatusFilter::OnStateChange(nsI
         return NS_OK;
     } else {
         // no need to forward this state change
         return NS_OK;
     }
 
     // If we're here, we have either STATE_START or STATE_STOP.  The
     // listener only cares about these in certain conditions.
+    //
+    // XXX The filter is applied in both parent and child processes, but the
+    // condition below doesn't guarantee STATE_START and STATE_STOP of
+    // STATE_IS_REQUEST are delivered in pairs, meaning that mFinishedRequests
+    // can exceeds mTotalRequests on parent side. This is potentially
+    // problematic.
     bool isLoadingDocument = false;
     if ((aStateFlags & nsIWebProgressListener::STATE_IS_NETWORK ||
          (aStateFlags & nsIWebProgressListener::STATE_IS_REQUEST &&
           mFinishedRequests == mTotalRequests &&
           NS_SUCCEEDED(aWebProgress->GetIsLoadingDocument(&isLoadingDocument)) &&
           !isLoadingDocument))) {
         if (mTimer && (aStateFlags & nsIWebProgressListener::STATE_STOP)) {
             mTimer->Cancel();