Last Comment Bug 1163862 - HTTP observer for webRequest onBeforeRequest + requestId and data: scheme support
: HTTP observer for webRequest onBeforeRequest + requestId and data: scheme sup...
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: WebExtensions: Untriaged (show other bugs)
: Trunk
: Unspecified Unspecified
: -- normal with 2 votes (vote)
: mozilla47
Assigned To: Giorgio Maone [:mao]
:
:
Mentors:
: 1202734 1224581 1272199 (view as bug list)
Depends on: 1157561
Blocks: 1176092 1202734 1224581
  Show dependency treegraph
 
Reported: 2015-05-11 17:14 PDT by Bill McCloskey (:billm)
Modified: 2016-05-14 14:20 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: 47.2 - Feb 22
Points: ---
Has Regression Range: ---
Has STR: ---
fixed

MozReview Requests
Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:
Show discarded requests

Attachments
rb34979.patch (17.50 KB, patch)
2016-02-15 08:55 PST, Giorgio Maone [:mao]
no flags Details | Diff | Splinter Review
MozReview Request: Bug 1163862 - Switch to HTTP observer + support requestId & data: URIs + test fixes (58 bytes, text/x-review-board-request)
2016-02-20 05:14 PST, Giorgio Maone [:mao]
wmccloskey: review+
Details | Review

Description User image Bill McCloskey (:billm) 2015-05-11 17:14:46 PDT
All of the other notifications in webRequest are implemented in the parent process using HTTP observers. It would be much better if the onBeforeRequest worked the same way, instead of using content policies in the child process as it does now. This would allow us to:
- redirect the request in onBeforeRequest
- use a weakmap to keep track of request IDs
- pass in the request method
Comment 1 User image Giorgio Maone [:mao] 2015-12-14 14:04:18 PST
Relocating to the WebExtensions component, since it's currently the main consumer of this module and would need to be modified as well, especially tests. Also, it's the de-facto blocker of multiple Chrome-parity bugs.
Comment 2 User image Giorgio Maone [:mao] 2016-01-25 04:56:48 PST
This became a bit more complicated, because in order to achieve Chrome parity (and better NoScript support, incidentally) we actually need to keep the ContentPolicy around (for corner cases) and add a WebProgressListener to the mix, too.
A patch is almost ready (hammering on the tests), I'll try to attach it this week.
Good news is that most pending webRequest issues are gonna be much easier to handle, after it lands.
Comment 3 User image Giorgio Maone [:mao] 2016-02-15 08:55:39 PST
Created attachment 8719524 [details] [diff] [review]
rb34979.patch

Switches to HTTP observer where makes senses and adds requestId support.
Comment 4 User image Bill McCloskey (:billm) 2016-02-18 14:54:22 PST
Comment on attachment 8719524 [details] [diff] [review]
rb34979.patch

Review of attachment 8719524 [details] [diff] [review]:
-----------------------------------------------------------------

This looks really good. I did notice one problem with the onStop listener for content policies.

I'm starting to get worried that we don't have any tests for non-HTTP requests. Can you please add something?

::: toolkit/modules/addons/WebRequest.jsm
@@ +20,5 @@
>                                    "resource://gre/modules/BrowserUtils.jsm");
>  XPCOMUtils.defineLazyModuleGetter(this, "WebRequestCommon",
>                                    "resource://gre/modules/WebRequestCommon.jsm");
>  
> +

No need for the extra line here.

@@ +56,5 @@
> +    return channel && extractFromChannel(channel, this.KEY) || this.create(channel);
> +  }
> +};
> +
> +function later(job) {

Let's call this runLater.

@@ +144,5 @@
> +    let listeners = HttpObserverManager.listeners[kind];
> +    let uri = BrowserUtils.makeURI(data.url);
> +    let policyType = data.type;
> +    for (let [callback, opts] of listeners.entries()) {
> +      if (!this.shouldRunListener(policyType, uri, opts.filter)) {

shouldRunListener is a method of HttpObserverManager, so this won't work afaik.

@@ +155,2 @@
>    addListener(callback, opts) {
> +    // Clone opts, since we're going to modify them for IPC

Please add a period at the end of the sentence.

@@ +267,5 @@
>      onStart: new Map(),
>      onStop: new Map(),
>    },
>  
> +  addOrRemove() { 

Remove the space at the end of the line.

@@ +348,5 @@
>      let channel = subject.QueryInterface(Ci.nsIHttpChannel);
> +    switch (topic) {
> +      case "http-on-modify-request":
> +        this.modify(channel, topic, data);
> +      break;

The formatting is a little weird. Can you indent the break statement to the same level as the one above it?

::: toolkit/modules/addons/WebRequestContent.js
@@ +78,5 @@
>  
>    shouldLoad(policyType, contentLocation, requestOrigin,
>               node, mimeTypeGuess, extra, requestPrincipal) {
> +    let url = contentLocation.spec;
> +    if (url.startsWith("http")) {

I'd feel a little more comfortable if this said:
  if (url == "http" || url == "https")
Comment 5 User image Giorgio Maone [:mao] 2016-02-19 06:01:30 PST
(In reply to Bill McCloskey (:billm) from comment #4)

> @@ +144,5 @@
> > +    let listeners = HttpObserverManager.listeners[kind];
> > +    let uri = BrowserUtils.makeURI(data.url);
> > +    let policyType = data.type;
> > +    for (let [callback, opts] of listeners.entries()) {
> > +      if (!this.shouldRunListener(policyType, uri, opts.filter)) {
> 
> shouldRunListener is a method of HttpObserverManager, so this won't work
> afaik.
> 

Yes, it should have been HttpObsrverManager.shouldListener(), and yes, if we had tests for data: URIs I would have caught it immediately :(

> > +    let url = contentLocation.spec;
> > +    if (url.startsWith("http")) {
> 
> I'd feel a little more comfortable if this said:
>   if (url == "http" || url == "https")

It wouldn't work, as we're checking contentLocation.spec (a full URI) 
If your worry is that a new httpsomething: protocol has been registered some way, we can either check contentLocation.scheme (which is theoretically a bit slower, with an extra XPCOM call and string copy) or use a RegExp:

const IS_HTTP = /^https?:/;
[...]
if (IS_HTTP.test(url)) {
Comment 6 User image Bill McCloskey (:billm) 2016-02-19 15:17:14 PST
(In reply to Giorgio Maone from comment #5)
> It wouldn't work, as we're checking contentLocation.spec (a full URI) 

Sorry, duh. Yeah.

> If your worry is that a new httpsomething: protocol has been registered some
> way, we can either check contentLocation.scheme (which is theoretically a
> bit slower, with an extra XPCOM call and string copy) or use a RegExp:
> 
> const IS_HTTP = /^https?:/;
> [...]
> if (IS_HTTP.test(url)) {

Yeah, that's my worry. url.startsWith("http:") || url.startsWith("https:") would be fine too.
Comment 7 User image Giorgio Maone [:mao] 2016-02-20 05:14:09 PST
Created attachment 8721655 [details]
MozReview Request: Bug 1163862 - Switch to HTTP observer + support requestId & data: URIs + test fixes

Review commit: https://reviewboard.mozilla.org/r/35759/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35759/
Comment 8 User image Giorgio Maone [:mao] 2016-02-22 09:00:34 PST
*** Bug 1224581 has been marked as a duplicate of this bug. ***
Comment 9 User image Giorgio Maone [:mao] 2016-02-22 09:01:17 PST
*** Bug 1202734 has been marked as a duplicate of this bug. ***
Comment 10 User image Bill McCloskey (:billm) 2016-02-22 16:24:19 PST
Comment on attachment 8721655 [details]
MozReview Request: Bug 1163862 - Switch to HTTP observer + support requestId & data: URIs + test fixes

https://reviewboard.mozilla.org/r/35759/#review32581

Thanks!

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest.html:34
(Diff revision 1)
> -                            BASE + "/xhr_resource"];
> +                            BASE + "/dummy_page.html",

Why did dummy_page get added here?
Comment 11 User image Giorgio Maone [:mao] 2016-02-22 17:19:33 PST
https://reviewboard.mozilla.org/r/35759/#review32581

> Why did dummy_page get added here?

Because now we support onBeforeRequest to be triggered after redirections (with the same requestId as before the redirection, but with a different underlying channel): that's basically how Chrome works, and how redirections can be blocked, since onBeforeRedirect doesn't allow blocking.
Comment 12 User image Giorgio Maone [:mao] 2016-02-23 10:31:20 PST
Comment on attachment 8721655 [details]
MozReview Request: Bug 1163862 - Switch to HTTP observer + support requestId & data: URIs + test fixes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35759/diff/1-2/
Comment 13 User image Giorgio Maone [:mao] 2016-02-23 10:36:24 PST
I modified the patch (tiny changes, but touches MatchPattern.jsm therefore asking for at least pro-forma review), because it turned out supporting data: URIs was a tad more complicated than foreseen, and was breaking some tests on try :(
It should be all right now.
Comment 14 User image Giorgio Maone [:mao] 2016-02-23 15:39:48 PST
Comment on attachment 8721655 [details]
MozReview Request: Bug 1163862 - Switch to HTTP observer + support requestId & data: URIs + test fixes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35759/diff/2-3/
Comment 15 User image Bill McCloskey (:billm) 2016-02-25 13:17:33 PST
Giorgio, is this ready to land?
Comment 16 User image Giorgio Maone [:mao] 2016-02-25 13:22:32 PST
(In reply to Bill McCloskey (:billm) from comment #15)
> Giorgio, is this ready to land?

I think so, the try run didn't report any test failure related to this patch.
Comment 17 User image Bill McCloskey (:billm) 2016-02-25 13:24:10 PST
(In reply to Giorgio Maone from comment #16)
> (In reply to Bill McCloskey (:billm) from comment #15)
> > Giorgio, is this ready to land?
> 
> I think so, the try run didn't report any test failure related to this patch.

OK, can you add the checkin-needed flag then?
Comment 20 User image Giorgio Maone [:mao] 2016-02-26 05:51:07 PST
(In reply to Phil Ringnalda (:philor) from comment #19)
> Win7 e10s says
> https://treeherder.mozilla.org/logviewer.html#?job_id=22444135&repo=mozilla-
> inbound

I know what's going on, even if those tests didn't fail for me neither locally nor in my two try pushes, and it's a problem in the test itself: the way we store the expected requestIDs, keying them by the request URLs, is not reliable when the same URL is loaded simultaneously by two requests, which is something I did not expect to happen in test_ext_webrequest.html. It didn't actually happen on me, I suppose because of configuration / concurrency / machine stress reasons, but I recognize I should have inspected the code more carefully :(

I'm posting a further patch fixing just the test later today, hoping in a quick review and checkin. Sorry for the hassle.
Comment 21 User image Giorgio Maone [:mao] 2016-02-26 10:25:18 PST
Comment on attachment 8721655 [details]
MozReview Request: Bug 1163862 - Switch to HTTP observer + support requestId & data: URIs + test fixes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35759/diff/3-4/
Comment 23 User image Giorgio Maone [:mao] 2016-02-26 10:47:35 PST
John-Galt is about to land the new revision fixing the test, per our IRC conversation:

<mao>	
John-Galt, I'd ask billm but I can't see him so I'll bother youI've got this patch, https://reviewboard.mozilla.org/r/35759/ , which passed tests locally and on try but failed on one instance in inbound. I think I know how it happened (lousy test which might fail randomly, depending on parallelism glitches) and fixed it. Should I run another try job (which is likely not fail aniway on try, even if my fix is bad -- I still couldn't make it fail locally) or just ask for relanding?
<John-Galt> 
mao: If the changes aren't likely to cause new failures, than I'd say, yeah, just ask for re-landing. And mention something about filing an intermittent if it fails again.
Comment 24 User image Carsten Book [:Tomcat] 2016-02-29 02:38:48 PST
https://hg.mozilla.org/mozilla-central/rev/06d2b67a562b
Comment 25 User image Kris Maglione [:kmag] 2016-05-14 14:20:55 PDT
*** Bug 1272199 has been marked as a duplicate of this bug. ***

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