Implement chrome.webRequest.onBeforeRedirect

RESOLVED FIXED in Firefox 46

Status

()

Toolkit
WebExtensions: Untriaged
RESOLVED FIXED
2 years ago
2 months ago

People

(Reporter: mao, Assigned: mao)

Tracking

(Blocks: 3 bugs, {dev-doc-complete})

unspecified
mozilla46
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: [webRequest], URL)

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

2 years ago
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
(Assignee)

Updated

2 years ago
Blocks: 1214433
(Assignee)

Updated

2 years ago
Blocks: 1214733

Updated

2 years ago
Flags: blocking-webextensions+

Updated

2 years ago
Blocks: 1213483

Updated

a year ago
Assignee: nobody → g.maone
Iteration: --- → 45.3 - Dec 14
(Assignee)

Comment 1

a year ago
Created attachment 8693855 [details] [diff] [review]
webRequest.onBeforeRedirect.patch

onBeforeRedirect implementation + tests for both WebRequest.jsm and ext_webRequest.
Attachment #8693855 - Flags: review?(wmccloskey)
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?
Attachment #8693855 - Flags: review?(wmccloskey)
(Assignee)

Comment 3

a year ago
Created attachment 8697073 [details] [diff] [review]
Implements onBeforeRedrect by using a channel event sink, v2

(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.
Attachment #8693855 - Attachment is obsolete: true
Attachment #8697073 - Flags: review?(wmccloskey)
(Assignee)

Updated

a year ago
Iteration: 45.3 - Dec 14 → 46.1 - Dec 28
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?
Attachment #8697073 - Flags: review?(wmccloskey)
(Assignee)

Comment 5

a year ago
Created attachment 8700868 [details] [diff] [review]
mplements onBeforeRedrect by using a channel event sink, v2 (for real)

Sorry, I managed to make a mess of my hg queue. 
In my defense, it was last Mozlando day.
Attachment #8697073 - Attachment is obsolete: true
Attachment #8700868 - Flags: review?(wmccloskey)
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 {.
Attachment #8700868 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 7

a year ago
Created attachment 8701251 [details] [diff] [review]
Implements onBeforeRedirect by using a channel event sink, v2.1 (nits)

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.
Attachment #8700868 - Attachment is obsolete: true
Flags: needinfo?(wmccloskey)
Attachment #8701251 - Flags: review+
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.
Flags: needinfo?(wmccloskey)
(Assignee)

Comment 9

a year ago
(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 :)
(Assignee)

Comment 10

a year ago
Created attachment 8702025 [details] [diff] [review]
Implements onBeforeRedrect by using a channel event sink, v2.1 rebased and with commit msg

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.
Attachment #8701251 - Attachment is obsolete: true
Attachment #8702025 - Flags: review+
(Assignee)

Comment 11

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=891675f09ba6
(Assignee)

Comment 12

a year ago
Created attachment 8702055 [details] [diff] [review]
Implements onBeforeRedirect by using a channel event sink, v2.1.1 rebased and fixed ext mochitest
Attachment #8702025 - Attachment is obsolete: true
Attachment #8702055 - Flags: review+
(Assignee)

Comment 13

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5f47c23a162
(Assignee)

Comment 14

a year ago
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.
Keywords: checkin-needed
We can't check something in if the test is failing every time. I'll see if I can reproduce it on Monday.
Keywords: checkin-needed
Blocks: 1235639
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.

Comment 17

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d91404daf19b
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.

Comment 20

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d91404daf19b
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Keywords: dev-doc-needed
Keywords: dev-doc-needed → dev-doc-complete

Comment 21

2 months ago
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.

Comment 22

2 months ago
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".

Comment 23

2 months ago
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.
You need to log in before you can comment on or make changes to this bug.