1115495 - Specific proxy server setting for browsing traffic

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Shian-Yow Wu [:swu]]

Currently, we have proxy server preferences for system wide traffic.
https://developer.mozilla.org/en-US/Firefox_OS/Debugging/Intercepting_traffic_using_a_proxy

For Firefox OS, we might want the proxy settings be applied only to browsing traffic, or to browser app.

Use case:
In some countries, it is enforced by law to have parental control mechanism for web browsing on the device.  The device redirects all browsing traffic to a 3rd-party proxy server that provides parental control service.  While other traffic go through user defined proxy server or without proxy setting.

Comment 1

[Shian-Yow Wu [:swu]]

After since v2.1, the browser app is bundled in system app.
See https://bugzilla.mozilla.org/show_bug.cgi?id=1070944#c26 
We can use the same idea to apply proxy server setting for browsering data.

There is another bug 983265, that bug emphasizes on supporting proxy server setting for browser app, while this bug emphasizes more on separating the browsing traffic and apply specific proxy server settings.

Comment 2

[Shian-Yow Wu [:swu]]

Attachment: (WIP) Patch: Allow to apply proxy server settings to browsing traffic.

This WIP patch uses a new preference "network.proxy.browsing_only" to specify whether to apply proxy server to browsing traffic only or to whole traffic.

TODO:
1. Add proxy server settings for Gaia app, and map them to preferences.

Comment 3

[Shian-Yow Wu [:swu]]

Attachment: (WIP) Patch: Add mapping from proxy server settings to Gecko pref.

WIP patch to support user defined proxy server settings.

Comment 4

[Shian-Yow Wu [:swu]]

Attachment: Patch: Support proxy server setting for browsing traffic.

Comment 5

[Shian-Yow Wu [:swu]]

Comment on attachment 8542943 [details] [diff] [review]
Patch: Support proxy server setting for browsing traffic.

The patch supports proxy server settings for browsing traffic on Firefox OS.  We may consider to apply it either to browsing traffic(check mozBrowser) or to browser app(check mozBrowser and appId), and the former case should more general.

Fabrice and Patrick, would you help to feedback on this patch?  Thank you.

Comment 6

[Patrick McManus [:mcmanus]]

I would rather use PAC to route by appid. Dragana has a patch for that as part of her work in bug 1033580. It can be teased out and landed separately if need be.

Comment 7

[[:fabrice] Fabrice Desré]

Comment on attachment 8542943 [details] [diff] [review]
Patch: Support proxy server setting for browsing traffic.

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

What Patrick said!

Comment 8

[Shian-Yow Wu [:swu]]

Thanks for the feedback.

It's a good idea to use PAC, but on current Firefox OS, Wifi or data connection overwrites manual proxy setting when the proxy server from network is available.
http://dxr.mozilla.org/mozilla-central/source/dom/system/gonk/NetworkService.js#377

If I understand correctly, manual and PAC configuration cannot co-exist.  So probably below are the things we need to do to use PAC for browsing traffic:
1. Change Wifi/data connection to use PROXYCONFIG_PAC instead of PROXYCONFIG_MANUAL.
2. Provide proprietary PAC function to check appId and mozBrowser for Firefox OS.
3. A runtime generated PAC configuration file for Wifi/data connection and browsing traffic.

Does it make sense or we have easier way to solve it?

Comment 9

[Patrick McManus [:mcmanus]]

:swu - two things

1] you can merge your polices in a pac file. It certainly seems better than having them implicitly merged through precedence of a pile of different prefs and c++ code :) .. oh and you can use a data url for your pac "file" as the preference, so you don't actually need to manage a file.

2] I'm concerned about how you say "proxy server from network is available". Setting a proxy discovered from the network is something Firefox explicitly does not do because of security concerns (that have been exploited in the past). Can you describe what's going on here - if we're getting the info and automatically applying it from dhcp or 3gpp I think we ought to have a meeting on the topic. I don't trust the network to give it that kind of power without some kind of policy based filter being applied (i.e. configuration).

Comment 10

[Shian-Yow Wu [:swu]]

(In reply to Patrick McManus [:mcmanus] from comment #9)
> :swu - two things
> 
> 1] you can merge your polices in a pac file. It certainly seems better than
> having them implicitly merged through precedence of a pile of different
> prefs and c++ code :) .. oh and you can use a data url for your pac "file"
> as the preference, so you don't actually need to manage a file.

Thank you for these pointings, I will study how to make use of PAC.

> 
> 2] I'm concerned about how you say "proxy server from network is available".
> Setting a proxy discovered from the network is something Firefox explicitly
> does not do because of security concerns (that have been exploited in the
> past). Can you describe what's going on here - if we're getting the info and
> automatically applying it from dhcp or 3gpp I think we ought to have a
> meeting on the topic. I don't trust the network to give it that kind of
> power without some kind of policy based filter being applied (i.e.
> configuration).

For 3G data connection, there is an APN database which can specify the http proxy server
  https://github.com/mozilla-b2g/gaia/blob/master/shared/resources/apn.json#L278
User can also manually add/change http proxy in Settings app for specific APN.

For Wifi, API to set http proxy per SSID is available in Gecko, but no UI support yet.
  http://dxr.mozilla.org/mozilla-central/source/dom/webidl/MozWifiManager.webidl#232

In either case, the proxy server settings are actually controlled by device, so there is no security concern.  I'd like to correct that it's configured for the network, instead from the network.  :)
Thank you to mention this for clarification.

Comment 11

[Patrick McManus [:mcmanus]]

(In reply to Shian-Yow Wu [:swu] from comment #10)
,
> so there is no security concern.  I'd like to correct that it's configured
> for the network, instead from the network.  :)

That's a big improvement and relief, thank you.

However, is there any authentication done on the apn? Such things can be and are faked. If the APN is signed in some way then I think its a good plan to use a device config to control the proxy.. if it isn't signed then its a pretty big hole.

Comment 12

[Shian-Yow Wu [:swu]]

(In reply to Patrick McManus [:mcmanus] from comment #11)
> However, is there any authentication done on the apn? Such things can be and
> are faked. If the APN is signed in some way then I think its a good plan to
> use a device config to control the proxy.. if it isn't signed then its a
> pretty big hole.

The APN database is prebuilt within the Settings app which has certified app permission, so it is supposed to be secure, unless we don't trust the device manufacturer.

Comment 13

[Patrick McManus [:mcmanus]]

(In reply to Shian-Yow Wu [:swu] from comment #12)

> The APN database is prebuilt within the Settings app which has certified app
> permission, so it is supposed to be secure, unless we don't trust the device
> manufacturer.

its not the database, its the actual APN advertisement. Why trust it if not signed? http://www.csoonline.com/article/2684064/mobile-security/rogue-cell-towers-discovered-in-washington-dc.html

better would be validating the proxy you use via https proxying.

Comment 14

[Shian-Yow Wu [:swu]]

(In reply to Patrick McManus [:mcmanus] from comment #13)
> (In reply to Shian-Yow Wu [:swu] from comment #12)
> 
> > The APN database is prebuilt within the Settings app which has certified app
> > permission, so it is supposed to be secure, unless we don't trust the device
> > manufacturer.
> 
> its not the database, its the actual APN advertisement. Why trust it if not
> signed?
> http://www.csoonline.com/article/2684064/mobile-security/rogue-cell-towers-
> discovered-in-washington-dc.html
> 
> better would be validating the proxy you use via https proxying.

3G data connection(UTMS) has mutual authentication between SIM card and service network, so it is secure against man-in-the-middle attack.  But it's still vulnerable during 2G/3G handover, because GSM does not require network to authenticate to the handset.  In the case of rogue cell tower with GSM network, however, almost everything is insecure, including phone calls, SMS, and data connections.  I think it's another level of security problem, because in such condition the data connection traffic is insecure no matter with/without proxy, unless user connects to https server with certificate or connects through vpn.

Comment 15

[Patrick McManus [:mcmanus]]

> I think it's another level of security problem,
> because in such condition the data connection traffic is insecure no matter
> with/without proxy, unless user connects to https server with certificate or
> connects through vpn.

authentication is exactly what I'm advocating for - DON'T TRUST THE NETWORK for proxy assignment. (we can authenticate the proxy) Full stop. Far too big of a hole.

Comment 16

[Shian-Yow Wu [:swu]]

(In reply to Patrick McManus [:mcmanus] from comment #15)
> > I think it's another level of security problem,
> > because in such condition the data connection traffic is insecure no matter
> > with/without proxy, unless user connects to https server with certificate or
> > connects through vpn.
> 
> authentication is exactly what I'm advocating for - DON'T TRUST THE NETWORK
> for proxy assignment. (we can authenticate the proxy) Full stop. Far too big
> of a hole.

To make sure I understand it correctly regarding authenticating the proxy, let me double confirm it as below.

1. Current design:
For data connection, on the device there is a APN database that maintains APN related information for each carrier.  In this database, some carriers have pre-defined http proxy setting(most others don't).  When a data connection is established, a couple of settings will be applied, such as default route, DNS, http proxy, etc.

2. Potential risk:
If device connects to a fake cell tower, the http proxy cannot be trusted.

3. Possible solution:
Have mechanism to authenticate the proxy via https for data connection.

Is it same as what you mean?

Comment 17

[Shian-Yow Wu [:swu]]

Patrick, shall we file a different bug for the proxy authentication issue when APN is untrusted?  It's a broader topic which may not be only related to proxy, and possible solution may be either in application level, OS level, or model firmware level.  But before that, I need to make sure we are talking about the same thing.

Comment 18

[Shian-Yow Wu [:swu]]

s/model firmware/modem firmware/

Comment 19

[Shian-Yow Wu [:swu]]

Attachment: Part 1: Support PAC for AppId and IsBrowserElement.

Based on part of the work in bug 1033580 that checks AppId for PAC, the patch adds IsBrowserElement check(iframe with mozbrowser but no mozapp attribute).

Comment 20

[Shian-Yow Wu [:swu]]

Attachment: Part 2: PAC generator for browsing and system wide proxy.

The patch provides:
1. New proxy setting for browsing traffic
   Settings:
     browsing.proxy.host
     browsing.proxy.port
   Prefs:
     network.proxy.browsing_proxy_host
     network.proxy.browsing_proxy_port
2. Provide a PAC generator that generates rules when related prefs change, and make b2g proxy always go with PAC.

Comment 21

[Shian-Yow Wu [:swu]]

Comment on attachment 8548752 [details] [diff] [review]
Part 1: Support PAC for AppId and IsBrowserElement.

Patrick, would you help to feedback on the design with PAC?  Thank you.

Comment 22

[Patrick McManus [:mcmanus]]

(In reply to Shian-Yow Wu [:swu] from comment #17)
> Patrick, shall we file a different bug for the proxy authentication issue
> when APN is untrusted?  It's a broader topic which may not be only related
> to proxy, and possible solution may be either in application level, OS
> level, or model firmware level.  But before that, I need to make sure we are
> talking about the same thing.

let's do that.. and maybe we should schedule a vidyo chat too. Will you propose something?

Comment 23

[Patrick McManus [:mcmanus]]

Comment on attachment 8548752 [details] [diff] [review]
Part 1: Support PAC for AppId and IsBrowserElement.

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

lgtm

Comment 24

[Patrick McManus [:mcmanus]]

Comment on attachment 8548760 [details] [diff] [review]
Part 2: PAC generator for browsing and system wide proxy.

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

makes sense

Comment 25

[Shian-Yow Wu [:swu]]

(In reply to Patrick McManus [:mcmanus] from comment #22)
> (In reply to Shian-Yow Wu [:swu] from comment #17)
> > Patrick, shall we file a different bug for the proxy authentication issue
> > when APN is untrusted?  It's a broader topic which may not be only related
> > to proxy, and possible solution may be either in application level, OS
> > level, or model firmware level.  But before that, I need to make sure we are
> > talking about the same thing.
> 
> let's do that.. and maybe we should schedule a vidyo chat too. Will you
> propose something?

I've sent an e-mail to you regarding this topic.

Comment 26

[Dragana Damjanovic [:dragana]]

Comment on attachment 8548752 [details] [diff] [review]
Part 1: Support PAC for AppId and IsBrowserElement.

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

looks good.

Comment 27

[Shian-Yow Wu [:swu]]

For this bug, we use a PAC generator to set PAC configuration when the proxy settings are changed.
However, using PAC for B2G by default causes testing failure when running mochitest on B2G emulator.
After checking, it's because mochitest framework uses PAC to proxy requests to proper location on the host, which conflicts with this feature.

One possible solution might be using a pref to disable the PAC generator before mochitest, and re-enable it after testing finished.  Though it means we are not able to test PAC functions in mochitest.

Andrew, do you have any suggestions?

1. What could be a good point if we want to disable PAC generator during mochitest?
2. Is there any way if we need to test PAC functions in mochitest?

Comment 28

[Andrew Halberstadt [:ahal]]

Yeah, mochitest needs PAC, and I'm not sure that there's an easy way around that.

1. You could add the pref to this file (currently only mochitest uses it):
https://dxr.mozilla.org/mozilla-central/source/testing/profiles/prefs_general.js
2. I don't know, maybe Joel knows.

If there isn't we could look into using another harness for these tests, like marionette or possibly xpcshell (if you don't need a ui).

Comment 29

[Joel Maher ( :jmaher ) (UTC -8)]

could you just change the network.proxy.type pref to be a value of 0, that would be direct connection.  This could be done temporarily while you want to avoid PAC.  It sounds like this feature doesn't need to be a mochitest specifically (xpcshell sounds like the best candidate), but if so, it should not affect the rest of the harness.

Maybe I don't understand the probably completely yet.

Comment 30

[Shian-Yow Wu [:swu]]

(In reply to Andrew Halberstadt [:ahal] from comment #28)
> 1. You could add the pref to this file (currently only mochitest uses it):
> https://dxr.mozilla.org/mozilla-central/source/testing/profiles/
> prefs_general.js

Thank you!  I will try this.

(In reply to Joel Maher (:jmaher) from comment #29)
> could you just change the network.proxy.type pref to be a value of 0, that
> would be direct connection.

Because mochitest framework requires PAC to work properly, so we cannot just change it to be direct connection, otherwise the mochitest of B2G emulator on try server will fail.

> If there isn't we could look into using another harness for these tests,
> like marionette or possibly xpcshell (if you don't need a ui).

> It sounds like this feature doesn't need to be a mochitest
> specifically (xpcshell sounds like the best candidate)

The xpcshell should be good enough for this moment, thank you!

Comment 31

[Shian-Yow Wu [:swu]]

Attachment: Part 1(v2): Support app info for PAC.

Thanks for the feedback.  The new patch has made the following changes and ready for review.

1. Re-factor for nsIProtocolProxyService.asyncResolve() change that uses channel instead URI(bug 436344).  Thanks to the change, because of that we can get app info from channel and simplify this patch.
2. Add myAppOrigin() API for PAC, which is be needed to identify browser app.
3. Test case.

Comment 32

[Shian-Yow Wu [:swu]]

Attachment: Part 2(v2): PAC generator for browsing and system wide proxy.

This new patch has made the following changes:
1. New pref "b2g.browser.origin" pref for browser app, so we can apply PAC rule to check browser app origin.
2. New pref "network.proxy.browsing.browser_app_only" pref to specify whether to apply browsing proxy setting for browser app only.
3. New pref "network.proxy.pac_generator" to enable/disable PAC generator.

The patch may need little change depending on bug 1120843.

Comment 33

[Shian-Yow Wu [:swu]]

Attachment: Part 2(v3): PAC generator for browsing and system wide proxy.

Changes in this patch:
1. Make PAC generator to be a service.
2. Check for http/https/ftp system proxy settings.
3. Allow to specify multiple app origins that use browsing proxy setting.
4. Test case for PAC generator.

Comment 34

[Shian-Yow Wu [:swu]]

Comment on attachment 8559777 [details] [diff] [review]
Part 2(v3): PAC generator for browsing and system wide proxy.

The patch is now ready for review.

Need help from:
Edgar for changes in dom/system/gonk.
Patrick for the whole design.

Thank you!

Comment 35

[Patrick McManus [:mcmanus]]

Comment on attachment 8559777 [details] [diff] [review]
Part 2(v3): PAC generator for browsing and system wide proxy.

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

nsipacgenerator - that's pretty fun :) I would have it return a pac string rather than installing the string into the prefs itself, but other than that the idea is great.

::: netwerk/base/PACGenerator.js
@@ +38,5 @@
> +                                     classDescription: "PACGenerator",
> +                                     interfaces: [Ci.nsIPACGenerator]}),
> +
> +  /**
> +   * Generate PAC with Data URL.

comment that the input is the non-pac proxy config

@@ +98,5 @@
> +        {"scheme": "http:", "pref": "http"},
> +        {"scheme": "https:", "pref": "ssl"},
> +        {"scheme": "ftp:", "pref": "ftp"}
> +      ];
> +      for (let i = 0; i < proxyTypes.length; i++) {

there is a problem here that you are not checking network.proxy.type.. if someone has installed a http proxy at some point and changed back to direct later on, these old proxy values will still be stored in the prefs (and just ignored because of type). this code is picking them up for the synthesized pac string.

Comment 36

[Edgar Chen [:edgar]]

Comment on attachment 8559777 [details] [diff] [review]
Part 2(v3): PAC generator for browsing and system wide proxy.

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

The changes in dom/system/gonk mostly looks good, but I would like to take a look again after comment #35 and comment below are addressed. Thank you.

::: dom/system/gonk/NetworkService.js
@@ +423,5 @@
> +      usePAC = Services.prefs.getBoolPref("network.proxy.pac_generator");
> +    } catch (ex) {}
> +
> +    if (usePAC) {
> +      gPACGenerator.generate();

I prefer setting proxy type to PROXY_TYPE_PAC here, instead of in PACGenerator.js.

Comment 37

[Shian-Yow Wu [:swu]]

Attachment: Part 2(v4): PAC generator for browsing and system wide proxy.

Thanks for comments from Patrick and Edgar.  Yes, it's better to move setting prefs outside from PACGenerator.js.

Here is the new patch that addressed review comments, please review again.

Comment 38

[Patrick McManus [:mcmanus]]

Comment on attachment 8561862 [details] [diff] [review]
Part 2(v4): PAC generator for browsing and system wide proxy.

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

just one thing

::: dom/system/gonk/NetworkService.js
@@ +36,5 @@
>  const NETD_COMMAND_UNSOLICITED  = 600;
>  
>  const WIFI_CTRL_INTERFACE = "wl0.1";
>  
> +const PROXY_TYPE_MANUAL = 1;

is there a reason you can't use the constants in nsIProtocolProxyService for this?

::: netwerk/base/PACGenerator.js
@@ +90,5 @@
> +        {"scheme": "ftp:", "pref": "ftp"}
> +      ];
> +      for (let i = 0; i < proxyTypes.length; i++) {
> +       try {
> +          host = Services.prefs.getCharPref("network.proxy." +

as I mentioned in the last review, I don't think its safe to use these prefs without consulting network.proxy.type..

Comment 39

[Shian-Yow Wu [:swu]]

(In reply to Patrick McManus [:mcmanus] from comment #38)
> Comment on attachment 8561862 [details] [diff] [review]
> Part 2(v4): PAC generator for browsing and system wide proxy.
> 
> Review of attachment 8561862 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> just one thing
> 
> ::: dom/system/gonk/NetworkService.js
> @@ +36,5 @@
> >  const NETD_COMMAND_UNSOLICITED  = 600;
> >  
> >  const WIFI_CTRL_INTERFACE = "wl0.1";
> >  
> > +const PROXY_TYPE_MANUAL = 1;
> 
> is there a reason you can't use the constants in nsIProtocolProxyService for
> this?

Thank you for this suggestion.

> 
> ::: netwerk/base/PACGenerator.js
> @@ +90,5 @@
> > +        {"scheme": "ftp:", "pref": "ftp"}
> > +      ];
> > +      for (let i = 0; i < proxyTypes.length; i++) {
> > +       try {
> > +          host = Services.prefs.getCharPref("network.proxy." +
> 
> as I mentioned in the last review, I don't think its safe to use these prefs
> without consulting network.proxy.type..

Earlier I thought this check was only needed when PACGenerator writes PAC string to the pref, and if it now just returns the PAC string then the responsibility to check is left to caller.  Now I got your point, the code returns the string by checking proxy type and settings.  So if the network.proxy.type is not PAC, it returns an empty string(and the caller can base on it to clear network.proxy.autoconfig_url).

However, I am concerning one thing.  For caller, there are two sequences to set PAC:
1) Set network.proxy.type to PAC, get a PAC string, then write it to network.proxy.autoconfig_url.
2) Get a PAC string, write it to network.proxy.autoconfig_url, then set network.proxy.type to PAC.

For #1, could there be a short moment that old/incorrect PAC string is used when switching from non-PAC to PAC?  If this won't happen or could be avoided, then we can check network.proxy.type inside the PACGenerator and it would be perfect.

Comment 40

[Shian-Yow Wu [:swu]]

Attachment: Part 2(v5): PAC generator for browsing and system wide proxy.

This patch has the following changes:
1. Use constants in nsIProtocolProxyService for proxy type.
2. Add validation check on proxy host for security.

May still need to change depending on the clarification on comment 39.

Comment 41

[Patrick McManus [:mcmanus]]

Comment on attachment 8563212 [details] [diff] [review]
Part 2(v5): PAC generator for browsing and system wide proxy.

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

::: netwerk/base/PACGenerator.js
@@ +122,5 @@
> +        {"scheme": "ftp:", "pref": "ftp"}
> +      ];
> +      for (let i = 0; i < proxyTypes.length; i++) {
> +       try {
> +          host = Services.prefs.getCharPref("network.proxy." +

What I am trying to say is that if, before your code runs, if network.proxy.type does not equal MANUAL then the value of network.proxy.http is meaningless. You can think of it like a c union - the type defines which members are meaningful.

Other bits of priv'd javascript could be messing with these prefs just like you are :)..

Comment 42

[Shian-Yow Wu [:swu]]

(In reply to Patrick McManus [:mcmanus] from comment #41)
> What I am trying to say is that if, before your code runs, if
> network.proxy.type does not equal MANUAL then the value of
> network.proxy.http is meaningless. You can think of it like a c union - the
> type defines which members are meaningful.
> 
> Other bits of priv'd javascript could be messing with these prefs just like
> you are :)..

Now I really got what you mean. :)

If we see network.proxy.http only meaningful when network.proxy.type is MANUAL, the caller has to switch to MANUAL every time before generating PAC string, then switch back to PAC.  During this period, the other proxy rules(e.g. browsing proxy setting) rely on PAC will be inactive.

Can we think of it in this way?
1. If network.proxy.type is MANUAL, network.proxy.http is used as the single global proxy setting.
2. If network.proxy.type is PAC, network.proxy.http is used to generate PAC rule. 

Different type gives different meaning of its members.

Comment 43

[Patrick McManus [:mcmanus]]

(In reply to Shian-Yow Wu [:swu] from comment #42)
> (In reply to Patrick McManus [:mcmanus] from comment #41)
> > What I am trying to say is that if, before your code runs, if
> > network.proxy.type does not equal MANUAL then the value of
> > network.proxy.http is meaningless. You can think of it like a c union - the
> > type defines which members are meaningful.
> > 
> > Other bits of priv'd javascript could be messing with these prefs just like
> > you are :)..
> 
> Now I really got what you mean. :)
> 
> If we see network.proxy.http only meaningful when network.proxy.type is
> MANUAL, the caller has to switch to MANUAL every time before generating PAC
> string, then switch back to PAC.  During this period, the other proxy
> rules(e.g. browsing proxy setting) rely on PAC will be inactive.
> 
> Can we think of it in this way?
> 1. If network.proxy.type is MANUAL, network.proxy.http is used as the single
> global proxy setting.
> 2. If network.proxy.type is PAC, network.proxy.http is used to generate PAC
> rule. 
> 
> Different type gives different meaning of its members.

I'm sorry about the delay in responding.. I had a bad merge incident with my todo list and some stuff got lost. I just found it now by querying bugzilla for stuff waiting for me :(

I think I'd prefer a new bool pref that indicated that you wanted to trust the network.proxy.http value even though we aren't in manual mode. afterall, something else could use PAC and you would get a broken semantic otherwise.

I'm only kinda firm on this one because, at least in desktop, we've had regressions around this in the past.

Comment 44

[Shian-Yow Wu [:swu]]

(In reply to Patrick McManus [:mcmanus] from comment #43)
> I'm sorry about the delay in responding.. I had a bad merge incident with my
> todo list and some stuff got lost. I just found it now by querying bugzilla
> for stuff waiting for me :(

No worries.

> 
> I think I'd prefer a new bool pref that indicated that you wanted to trust
> the network.proxy.http value even though we aren't in manual mode. afterall,
> something else could use PAC and you would get a broken semantic otherwise.
> 
> I'm only kinda firm on this one because, at least in desktop, we've had
> regressions around this in the past.

We have a bool pref "network.proxy.pac_generator" which only been enabled for B2G in this patch.  Only when this pref is true will make PACGenerator.generate() been called to generate rules from "network.proxy.http" for PAC.  If it's false, PACGenerator.generate() will not be called, thus "network.proxy.http" will never been used for PAC.

This works the same logic as your suggestion, the only difference is whether to check this bool pref before calling or inside PACGenerator.generate().  Since we are already checking it before calling, we shouldn't need to check it again inside, what do you think?

Comment 45

[Patrick McManus [:mcmanus]]

this seems fully synchronous and single threaded right? The one check should be fine

Comment 46

[Shian-Yow Wu [:swu]]

(In reply to Patrick McManus [:mcmanus] from comment #45)
> this seems fully synchronous and single threaded right? The one check should
> be fine

Yes, that's right.

Comment 47

[Shian-Yow Wu [:swu]]

Attachment: Part 2(v5): PAC generator for browsing and system wide proxy.

The concern on network.proxy.http semantic is now clarified, the patch is ready for review.

Please help to review overall design(Patrick) and dom/system/gonk(Edgar), thank you.

Comment 48

[Patrick McManus [:mcmanus]]

Comment on attachment 8579133 [details] [diff] [review]
Part 2(v5): PAC generator for browsing and system wide proxy.

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

::: dom/system/gonk/NetworkService.js
@@ +535,5 @@
> +      Services.prefs.setCharPref("network.proxy.autoconfig_url",
> +                                 gPACGenerator.generate());
> +      Services.prefs.setIntPref("network.proxy.type", PROXY_TYPE_PAC);
> +    } else {
> +      Services.prefs.clearUserPref("network.proxy.type");

I think you need to pop the old value back here.. it could also have been a userpref -right?

Comment 49

[Shian-Yow Wu [:swu]]

(In reply to Patrick McManus [:mcmanus] from comment #48)
> Comment on attachment 8579133 [details] [diff] [review]
> Part 2(v5): PAC generator for browsing and system wide proxy.
> 
> Review of attachment 8579133 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/NetworkService.js
> @@ +535,5 @@
> > +      Services.prefs.setCharPref("network.proxy.autoconfig_url",
> > +                                 gPACGenerator.generate());
> > +      Services.prefs.setIntPref("network.proxy.type", PROXY_TYPE_PAC);
> > +    } else {
> > +      Services.prefs.clearUserPref("network.proxy.type");
> 
> I think you need to pop the old value back here.. it could also have been a
> userpref -right?

This code inside clearNetworkProxy() is intended to clear system wide proxy settings.  When network.proxy.pac_generator is false, it will behave as original design(i.e. no proxy settings at all), so we are not popping old value here.

Comment 50

[Patrick McManus [:mcmanus]]

Comment on attachment 8579133 [details] [diff] [review]
Part 2(v5): PAC generator for browsing and system wide proxy.

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

::: dom/system/gonk/NetworkService.js
@@ +508,5 @@
> +        Services.prefs.setCharPref("network.proxy.autoconfig_url",
> +                                   gPACGenerator.generate());
> +        Services.prefs.setIntPref("network.proxy.type", PROXY_TYPE_PAC);
> +      } else {
> +        Services.prefs.setIntPref("network.proxy.type", PROXY_TYPE_MANUAL);

so this is the push I was referring to.. it is when pac_generator is false. am I misreading?

Comment 51

[Shian-Yow Wu [:swu]]

(In reply to Patrick McManus [:mcmanus] from comment #50)
> Comment on attachment 8579133 [details] [diff] [review]
> Part 2(v5): PAC generator for browsing and system wide proxy.
> 
> Review of attachment 8579133 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/NetworkService.js
> @@ +508,5 @@
> > +        Services.prefs.setCharPref("network.proxy.autoconfig_url",
> > +                                   gPACGenerator.generate());
> > +        Services.prefs.setIntPref("network.proxy.type", PROXY_TYPE_PAC);
> > +      } else {
> > +        Services.prefs.setIntPref("network.proxy.type", PROXY_TYPE_MANUAL);
> 
> so this is the push I was referring to.. it is when pac_generator is false.
> am I misreading?

On B2G, the original design has only manual proxy type or direct connection, and 
there are two functions in NetworkService.js for proxy setting:
  setNetworkProxy() -> Set proxy settings and set proxy type to manual
  clearNetworkProxy() -> Clear proxy settings and clear proxy type (i.e. sets direct connection to internet)

When network.proxy.pac_generator is true, the proxy type for both functions are set to PAC to take over the proxy routing.
When network.proxy.pac_generator is false, the code just keeps the original behavior(manual or direct).

Comment 52

[Edgar Chen [:edgar]]

Comment on attachment 8579133 [details] [diff] [review]
Part 2(v5): PAC generator for browsing and system wide proxy.

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

r=me for the changes in dom/system/gonk. Thank you.

Comment 53

[Shian-Yow Wu [:swu]]

Attachment: Part 2(v6): PAC generator for browsing and system wide proxy. (r=echen for dom/system/gonk)

The patch adds the following changes:
1. Direct connection for localhost/127.0.0.1 in PAC rule(for bug 1146215).
2. The generate() function is only effective when the network.proxy.pac_generator preference is true, otherwise it returns empty PAC rule.

The question in comment 48 and comment 50 regarding old type push/pop was replied in comment 51, basically it should not be a problem by original design logic that only has manual/direct mode.  If it could be a problem, we can file a separate bug for it.

Everything been discussed earlier should have been covered, please review again, thank you.

Comment 54

[Patrick McManus [:mcmanus]]

Comment on attachment 8582246 [details] [diff] [review]
Part 2(v6): PAC generator for browsing and system wide proxy. (r=echen for dom/system/gonk)

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

I think this is a little short sighted in the assumption that the universe of components you're aware of are the only ones that can (or in the future will be able to) change that pref so you don't need push/pop.. but maybe I still don't understand your reasoning... there is no reason to not go ahead at this point however, its a minor point.

Comment 55

[Shian-Yow Wu [:swu]]

(In reply to Patrick McManus [:mcmanus] from comment #54)
> Comment on attachment 8582246 [details] [diff] [review]
> Part 2(v6): PAC generator for browsing and system wide proxy. (r=echen for
> dom/system/gonk)
> 
> Review of attachment 8582246 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think this is a little short sighted in the assumption that the universe
> of components you're aware of are the only ones that can (or in the future
> will be able to) change that pref so you don't need push/pop.. but maybe I
> still don't understand your reasoning... there is no reason to not go ahead
> at this point however, its a minor point.

Thank you for the review and reminding this point.

This is a point that exists in original design before the patch.  For a patch that implements a new feature, in disabled case, shouldn't we try to keep the original design as unchanged as possible(even there are things to improve)?

Comment 56

[Shian-Yow Wu [:swu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c6a9943ddf80
https://hg.mozilla.org/integration/mozilla-inbound/rev/748b96aa103b

Try server result looks good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4b0d28625d3

Comment 57

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c6a9943ddf80
https://hg.mozilla.org/mozilla-central/rev/748b96aa103b

Comment 59

[Philip Chee]

https://hg.mozilla.org/mozilla-central/rev/748b96aa103b#l13.12
Shouldn't this test be B2G only?

Comment 60

[Shian-Yow Wu [:swu]]

(In reply to Philip Chee from comment #59)
> https://hg.mozilla.org/mozilla-central/rev/748b96aa103b#l13.12
> Shouldn't this test be B2G only?

Yes, it should be B2G only, will do it in bug 1148503.