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

RESOLVED FIXED in Firefox 47

Status

()

Toolkit
WebExtensions: Untriaged
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: billm, Assigned: mao)

Tracking

Trunk
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

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

Comment 1

2 years ago
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.
Iteration: --- → 46.1 - Dec 28
Component: Extension Compatibility → WebExtensions
Product: Firefox → Toolkit
Version: 34 Branch → Trunk
(Assignee)

Updated

2 years ago
Assignee: nobody → g.maone
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
Iteration: 46.1 - Dec 28 → 46.2 - Jan 11

Updated

2 years ago
Iteration: 46.2 - Jan 11 → 46.3 - Jan 25
(Assignee)

Comment 2

a year ago
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.
Iteration: 46.3 - Jan 25 → 47.1 - Feb 8
(Assignee)

Updated

a year ago
Blocks: 1224581
(Assignee)

Updated

a year ago
Blocks: 1202734
(Assignee)

Updated

a year ago
Iteration: 47.1 - Feb 8 → 47.2 - Feb 22
(Assignee)

Comment 3

a year ago
Created attachment 8719524 [details] [diff] [review]
rb34979.patch

Switches to HTTP observer where makes senses and adds requestId support.
Attachment #8719524 - Flags: review?(wmccloskey)
(Reporter)

Comment 4

a year ago
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")
Attachment #8719524 - Flags: review?(wmccloskey)
(Assignee)

Comment 5

a year ago
(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)) {
(Reporter)

Comment 6

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

Comment 7

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

Updated

a year ago
Attachment #8721655 - Flags: review?(wmccloskey)
(Assignee)

Updated

a year ago
Duplicate of this bug: 1224581
(Assignee)

Updated

a year ago
Duplicate of this bug: 1202734
(Reporter)

Comment 10

a year ago
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?
Attachment #8721655 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 11

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

Comment 12

a year ago
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/
Attachment #8721655 - Attachment description: MozReview Request: Bug 1163862 - Switch to HTTP observer where possible and support requestId → MozReview Request: Bug 1163862 - Switch to HTTP observer where possible + support requestId & data: URIs
(Assignee)

Comment 13

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

Comment 14

a year ago
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/
(Reporter)

Comment 15

a year ago
Giorgio, is this ready to land?
(Assignee)

Comment 16

a year ago
(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.
(Reporter)

Comment 17

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

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ae0fba61023
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
(Assignee)

Comment 20

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

Comment 21

a year ago
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/
Attachment #8721655 - Attachment description: MozReview Request: Bug 1163862 - Switch to HTTP observer where possible + support requestId & data: URIs → MozReview Request: Bug 1163862 - Switch to HTTP observer + support requestId & data: URIs + test fixes

Comment 22

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/06d2b67a562b
(Assignee)

Comment 23

a year ago
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/06d2b67a562b
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(Assignee)

Updated

a year ago
Summary: Use an HTTP observer for webRequest onBeforeRequest → HTTP observer for webRequest onBeforeRequest + requestId and data: scheme support
(Assignee)

Updated

a year ago
Blocks: 1176092
(Assignee)

Updated

a year ago
See Also: → bug 1256264
Duplicate of this bug: 1272199
You need to log in before you can comment on or make changes to this bug.