See https://developer.chrome.com/extensions/webRequest#event-onBeforeRedirect Without this we are prevented from tracking a request lifecycle reliably, because onCompleted cannot be fired for redirected requests (no stream listener to wrap), so we can never know if they're done or not yet. Implementation should not be hard, see https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIChannelEventSink

onBeforeRedirect implementation + tests for both WebRequest.jsm and ext_webRequest.

Comment on attachment 8693855 [details] [diff] [review] webRequest.onBeforeRedirect.patch Review of attachment 8693855 [details] [diff] [review]: ----------------------------------------------------------------- Thanks very much. I found some very small syntax nits and also (maybe) a problem with onCompleted firing for redirected requests. Let me know if I misunderstood something. ::: toolkit/components/extensions/test/mochitest/test_ext_webrequest.html @@ +27,5 @@ > BASE + "/file_script_redirect.js", > BASE + "/file_script_xhr.js", > BASE + "/file_WebRequest_page2.html", > BASE + "/nonexistent_script_url.js", > + BASE + "/redirection.sjs", Do you know why we don't get dummy_page.html here? @@ +54,5 @@ > BASE + "/file_script_good.js", > BASE + "/file_script_xhr.js", > BASE + "/file_WebRequest_page2.html", > BASE + "/nonexistent_script_url.js", > + BASE + "/redirection.sjs", According to the Chrome docs, I think we should not fire onCompleted for redirected requests. Maybe this is happening because of the onStop issue below? ::: toolkit/modules/addons/WebRequest.jsm @@ +174,5 @@ > + }, > + > + // nsIChannelEventSink implementation > + asyncOnChannelRedirect(oldChannel, newChannel, flags, redirectCallback) { > + Services.tm.currentThread.dispatch(() => redirectCallback.onRedirectVerifyCallback(0), Ci.nsIEventTarget.DISPATCH_NORMAL); Cr.NS_OK instead of 0 please. @@ +181,5 @@ > + } catch (e) { > + // we don't wanna throw: it would abort the redirection > + } > + }, > + There are some spaces in this line. Please remove the spaces. @@ +184,5 @@ > + }, > + > + // nsIFactory implementation > + createInstance(outer, iid) { > + if (outer) throw Cr.NS_ERROR_NO_AGGREGATION; This should be: if (outer) { throw Cr.NS_ERROR_NO_AGGREGATION; } @@ +230,5 @@ > Services.obs.removeObserver(this, "http-on-examine-cached-response"); > Services.obs.removeObserver(this, "http-on-examine-merged-response"); > } > + > + if (this.listeners.onRedirect.size) { Could you do this in the style of the ones above? I think they're easier to read, if less efficient: if (needRedirect && !this.redirectInitialized) { ... } else if (!needRedirect && this.redirectInitialized) { ... } @@ +238,5 @@ > + } > + } else if (this.redirectInitialized) { > + this.redirectInitialized = false; > + ChannelEventSink.unregister(); > + } There's an extra space at the end of the line. @@ +310,5 @@ > > let requestHeaders; > let responseHeaders; > > + let includeStatus = kind === "headersReceived" || Extra space at the end. @@ +312,5 @@ > let responseHeaders; > > + let includeStatus = kind === "headersReceived" || > + kind === "onBeforeRedirect" || > + kind === "onStart" || Same. @@ +328,5 @@ > type: WebRequestCommon.typeForPolicyType(policyType), > windowId: loadInfo ? loadInfo.outerWindowID : 0, > parentWindowId: loadInfo ? loadInfo.parentOuterWindowID : 0, > }; > + if (extraData) Object.assign(data, extraData); if (extraData) { ... } @@ +413,5 @@ > }, > > + onChannelReplaced(oldChannel, newChannel) { > + this.runChannelListener(oldChannel, this.getLoadContext(oldChannel), > + "onRedirect", { redirectUrl: newChannel.URI.spec }); this.runChannelListener(oldChannel, this.getLoadContext(oldChannel), "onRedirect", {redirectUrl: newChannel.URI.spec}); Also, I'm worried about the StartStopListener added in |examine|. I think maybe we want to add it to the new channel and remove it from the old channel?

(In reply to Bill McCloskey (:billm) from comment #2) > Review of attachment 8693855 [details] [diff] [review]: > ----------------------------------------------------------------- > [...] > ::: toolkit/components/extensions/test/mochitest/test_ext_webrequest.html > @@ +27,5 @@ > > BASE + "/file_script_redirect.js", > > BASE + "/file_script_xhr.js", > > BASE + "/file_WebRequest_page2.html", > > BASE + "/nonexistent_script_url.js", > > + BASE + "/redirection.sjs", > > Do you know why we don't get dummy_page.html here? > Yes I do: redirections never go through content policies. As soon as we start using HTTP observer for onBeforeRequest as well we're going to see it called for the destination of redirections as well. In facts, bug 1163862 is next in my pipeline as it's gonna fix a lot of inconsistencies (but also introduces problems, such as how we trace/block non-HTTP loads? I'm tempted of adding an "onLoadAttempt" event for such cases, still backed by our content policy for initial requests and by our channel event sink for redirections). > @@ +54,5 @@ > > BASE + "/file_script_good.js", > > BASE + "/file_script_xhr.js", > > BASE + "/file_WebRequest_page2.html", > > BASE + "/nonexistent_script_url.js", > > + BASE + "/redirection.sjs", > > According to the Chrome docs, I think we should not fire onCompleted for > redirected requests. Maybe this is happening because of the onStop issue > below? Yes, kind of. We shouldn't actually add the listener to begin with in case of a redirection, see bug 728901. I'm adding a check to avoid it in this patch revision.

Comment on attachment 8697073 [details] [diff] [review] Implements onBeforeRedrect by using a channel event sink, v2 Review of attachment 8697073 [details] [diff] [review]: ----------------------------------------------------------------- (Sorry, was on vacation last week.) This seems to be the same patch as the last one. Did you mean to upload a different one?

Sorry, I managed to make a mess of my hg queue. In my defense, it was last Mozlando day.

Comment on attachment 8700868 [details] [diff] [review] mplements onBeforeRedrect by using a channel event sink, v2 (for real) Review of attachment 8700868 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! Let me know if you need help checking this in. ::: toolkit/modules/addons/WebRequest.jsm @@ +329,5 @@ > type: WebRequestCommon.typeForPolicyType(policyType), > windowId: loadInfo ? loadInfo.outerWindowID : 0, > parentWindowId: loadInfo ? loadInfo.parentOuterWindowID : 0, > }; > + if (extraData) Object.assign(data, extraData); This should look like: if (extraData) { Object.assign(...); } @@ +403,5 @@ > let loadContext = this.getLoadContext(channel); > > if (this.listeners.onStart.size || this.listeners.onStop.size) { > if (channel instanceof Components.interfaces.nsITraceableChannel) { > + let responseStatus = channel.nsIHttpChannel.responseStatus; Is channel.nsIHttpChannel doing a QueryInterface? I haven't seen that before. I think it would be clearer to do channel.QueryInterface(Ci.nsIHttpChannel).responseStatus. @@ +418,5 @@ > }, > > + onChannelReplaced(oldChannel, newChannel) { > + this.runChannelListener(oldChannel, this.getLoadContext(oldChannel), > + "onRedirect", { redirectUrl: newChannel.URI.spec }); This should be indented to the same level as the ( in the line above. Also, it looks like there are two spaces in between the , and {.

jsObject.nsISomeInterface is ancient veterans-only XPConnect black magic syntactic sugar ;) But it turns out we don't need to QI to nsIHttpChannel there because we do it once for all in observe(). Nits fixed. I don't have any commit rights, so I do need your help for landing, I guess.

OK. Do you have access to try server? If so, please push the patch to try. You probably only need to test linux. Then follow these instructions for setting the commit message: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F and set the checkin-needed keyword on the bug. If you don't have access to try, I can push it for you.

(In reply to Bill McCloskey (:billm) from comment #8) > OK. Do you have access to try server? I don't think so. As I said, I've got no commit rights, which seem to be required ( https://wiki.mozilla.org/ReleaseEngineering/TryServer#Try_Server ). I'll go through https://www.mozilla.org/en-US/about/governance/policies/commit/ ASAP, but in the meanwhile... > If you don't have access to try, I can push it for you. ... I'd appreciate it :)

Rebased and added proper commit message. Now that I've got sufficient access level, I'm gonna try by myself to push it to try (!) and prepare it for landing. Thanks :billm and :kmag for all the shepherding and the vouching.

The orange from the browser_WebRequest.js test, despite the suggestive appearance, seems unrelated (a tab mismatch in onBeforeSendHeaders when attempting to load a non-existent script, probably a prefetching artifact) and I cannot reproduce it locally, anyway. Requesting check-in.

We can't check something in if the test is failing every time. I'll see if I can reproduce it on Monday.

I managed to reproduce this, but I'm having trouble understanding the case. You should be able to see it by duplicating this line many times (maybe 100): https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/tests/browser/file_WebRequest_page2.html#22 Then when I run the test locally, I see the error. If I remove the server-side redirection, it goes away.

It turns out the test was broken. It was waiting for any load event to signal that the test was finished. Previously that sort of worked because if either page1.html or page2.html was finished, then everything had pretty much finished loading. But Giorgio's patch added another frame, and if we got the load event for that one first, not everything would actually be loaded. I fixed the test to wait for a top-level load event.

Oh, man, sorry, I messed up the commit author when rebasing. It should credit Giorgio.

I am currently seeing a problem with onBeforeRequest for a new experimental extension I have been working, the behavior I get with Firefox's WebExtensions is different than the one I get with Chromium's extensions API. I would just like to know if it's by design or if I should open a new issue for this along with two mini demo extensions. The documentation for Chrome concerning onBeforeRequest states: > The web request API guarantees that for each request either onCompleted or onErrorOccurred > is fired as the final event with one exception: If a request is redirected to a data:// URL, > onBeforeRedirect is the last reported event. This new experimental extension install a non-blocking listener for onBeforeRequest, and also listeners for onCompleted and onErrorOccurred. The issue I observe is that none of onCompleted, onErrorOccurred, or onBeforeRedirect is called when a resource is redirected to a data: URI by another extension (webext version of uBO). This works as per documentation for Chromium.

Sigh. Please replace all occurrences of "onBeforeRequest" with "onBeforeRedirect" in my comment above, somehow I ended up typing "onBeforeRequest" while I had in mind "onBeforeRedirect".

Could you file a new bug please? Then it will get triaged and we can mark it as a blocker if need be. Comments on old closed bugs do get looked at, but not as quickly.