Implement chrome.webRequest.onBeforeRedirect

RESOLVED FIXED in Firefox 46

Status

defect
RESOLVED FIXED
4 years ago
Last year

People

(Reporter: mao, Assigned: mao)

Tracking

(Blocks 2 bugs, {dev-doc-complete})

unspecified
mozilla46
Dependency tree / graph
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: [webRequest], )

Attachments

(1 attachment, 5 obsolete attachments)

Assignee

Description

4 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

4 years ago
Blocks: webext
Assignee

Updated

4 years ago

Updated

4 years ago
Flags: blocking-webextensions+

Updated

4 years ago

Updated

4 years ago
Assignee: nobody → g.maone
Iteration: --- → 45.3 - Dec 14
Assignee

Comment 1

4 years ago
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

4 years ago
(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

4 years 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

4 years ago
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

4 years ago
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

4 years 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

4 years ago
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 14

4 years 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
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.

Comment 20

4 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d91404daf19b
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46

Comment 21

2 years 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 years 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 years 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.

Updated

Last year
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.