Closed Bug 1373646 Opened 3 years ago Closed 3 years ago

Add nsIProxyInfo to webRequest

Categories

(WebExtensions :: Request Handling, defect, P3)

defect

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: ashley, Assigned: mixedpuppy)

Details

(Keywords: dev-doc-complete, Whiteboard: [proxy])

Attachments

(1 file)

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).
Whiteboard: design-decision-needed
What information specifically do you need from nsIProxiedChannel?
Component: WebExtensions: Untriaged → WebExtensions: Request Handling
Whiteboard: design-decision-needed → [proxy] design-decision-needed
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.
Priority: -- → P3
Summary: Add proxy information to webRequest → Add nsIProxyInfo to webRequest
Whiteboard: [proxy] design-decision-needed → [proxy]
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.
Attachment #8899539 - Flags: feedback?(kmaglione+bmo)
Attachment #8899539 - Flags: feedback?(eric)
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.
> 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.
If failover isn't important in this context, I'd prefer to remove that.
Assignee: nobody → mixedpuppy
> If failover isn't important in this context, I'd prefer to remove that.

Can you explain why that is your preference?
(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 on attachment 8899539 [details]
Bug 1373646 add nsIProxyInfo to webrequest,

https://reviewboard.mozilla.org/r/170824/#review178202
Attachment #8899539 - Flags: review?(kmaglione+bmo) → review+
Keywords: dev-doc-needed
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)
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/94b3aaa5e27e
add nsIProxyInfo to webrequest, r=kmag
https://hg.mozilla.org/mozilla-central/rev/94b3aaa5e27e
Status: UNCONFIRMED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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
I've added a bit to each of the events, mentioning this. Let me know if that covers it.
Flags: needinfo?(mixedpuppy)
lgtm.
Flags: needinfo?(mixedpuppy)
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.
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(mixedpuppy) → qe-verify-
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.