Last Comment Bug 1215197 - Implement chrome.webRequest.onBeforeRedirect
: Implement chrome.webRequest.onBeforeRedirect
Status: RESOLVED FIXED
[webRequest]
: dev-doc-complete
Product: Toolkit
Classification: Components
Component: WebExtensions: Untriaged (show other bugs)
: unspecified
: Unspecified Unspecified
: -- normal (vote)
: mozilla46
Assigned To: Giorgio Maone [:mao]
:
:
Mentors:
https://developer.chrome.com/extensio...
Depends on:
Blocks: webextensions-chrome-gaps webRequest-full webext-port-noscript e10s-noscript webext1.0 1235639
  Show dependency treegraph
 
Reported: 2015-10-15 10:13 PDT by Giorgio Maone [:mao]
Modified: 2016-02-03 11:56 PST (History)
7 users (show)
amckay: blocking‑webextensions+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: 46.1 - Dec 28
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
webRequest.onBeforeRedirect.patch (27.30 KB, patch)
2015-11-30 16:29 PST, Giorgio Maone [:mao]
no flags Details | Diff | Splinter Review
Implements onBeforeRedrect by using a channel event sink, v2 (27.30 KB, patch)
2015-12-09 11:49 PST, Giorgio Maone [:mao]
no flags Details | Diff | Splinter Review
mplements onBeforeRedrect by using a channel event sink, v2 (for real) (27.98 KB, patch)
2015-12-21 17:43 PST, Giorgio Maone [:mao]
wmccloskey: review+
Details | Diff | Splinter Review
Implements onBeforeRedirect by using a channel event sink, v2.1 (nits) (28.12 KB, patch)
2015-12-22 15:20 PST, Giorgio Maone [:mao]
g.maone: review+
Details | Diff | Splinter Review
Implements onBeforeRedrect by using a channel event sink, v2.1 rebased and with commit msg (28.35 KB, patch)
2015-12-25 17:14 PST, Giorgio Maone [:mao]
g.maone: review+
Details | Diff | Splinter Review
Implements onBeforeRedirect by using a channel event sink, v2.1.1 rebased and fixed ext mochitest (28.85 KB, patch)
2015-12-26 03:00 PST, Giorgio Maone [:mao]
g.maone: review+
Details | Diff | Splinter Review

Description User image Giorgio Maone [:mao] 2015-10-15 10:13:17 PDT
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
Comment 1 User image Giorgio Maone [:mao] 2015-11-30 16:29:36 PST
Created attachment 8693855 [details] [diff] [review]
webRequest.onBeforeRedirect.patch

onBeforeRedirect implementation + tests for both WebRequest.jsm and ext_webRequest.
Comment 2 User image Bill McCloskey (:billm) 2015-12-01 16:18:41 PST
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?
Comment 3 User image Giorgio Maone [:mao] 2015-12-09 11:49:56 PST
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.
Comment 4 User image Bill McCloskey (:billm) 2015-12-21 17:05:17 PST
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?
Comment 5 User image Giorgio Maone [:mao] 2015-12-21 17:43:34 PST
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.
Comment 6 User image Bill McCloskey (:billm) 2015-12-22 14:11:31 PST
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 {.
Comment 7 User image Giorgio Maone [:mao] 2015-12-22 15:20:44 PST
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.
Comment 8 User image Bill McCloskey (:billm) 2015-12-22 15:29:56 PST
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.
Comment 9 User image Giorgio Maone [:mao] 2015-12-22 15:50:11 PST
(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 :)
Comment 10 User image Giorgio Maone [:mao] 2015-12-25 17:14:09 PST
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.
Comment 12 User image Giorgio Maone [:mao] 2015-12-26 03:00:28 PST
Created attachment 8702055 [details] [diff] [review]
Implements onBeforeRedirect by using a channel event sink, v2.1.1 rebased and fixed ext mochitest
Comment 14 User image Giorgio Maone [:mao] 2015-12-26 16:40:11 PST
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.
Comment 15 User image Bill McCloskey (:billm) 2015-12-26 18:03:26 PST
We can't check something in if the test is failing every time. I'll see if I can reproduce it on Monday.
Comment 16 User image Bill McCloskey (:billm) 2015-12-29 17:48:31 PST
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 18 User image Bill McCloskey (:billm) 2015-12-30 13:36:57 PST
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.
Comment 19 User image Bill McCloskey (:billm) 2015-12-30 13:37:42 PST
Oh, man, sorry, I messed up the commit author when rebasing. It should credit Giorgio.
Comment 20 User image Wes Kocher (:KWierso) 2015-12-30 17:37:38 PST
https://hg.mozilla.org/mozilla-central/rev/d91404daf19b

Note You need to log in before you can comment on or make changes to this bug.