Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15665006:
Allow specifying proxy scripts using file:// URLs. (Closed)
|
Created:
7 years, 6 months ago by pauljensen Modified:
7 years, 6 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAllow specifying proxy scripts using file:, data:, and ftp: URLs. This addresses a regression from r198915.
BUG=243974
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=203074
Patch Set 1 #Patch Set 2 : Add data: URL support #Patch Set 3 : Add ftp: URL support #
Total comments: 1
Patch Set 4 : Fix indent #Patch Set 5 : Add test. #
Total comments: 11
Patch Set 6 : Address comments. #Patch Set 7 : Look for ERR_ string #
Messages
Total messages: 26 (0 generated)
Matt, PTAL. I plan to add a regression test for this bug also, do you think it's worth delaying this CL to add the regression test?
On 2013/05/28 14:59:28, pauljensen wrote: > Matt, PTAL. I plan to add a regression test for this bug also, do you think > it's worth delaying this CL to add the regression test? How long do you think it'll take you to write a test? Doesn't seem to be much testing of pac scripts, browser-side, except chrome/browser/net/proxy_browsertest.cc, which just contains a websockets test. If it's just a couple days, I think it's fine to hold off. Longer, and it may be worth just landing it.
Will the proxy script fetching support FTP in the new mode? (it should). Regarding tests, the extension API for proxy may have existing tests that can be plugged into. Lastly, I wouldn't block on having the browser-side unit-tests, so long as you have done some manual testing.
Matt, I added the FTP support and manually verified PAC scripts can be fetched via file, data and FTP URLs. I think I could get a test for this working in a day or so (hopefully much less). Given Eric's comment , do you want to review it as-is or are you more comfortable waiting for the test?
Ok, let's land the tests in a followup CL. LGTM. https://codereview.chromium.org/15665006/diff/12001/chrome/browser/io_thread.cc File chrome/browser/io_thread.cc (right): https://codereview.chromium.org/15665006/diff/12001/chrome/browser/io_thread.... chrome/browser/io_thread.cc:570: globals_->proxy_script_fetcher_ftp_transaction_factory.get())); nit: Fix indent. (All 3 lines should be indented 2 more)
lgtm. minor nit -- in the CL description you mention data:// URL. Suggest changing to just data:
James, PTAL at this small CL to fix a Pri-1 bug. Current reviewers agreed that tests can be added promptly in a follow-up CL to avoid holding up this fix.
On 2013/05/28 19:14:36, pauljensen wrote: > James, PTAL at this small CL to fix a Pri-1 bug. Current reviewers agreed that > tests can be added promptly in a follow-up CL to avoid holding up this fix. I disagree with that assessment. If we can't write a test in a day (or two at most), then we have a bigger problem. I see two viable solutions from reading this CL and the bug: a) Revert the CL in question [r198915], or b) Add a test for this CL before landing.
Matt & Eric, I added tests for fetching PAC scripts via HTTP, FTP, file:// and data: schemes. The io_thread.* files did not change. PTAL.
Thanks for writing the test; I appreciate it. LGTM with optional nit. https://codereview.chromium.org/15665006/diff/32002/chrome/browser/net/proxy_... File chrome/browser/net/proxy_browsertest.cc (right): https://codereview.chromium.org/15665006/diff/32002/chrome/browser/net/proxy_... chrome/browser/net/proxy_browsertest.cc:127: const base::FilePath::CharType kPACScript[] = FILE_PATH_LITERAL( Optional nit: Move helpers and related constants to the top of the file.
thanks for the test. still lgtm https://codereview.chromium.org/15665006/diff/32002/chrome/browser/net/proxy_... File chrome/browser/net/proxy_browsertest.cc (right): https://codereview.chromium.org/15665006/diff/32002/chrome/browser/net/proxy_... chrome/browser/net/proxy_browsertest.cc:134: const char *expected_title = "http://google.com/ is not available"; nit: asterisk on the left. nit: [optional] make it more const: const char expected_title[] = "...."
Seems to me like one potential problem with this approach is that if we ever decide just to fail when given a weird PAC URL, then these tests would then fail to test for the regression they're targetted at. Do we care? Is this likely to happen? https://codereview.chromium.org/15665006/diff/32002/chrome/browser/net/proxy_... File chrome/browser/net/proxy_browsertest.cc (right): https://codereview.chromium.org/15665006/diff/32002/chrome/browser/net/proxy_... chrome/browser/net/proxy_browsertest.cc:136: ui_test_utils::NavigateToURL(browser, GURL("http://google.com")); Not a big fan of depending on UI strings, since I don't think these tests should have to be updated whenever we change strings. Think we can get rid of the TitleWatcher and the title check. https://codereview.chromium.org/15665006/diff/32002/chrome/browser/net/proxy_... chrome/browser/net/proxy_browsertest.cc:146: " 'Unable to connect to the proxy server') >= 0;" Rather than depend on UI strings, how about searching for "ERR_PROXY_CONNECTION_FAILED"?
The tests should be regression-proof. I like the technique that Paul is using. ERR_PROXY_CONNECTION_FAILED is very specific to a failure connecting to the proxy server. If the command line were to stop working one day, then no PAC script would be getting specified to the browser, and the tests would hence be defaulting to DIRECT, and therefore have no chance to generate that error.
On 2013/05/29 18:20:42, eroman wrote: > The tests should be regression-proof. I like the technique that Paul is using. > > ERR_PROXY_CONNECTION_FAILED is very specific to a failure connecting to the > proxy server. If the command line were to stop working one day, then no PAC > script would be getting specified to the browser, and the tests would hence be > defaulting to DIRECT, and therefore have no chance to generate that error. I'm not concerned about the command line not working, but with all funky PAC urls returning the same error, like, say, --proxy-pac-url=goat://hat returning the same error as --proxy-pac-url=ftp://whatever
I am not too concerned about that, since each test involves a separate browser launch, and hence it would be difficult for a successful load of the PAC script to spill over between tests. I am happy with the tests as-is, however one possibility to address your concern would be to have separate PAC script loaded over each protocol. The scripts could return 0.0.0.0 ONLY in response to a magic url, and return DIRECT for everything else. The browsertests would now resolve the proxy for the respective magic URLs, rather than the same URL in each test.
On 2013/05/29 18:32:12, eroman wrote: > I am not too concerned about that, since each test involves a separate browser > launch, and hence it would be difficult for a successful load of the PAC script > to spill over between tests. It's not a successful load spilling over between tests... The original bug was that we didn't attach a handler for data URLs to the context used to get PACs. My concerns is that, at some point, a future putative CL could result in URLs with known schemes but pointing to resources that can't be fetched could return the same error code as PAC URLs for unrecognized schemes (Which we currently just completely ignore, which seems a little weird to me). > I am happy with the tests as-is, however one possibility to address your concern > would be to have separate PAC script loaded over each protocol. > > The scripts could return 0.0.0.0 ONLY in response to a magic url, and return > DIRECT for everything else. > > The browsertests would now resolve the proxy for the respective magic URLs, > rather than the same URL in each test. You're the expert on PAC behavior, I defer to you on this - I'm not familiar with the PAC fetching code. Since ERR_PROXY_CONNECTION_FAILED is as explicit as you say (We definitely have a valid, downloaded PAC file, but either can't run it (see the test with "data:,") or can't connect to it, then this sounds like it should be fine).
https://codereview.chromium.org/15665006/diff/32002/chrome/browser/net/proxy_... File chrome/browser/net/proxy_browsertest.cc (right): https://codereview.chromium.org/15665006/diff/32002/chrome/browser/net/proxy_... chrome/browser/net/proxy_browsertest.cc:127: const base::FilePath::CharType kPACScript[] = FILE_PATH_LITERAL( On 2013/05/29 17:24:33, James Hawkins wrote: > Optional nit: Move helpers and related constants to the top of the file. Done. https://codereview.chromium.org/15665006/diff/32002/chrome/browser/net/proxy_... chrome/browser/net/proxy_browsertest.cc:134: const char *expected_title = "http://google.com/ is not available"; On 2013/05/29 17:45:52, eroman wrote: > nit: asterisk on the left. > nit: [optional] make it more const: const char expected_title[] = "...." I deleted this code per mmenke's comment. https://codereview.chromium.org/15665006/diff/32002/chrome/browser/net/proxy_... chrome/browser/net/proxy_browsertest.cc:136: ui_test_utils::NavigateToURL(browser, GURL("http://google.com")); On 2013/05/29 18:06:25, mmenke wrote: > Not a big fan of depending on UI strings, since I don't think these tests should > have to be updated whenever we change strings. Think we can get rid of the > TitleWatcher and the title check. Done. https://codereview.chromium.org/15665006/diff/32002/chrome/browser/net/proxy_... chrome/browser/net/proxy_browsertest.cc:146: " 'Unable to connect to the proxy server') >= 0;" On 2013/05/29 18:06:25, mmenke wrote: > Rather than depend on UI strings, how about searching for > "ERR_PROXY_CONNECTION_FAILED"? I tried that first, but ERR_PROXY_CONNECTION_FAILED isn't visible to the user or via document.body.textContent. If you press the button then it's visible but I didn't want to complicate the test to do this. I don't know of a simple way to lookup the UI string for ERR_PROXY_CONNECTION_FAILED either. This is something we could always address later if we find ourselves changing this string frequently.
https://codereview.chromium.org/15665006/diff/32002/chrome/browser/net/proxy_... File chrome/browser/net/proxy_browsertest.cc (right): https://codereview.chromium.org/15665006/diff/32002/chrome/browser/net/proxy_... chrome/browser/net/proxy_browsertest.cc:146: " 'Unable to connect to the proxy server') >= 0;" On 2013/05/29 18:45:15, pauljensen wrote: > On 2013/05/29 18:06:25, mmenke wrote: > > Rather than depend on UI strings, how about searching for > > "ERR_PROXY_CONNECTION_FAILED"? > I tried that first, but ERR_PROXY_CONNECTION_FAILED isn't visible to the user or > via document.body.textContent. If you press the button then it's visible but I > didn't want to complicate the test to do this. I don't know of a simple way to > lookup the UI string for ERR_PROXY_CONNECTION_FAILED either. This is something > we could always address later if we find ourselves changing this string > frequently. Hmm... textContent includes the entire raw page, including Javascript and hidden HTML text. The string should be present both in the Javascript and in the page body.
https://codereview.chromium.org/15665006/diff/32002/chrome/browser/net/proxy_... File chrome/browser/net/proxy_browsertest.cc (right): https://codereview.chromium.org/15665006/diff/32002/chrome/browser/net/proxy_... chrome/browser/net/proxy_browsertest.cc:146: " 'Unable to connect to the proxy server') >= 0;" On 2013/05/29 18:47:51, mmenke wrote: > On 2013/05/29 18:45:15, pauljensen wrote: > > On 2013/05/29 18:06:25, mmenke wrote: > > > Rather than depend on UI strings, how about searching for > > > "ERR_PROXY_CONNECTION_FAILED"? > > I tried that first, but ERR_PROXY_CONNECTION_FAILED isn't visible to the user > or > > via document.body.textContent. If you press the button then it's visible but > I > > didn't want to complicate the test to do this. I don't know of a simple way > to > > lookup the UI string for ERR_PROXY_CONNECTION_FAILED either. This is > something > > we could always address later if we find ourselves changing this string > > frequently. > > Hmm... textContent includes the entire raw page, including Javascript and > hidden HTML text. The string should be present both in the Javascript and in > the page body. Just tested it manually with ERR_UNSAFE_PORT, and it worked fine. There's are also other tests that does just this (See src/chrome/browser/policy/policy_browsertest.cc and src/chrome/browser/errorpage_browsertest.cc)
https://codereview.chromium.org/15665006/diff/32002/chrome/browser/net/proxy_... File chrome/browser/net/proxy_browsertest.cc (right): https://codereview.chromium.org/15665006/diff/32002/chrome/browser/net/proxy_... chrome/browser/net/proxy_browsertest.cc:146: " 'Unable to connect to the proxy server') >= 0;" On 2013/05/29 18:50:28, mmenke wrote: > On 2013/05/29 18:47:51, mmenke wrote: > > On 2013/05/29 18:45:15, pauljensen wrote: > > > On 2013/05/29 18:06:25, mmenke wrote: > > > > Rather than depend on UI strings, how about searching for > > > > "ERR_PROXY_CONNECTION_FAILED"? > > > I tried that first, but ERR_PROXY_CONNECTION_FAILED isn't visible to the > user > > or > > > via document.body.textContent. If you press the button then it's visible > but > > I > > > didn't want to complicate the test to do this. I don't know of a simple way > > to > > > lookup the UI string for ERR_PROXY_CONNECTION_FAILED either. This is > > something > > > we could always address later if we find ourselves changing this string > > > frequently. > > > > Hmm... textContent includes the entire raw page, including Javascript and > > hidden HTML text. The string should be present both in the Javascript and in > > the page body. > > Just tested it manually with ERR_UNSAFE_PORT, and it worked fine. There's are > also other tests that does just this (See > src/chrome/browser/policy/policy_browsertest.cc and > src/chrome/browser/errorpage_browsertest.cc) Huh, you're right. I've changed it to look for ERR_PROXY_CONNECTION_FAILED.
On 2013/05/29 18:54:51, pauljensen wrote: > https://codereview.chromium.org/15665006/diff/32002/chrome/browser/net/proxy_... > File chrome/browser/net/proxy_browsertest.cc (right): > > https://codereview.chromium.org/15665006/diff/32002/chrome/browser/net/proxy_... > chrome/browser/net/proxy_browsertest.cc:146: " 'Unable to connect to the > proxy server') >= 0;" > On 2013/05/29 18:50:28, mmenke wrote: > > On 2013/05/29 18:47:51, mmenke wrote: > > > On 2013/05/29 18:45:15, pauljensen wrote: > > > > On 2013/05/29 18:06:25, mmenke wrote: > > > > > Rather than depend on UI strings, how about searching for > > > > > "ERR_PROXY_CONNECTION_FAILED"? > > > > I tried that first, but ERR_PROXY_CONNECTION_FAILED isn't visible to the > > user > > > or > > > > via document.body.textContent. If you press the button then it's visible > > but > > > I > > > > didn't want to complicate the test to do this. I don't know of a simple > way > > > to > > > > lookup the UI string for ERR_PROXY_CONNECTION_FAILED either. This is > > > something > > > > we could always address later if we find ourselves changing this string > > > > frequently. > > > > > > Hmm... textContent includes the entire raw page, including Javascript and > > > hidden HTML text. The string should be present both in the Javascript and > in > > > the page body. > > > > Just tested it manually with ERR_UNSAFE_PORT, and it worked fine. There's are > > also other tests that does just this (See > > src/chrome/browser/policy/policy_browsertest.cc and > > src/chrome/browser/errorpage_browsertest.cc) > Huh, you're right. I've changed it to look for ERR_PROXY_CONNECTION_FAILED. Thanks for bearing with me. LGTM!
On 2013/05/29 18:56:46, mmenke wrote: > On 2013/05/29 18:54:51, pauljensen wrote: > > > https://codereview.chromium.org/15665006/diff/32002/chrome/browser/net/proxy_... > > File chrome/browser/net/proxy_browsertest.cc (right): > > > > > https://codereview.chromium.org/15665006/diff/32002/chrome/browser/net/proxy_... > > chrome/browser/net/proxy_browsertest.cc:146: " 'Unable to connect to the > > proxy server') >= 0;" > > On 2013/05/29 18:50:28, mmenke wrote: > > > On 2013/05/29 18:47:51, mmenke wrote: > > > > On 2013/05/29 18:45:15, pauljensen wrote: > > > > > On 2013/05/29 18:06:25, mmenke wrote: > > > > > > Rather than depend on UI strings, how about searching for > > > > > > "ERR_PROXY_CONNECTION_FAILED"? > > > > > I tried that first, but ERR_PROXY_CONNECTION_FAILED isn't visible to the > > > user > > > > or > > > > > via document.body.textContent. If you press the button then it's > visible > > > but > > > > I > > > > > didn't want to complicate the test to do this. I don't know of a simple > > way > > > > to > > > > > lookup the UI string for ERR_PROXY_CONNECTION_FAILED either. This is > > > > something > > > > > we could always address later if we find ourselves changing this string > > > > > frequently. > > > > > > > > Hmm... textContent includes the entire raw page, including Javascript and > > > > hidden HTML text. The string should be present both in the Javascript and > > in > > > > the page body. > > > > > > Just tested it manually with ERR_UNSAFE_PORT, and it worked fine. There's > are > > > also other tests that does just this (See > > > src/chrome/browser/policy/policy_browsertest.cc and > > > src/chrome/browser/errorpage_browsertest.cc) > > Huh, you're right. I've changed it to look for ERR_PROXY_CONNECTION_FAILED. > > Thanks for bearing with me. LGTM! Oh, and Eric put my other concern completely to rest via IM.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pauljensen@chromium.org/15665006/51001
Retried try job too often on win_rel for step(s) chrome_frame_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pauljensen@chromium.org/15665006/51001
Message was sent while issue was closed.
Change committed as 203074 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
