From 5364c6acbc679a14e9b0983258cf24eea389ebb7 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Wed, 9 Sep 2020 09:16:03 -0700 Subject: [PATCH] browser(firefox): fix automatic http->https redirect (#3812) browser(firefox): fix automatic http->https redirect Sometimes, Firefox does an automatic http->https redirect without hitting the network (e.g. for http://wikipedia.org). In this case, the http request is very strange: - it does not actually hit the network; - it is never intercepted; - we cannot access its response because there was no actual response. So, we had a bug where: - redirects inherited the original request's listener; - that listener was throwing an error. This lead to the error in the listeners onDataAvailable call chain, and original listener that renders the response was never called, resulting in an empty page. This change: - ignores the original request that did not hit the network; - does not inherit the listener; - adds try/catch around problematic calls. --- browser_patches/firefox/BUILD_NUMBER | 4 +-- .../firefox/juggler/NetworkObserver.js | 36 ++++++++++++++----- .../juggler/protocol/NetworkHandler.js | 5 +-- 3 files changed, 31 insertions(+), 14 deletions(-) diff --git a/browser_patches/firefox/BUILD_NUMBER b/browser_patches/firefox/BUILD_NUMBER index 70cd825f99..a46c4a556b 100644 --- a/browser_patches/firefox/BUILD_NUMBER +++ b/browser_patches/firefox/BUILD_NUMBER @@ -1,2 +1,2 @@ -1169 -Changed: yurys@chromium.org Tue Sep 1 17:07:52 PDT 2020 +1170 +Changed: dgozman@gmail.com Tue Sep 8 22:17:56 PDT 2020 diff --git a/browser_patches/firefox/juggler/NetworkObserver.js b/browser_patches/firefox/juggler/NetworkObserver.js index 03cab787ab..e271468d29 100644 --- a/browser_patches/firefox/juggler/NetworkObserver.js +++ b/browser_patches/firefox/juggler/NetworkObserver.js @@ -131,12 +131,22 @@ class NetworkRequest { this.requestId = httpChannel.channelId + ''; this.navigationId = httpChannel.isMainDocumentChannel ? this.requestId : undefined; + const internalCauseType = this.httpChannel.loadInfo ? this.httpChannel.loadInfo.internalContentPolicyType : Ci.nsIContentPolicy.TYPE_OTHER; + this.channelKey = this.httpChannel.channelId + ':' + internalCauseType; + this._redirectedIndex = 0; - if (redirectedFrom) { + const ignoredRedirect = redirectedFrom && !redirectedFrom._sentOnResponse; + if (ignoredRedirect) { + // We just ignore redirect that did not hit the network before being redirected. + // This happens, for example, for automatic http->https redirects. + this.navigationId = redirectedFrom.navigationId; + this.channelKey = redirectedFrom.channelKey; + } else if (redirectedFrom) { this.redirectedFromId = redirectedFrom.requestId; this._redirectedIndex = redirectedFrom._redirectedIndex + 1; this.requestId = this.requestId + '-redirect' + this._redirectedIndex; this.navigationId = redirectedFrom.navigationId; + this.channelKey = redirectedFrom.channelKey; // Finish previous request now. Since we inherit the listener, we could in theory // use onStopRequest, but that will only happen after the last redirect has finished. redirectedFrom._sendOnRequestFinished(); @@ -157,7 +167,7 @@ class NetworkRequest { httpChannel.QueryInterface(Ci.nsITraceableChannel); this._originalListener = httpChannel.setNewListener(this); - if (redirectedFrom && this._originalListener === redirectedFrom) { + if (redirectedFrom) { // Listener is inherited for regular redirects, so we'd like to avoid // calling into previous NetworkRequest. this._originalListener = redirectedFrom._originalListener; @@ -492,7 +502,7 @@ class NetworkRequest { navigationId: this.navigationId, cause: causeTypeToString(causeType), internalCause: causeTypeToString(internalCauseType), - }, this.httpChannel.channelId + ':' + internalCauseType); + }, this.channelKey); } _sendOnResponse(fromCache) { @@ -506,10 +516,20 @@ class NetworkRequest { return; this.httpChannel.QueryInterface(Ci.nsIHttpChannelInternal); + const headers = []; - this.httpChannel.visitResponseHeaders({ - visitHeader: (name, value) => headers.push({name, value}), - }); + let status = 0; + let statusText = ''; + try { + status = this.httpChannel.responseStatus; + statusText = this.httpChannel.responseStatusText; + this.httpChannel.visitResponseHeaders({ + visitHeader: (name, value) => headers.push({name, value}), + }); + } catch (e) { + // Response headers, status and/or statusText are not available + // when redirect did not actually hit the network. + } let remoteIPAddress = undefined; let remotePort = undefined; @@ -527,8 +547,8 @@ class NetworkRequest { headers, remoteIPAddress, remotePort, - status: this.httpChannel.responseStatus, - statusText: this.httpChannel.responseStatusText, + status, + statusText, }); } diff --git a/browser_patches/firefox/juggler/protocol/NetworkHandler.js b/browser_patches/firefox/juggler/protocol/NetworkHandler.js index 9df5817f3c..49e2fe3f01 100644 --- a/browser_patches/firefox/juggler/protocol/NetworkHandler.js +++ b/browser_patches/firefox/juggler/protocol/NetworkHandler.js @@ -23,7 +23,6 @@ class NetworkHandler { this._requestInterception = false; this._eventListeners = []; this._pendingRequstWillBeSentEvents = new Set(); - this._requestIdToFrameId = new Map(); } async enable() { @@ -126,9 +125,7 @@ class NetworkHandler { this._pendingRequstWillBeSentEvents.delete(pendingRequestPromise); return; } - // Inherit frameId for redirects when details are not available. - const frameId = details ? details.frameId : (eventDetails.redirectedFrom ? this._requestIdToFrameId.get(eventDetails.redirectedFrom) : undefined); - this._requestIdToFrameId.set(eventDetails.requestId, frameId); + const frameId = details ? details.frameId : undefined; const activity = this._ensureHTTPActivity(eventDetails.requestId); activity.request = { frameId,