Support bypassList in Proxy Settings API.

Support bypassList in Proxy Settings API.


BUG=72549, 67778
TEST=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=74811

[battre, 2011-02-10 15:05:02]

Hi Eric,

could you please review this CL and let me know whether you had more in mind
regarding http://crbug.com/67778.

Thanks,
Dominic

[battre, 2011-02-10 18:12:10]

Sorry, had do upload a new version to fix a unit test.

[eroman, 2011-02-11 04:52:28]

http://codereview.chromium.org/6469030/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_proxy_api.cc (right):

http://codereview.chromium.org/6469030/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_proxy_api.cc:178: // caller in a proxy
configuration and returns it as a joined string.
The wording of this comment doesn't seem right.

http://codereview.chromium.org/6469030/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_proxy_api.cc:256: if (proxy_rules &&
!GetBypassList(proxy_rules, &bypass_list)) {
There are string encoding problems in this file.

Note that strings originating from the javascript are wide. When calling
DictionaryValue:GetString() with a std::string, really we are getting UTF-8
encoded strings.

However a lot of this file is doing tokenizing assuming an ASCII input. And
certainly the net::* proxy parsing functions which are ultimately passed these
inputs, is similarly expecting ASCII input.

I generally recommend avoiding this pitfall by only ever extracting strings from
Value's using string16 -- this forces the code to have to deal with unicode of
the string, and avoids passing it around to functions that take ASCII inputs
(since you get compiletime failures!)

For now you might want to call DictionaryValue::GetStringAsASCII(), so the other
code doesn't need to be prepared to handle UTF8, and then just file some
TODOs/bugs on handling wide inputs.

You might be wandering whether it makes sense for javascript to be passing wide
strings -- yes it does. As far as the network stack is concerned, hostnames are
always ASCII. However as far as the user is concerned, international hostnames
can be represented using unicode (there is a translation layer to ASCII called
punycode).

This same issues has come up for PAC scripts, which similarly are javascript
code doing proxy stuff. We currently handle wide hostnames issued by PAC scripts
correctly (in the dnsResolve()/dnsResolveEx() bindings. However there is an
outstanding bug to create UTF16 aware parsing routines for proxy lists. See
http://crbug.com/47234.

http://codereview.chromium.org/6469030/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_proxy_api.cc:385: return false;
See comment below.

http://codereview.chromium.org/6469030/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_proxy_api.cc:392: return false;
Isn't this going to leak |rules_dict|?

I suggest putting rules_dict into a scoped_ptr<> to avoid that.

http://codereview.chromium.org/6469030/diff/2001/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/proxy/bypass/test.js (right):

http://codereview.chromium.org/6469030/diff/2001/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/proxy/bypass/test.js:31: bypassList:
["localhost", "::1", "foo.bar"]
side note: there are also some "magical" values which will be supported in the
bypass list accidentally because it is sharing the parsing with windows (for
instance, "<local>").

[battre, 2011-02-11 15:06:02]

I have addressed your comments. Thanks.

http://codereview.chromium.org/6469030/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_proxy_api.cc (right):

http://codereview.chromium.org/6469030/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_proxy_api.cc:178: // caller in a proxy
configuration and returns it as a joined string.
On 2011/02/11 04:52:28, eroman wrote:
> The wording of this comment doesn't seem right.

Done.

http://codereview.chromium.org/6469030/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_proxy_api.cc:256: if (proxy_rules &&
!GetBypassList(proxy_rules, &bypass_list)) {
On 2011/02/11 04:52:28, eroman wrote:
> There are string encoding problems in this file.
> 
> Note that strings originating from the javascript are wide. When calling
> DictionaryValue:GetString() with a std::string, really we are getting UTF-8
> encoded strings.
> 
> However a lot of this file is doing tokenizing assuming an ASCII input. And
> certainly the net::* proxy parsing functions which are ultimately passed these
> inputs, is similarly expecting ASCII input.
> 
> I generally recommend avoiding this pitfall by only ever extracting strings
from
> Value's using string16 -- this forces the code to have to deal with unicode of
> the string, and avoids passing it around to functions that take ASCII inputs
> (since you get compiletime failures!)
> 
> For now you might want to call DictionaryValue::GetStringAsASCII(), so the
other
> code doesn't need to be prepared to handle UTF8, and then just file some
> TODOs/bugs on handling wide inputs.
> 
> You might be wandering whether it makes sense for javascript to be passing
wide
> strings -- yes it does. As far as the network stack is concerned, hostnames
are
> always ASCII. However as far as the user is concerned, international hostnames
> can be represented using unicode (there is a translation layer to ASCII called
> punycode).
> 
> This same issues has come up for PAC scripts, which similarly are javascript
> code doing proxy stuff. We currently handle wide hostnames issued by PAC
scripts
> correctly (in the dnsResolve()/dnsResolveEx() bindings. However there is an
> outstanding bug to create UTF16 aware parsing routines for proxy lists. See
> http://crbug.com/47234.

I have replaced all GetString calls with GetStringASCII where extension_api.json
prescribes an enum value and therefore guarantees an ASCII value.

For URLs I have used GetString(string16*), and bail out if IsStringASCII(string)
returns false. Otherwise I convert the string with UTF16ToASCII().

I have added TODO(battre)s with a reference to http://crbug.com/72692 in the
code.

So for now, the proxy settings API will accept only ASCII strings. I will do the
UTF-8 CodeLab and see how I can resolve this.

Done.

http://codereview.chromium.org/6469030/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_proxy_api.cc:385: return false;
On 2011/02/11 04:52:28, eroman wrote:
> See comment below.

Done.

http://codereview.chromium.org/6469030/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_proxy_api.cc:392: return false;
On 2011/02/11 04:52:28, eroman wrote:
> Isn't this going to leak |rules_dict|?
> 
> I suggest putting rules_dict into a scoped_ptr<> to avoid that.

Done.

http://codereview.chromium.org/6469030/diff/2001/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/proxy/bypass/test.js (right):

http://codereview.chromium.org/6469030/diff/2001/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/proxy/bypass/test.js:31: bypassList:
["localhost", "::1", "foo.bar"]
On 2011/02/11 04:52:28, eroman wrote:
> side note: there are also some "magical" values which will be supported in the
> bypass list accidentally because it is sharing the parsing with windows (for
> instance, "<local>").

I realized this when writing the documentation for the proxy settinsg API. I
have added a <local> field as well. Just to demonstrate this.

Done.

[eroman, 2011-02-11 19:31:45]

LGTM