1373646 - Add nsIProxyInfo to webRequest

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, defect, P3)

Description

[ashley]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_4) AppleWebKit/603.1.30 (KHTML, like Gecko) Version/10.1 Safari/603.1.30

Steps to reproduce:

A feature of my addon (SixOrNot) is to show information about any proxies used in loading a web page. Currently I'm making use of subject.QueryInterface(Components.interfaces.nsIProxiedChannel); to pull information about the proxy out of the "http-on-examine-response" event (proxy host, port and type). As far as I can see the webRequest API in WebExtensions doesn't have any way to get this information per-request. Similar to https://bugzilla.mozilla.org/show_bug.cgi?id=1322748 (for TLS information per-request) I'd like to request proxy information be added (e.g. to webRequest onCompleted for my use case).

Comment 1

[Shane Caraveo (:mixedpuppy)]

What information specifically do you need from nsIProxiedChannel?

Comment 2

[Shane Caraveo (:mixedpuppy)]

I think we should just do it, and it's not that much effort as I'm already doing it for onAuthRequired in bug 1388289, this is a simple addition.

Comment 3

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1373646 add nsIProxyInfo to webrequest,

Review commit: https://reviewboard.mozilla.org/r/170824/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/170824/

Comment 4

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8899539 [details]
Bug 1373646 add nsIProxyInfo to webrequest,

A quick implementation adding proxyInfo to webRequest.  Would like feedback prior to writing tests.  

Purposely leaving out nsIProxyInfo.password, if this is necessary we need a clear use case and security review.

Comment 5

[Eric Jung [:ericjung]]

Looks good. Just one comment:

> if (failoverCount > 5) {

I think 5 is OK for this patch; I think you'll hit 90% or more of typical failover use-cases. However, bumping it to 10 would bring you to 99%. Those figures are strictly my opinion and not based on any analytics or raw data. For bug 1381290 (or whatever we replace it with based on https://goo.gl/4yCxe1) -- for the addon building the proxy failover chain -- I think the limit should be much higher or or no limit at all.

Comment 6

[Eric Jung [:ericjung]]

> if (failoverCount > 5) {

Also, I'm not really sure how important failover info is from within webRequest, unless an addon is doing some kind of accounting or logging in excruciating detail.

Comment 7

[Shane Caraveo (:mixedpuppy)]

If failover isn't important in this context, I'd prefer to remove that.

Comment 8

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8899539 [details]
Bug 1373646 add nsIProxyInfo to webrequest,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170824/diff/1-2/

Comment 9

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8899539 [details]
Bug 1373646 add nsIProxyInfo to webrequest,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170824/diff/2-3/

Comment 10

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8899539 [details]
Bug 1373646 add nsIProxyInfo to webrequest,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/170824/diff/3-4/

Comment 11

[Eric Jung [:ericjung]]

> If failover isn't important in this context, I'd prefer to remove that.

Can you explain why that is your preference?

Comment 12

[Shane Caraveo (:mixedpuppy)]

(In reply to Eric Jung [:ericjung] from comment #11)
> > If failover isn't important in this context, I'd prefer to remove that.
> 
> Can you explain why that is your preference?

Without a good use case, this code (which gets hit a lot) is simpler without it.

Comment 13

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8e247ca17b0f770c24f5b05235c6f98bdccff3a

Comment 14

[Kris Maglione [:kmag]]

Comment on attachment 8899539 [details]
Bug 1373646 add nsIProxyInfo to webrequest,

https://reviewboard.mozilla.org/r/170824/#review178202

Comment 15

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s d4655e6851a4 -d f570921adc79: rebasing 415931:d4655e6851a4 "Bug 1373646 add nsIProxyInfo to webrequest, r=kmag" (tip)
other [source] changed toolkit/components/extensions/test/xpcshell/test_ext_proxy_auth.js which local [dest] deleted
use (c)hanged version, leave (d)eleted, or leave (u)nresolved? u
merging toolkit/modules/addons/WebRequest.jsm
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/94b3aaa5e27e
add nsIProxyInfo to webrequest, r=kmag

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/94b3aaa5e27e

Comment 18

[Shane Caraveo (:mixedpuppy)]

for dev-doc

All webrequest listeners return a details object.  This adds "proxyInfo" to that object.  The structure is essentially the same as nsIProxyInfo[1] except it does not include:

 flags
 resolveFlags
 password
 failoverProxy

and it adds proxyDNS (boolean) which is based on the flags indicating TRANSPARENT_PROXY_RESOLVES_HOST.

details {
 proxyInfo {
   host,
   port,
   type,
   username,
   proxyDNS,
   failoverTimeout,
 }
}

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIProxyInfo.idl

Comment 19

[Will Bamberg [:wbamberg]]

I've added a bit to each of the events, mentioning this. Let me know if that covers it.

Comment 20

[Will Bamberg [:wbamberg]]

Oh sorry, link: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest

Comment 21

[Shane Caraveo (:mixedpuppy)]

lgtm.

Comment 22

[marius.santa]

Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.