1163862 - HTTP observer for webRequest onBeforeRequest + requestId and data: scheme support

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

[Giorgio Maone [:ma1]]

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

[Giorgio Maone [:ma1]]

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

[Giorgio Maone [:ma1]]

Attachment: rb34979.patch

Switches to HTTP observer where makes senses and adds requestId support.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

[Giorgio Maone [:ma1]]

(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

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(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

[Giorgio Maone [:ma1]]

Attachment: 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 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

[Giorgio Maone [:ma1]]

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

[Giorgio Maone [:ma1]]

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

[Giorgio Maone [:ma1]]

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

[Giorgio Maone [:ma1]]

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

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Giorgio, is this ready to land?

Comment 16

[Giorgio Maone [:ma1]]

(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

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(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 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ae0fba61023

Comment 19

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/535dbd1cfd00 - Win7 e10s says https://treeherder.mozilla.org/logviewer.html#?job_id=22444135&repo=mozilla-inbound

Comment 20

[Giorgio Maone [:ma1]]

(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

[Giorgio Maone [:ma1]]

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 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/06d2b67a562b

Comment 23

[Giorgio Maone [:ma1]]

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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/06d2b67a562b