Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
6730021:
crash-reporter: Create a list_proxies command (Closed)
|
Created:
9 years, 9 months ago by Michael Krebs Modified:
9 years, 3 months ago Reviewers:
kmixter1 CC:
chromium-os-reviews_chromium.org Visibility:
Public. |
Descriptioncrash-reporter: Create a list_proxies command
Create a list_proxies command to be used by crash_sender to determine the correct proxy for a URL. This uses D-Bus to get them from either the browser or the session manager.
BUG=chromium-os:6828
TEST=Ran list_proxies command manually to test response for various URLs: "http://...", "https://...", empty/NULL, and invalid (eg. just "blah.com"). Also did this for the various proxy settings in the browser: direct, manual proxy configuration, and automatic proxy configuration. Ran list_proxies command using invalid D-Bus method name for proxy_resolver.StartMonitoring() to make sure timeout works. Ran a modified crash_sender manually to make sure it passed the appropriate "--proxy" arguments correctly.
Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=58451b5
Patch Set 1 #
Total comments: 28
Patch Set 2 : Commit most of the changes kmixter requested in the code review #Patch Set 3 : Fix a compile error. #
Total comments: 14
Patch Set 4 : Fix some issues per the codereview. #
Total comments: 14
Patch Set 5 : Fix remaining issues per the codereview (maybe?) #
Total comments: 2
Patch Set 6 : Reorder command-line flags per codereview #Messages
Total messages: 12 (0 generated)
Here's the initial CL for the get_proxies program.
Some comments on first pass. You'll want to add a few unit tests - string parsing ones are low-hanging. http://codereview.chromium.org/6730021/diff/1/Makefile File Makefile (right): http://codereview.chromium.org/6730021/diff/1/Makefile#newcode6 Makefile:6: GET_PROXIES = get_proxies Would add _BIN http://codereview.chromium.org/6730021/diff/1/Makefile#newcode27 Makefile:27: $(shell $(PKG_CONFIG) --libs gobject-2.0 dbus-1 dbus-glib-1) prefer to not link gobject/dbus into crash reporter lest someone start using it. http://codereview.chromium.org/6730021/diff/1/Makefile#newcode29 Makefile:29: # MTEMP: GET_PROXIES_LIBS = $(COMMON_LIBS) -lprotobuf remove? http://codereview.chromium.org/6730021/diff/1/get_proxies.cc File get_proxies.cc (right): http://codereview.chromium.org/6730021/diff/1/get_proxies.cc#newcode8 get_proxies.cc:8: #include <chromeos/dbus/dbus.h> existing style is to put this in ""s with the other ChromeOS-specific includes. http://codereview.chromium.org/6730021/diff/1/get_proxies.cc#newcode14 get_proxies.cc:14: #include "base/json/json_reader.h" abc order http://codereview.chromium.org/6730021/diff/1/get_proxies.cc#newcode43 get_proxies.cc:43: // For ShowSessionProxies() Why do we need to get the proxy setting directly from session_manager - shouldn't Chrome correctly decide when to use that? http://codereview.chromium.org/6730021/diff/1/get_proxies.cc#newcode90 get_proxies.cc:90: std::deque<std::string> ParseProxyString(const std::string &input) { This should eventually be moved into a library in src/common. Comment eventually should say it validates the proxy string returned and the format of input. http://codereview.chromium.org/6730021/diff/1/get_proxies.cc#newcode132 get_proxies.cc:132: class BrowserProxyResolvedSignalWatcher : public chromeos::dbus::SignalWatcher { I think I've run into a simpler way to do this with dbus alone (you can do a blocking call to return the next signal and handle it in your own main loop). http://codereview.chromium.org/6730021/diff/1/get_proxies.cc#newcode154 get_proxies.cc:154: PrinterrAndLog("Error getting url, proxy list from D-Bus signal"); Why isn't this just LOG(ERROR)? http://codereview.chromium.org/6730021/diff/1/get_proxies.cc#newcode245 get_proxies.cc:245: bool GetProxiesForUrlWithSettings(const char *url, Needs docs. What is json_settings besides json? http://codereview.chromium.org/6730021/diff/1/get_proxies.cc#newcode262 get_proxies.cc:262: TEST_AND_RETURN_FALSE(root->IsType(Value::TYPE_DICTIONARY)); I really don't like macros that have control flow embedded in them. Suggest expanding to a one-liner if statement at least. http://codereview.chromium.org/6730021/diff/1/get_proxies.cc#newcode297 get_proxies.cc:297: // Find which scheme we are. An undefined URL defaults to "http://". This should have been a separate function and an easy one to unit test. http://codereview.chromium.org/6730021/diff/1/get_proxies.cc#newcode382 get_proxies.cc:382: chromeos::OpenLog(my_path.BaseName().value().c_str(), true); Doing OpenLog may not be necessary (and thus the other basename my_path stuff goes away as well). http://codereview.chromium.org/6730021/diff/1/get_proxies.cc#newcode390 get_proxies.cc:390: g_print("Resolving proxies for URL: %s\n", url); do you plan to leave this in? If so why not LOG(INFO)?
I'll upload the changes soon. http://codereview.chromium.org/6730021/diff/1/Makefile File Makefile (right): http://codereview.chromium.org/6730021/diff/1/Makefile#newcode6 Makefile:6: GET_PROXIES = get_proxies On 2011/03/24 17:24:34, kmixter1 wrote: > Would add _BIN Done. http://codereview.chromium.org/6730021/diff/1/Makefile#newcode27 Makefile:27: $(shell $(PKG_CONFIG) --libs gobject-2.0 dbus-1 dbus-glib-1) On 2011/03/24 17:24:34, kmixter1 wrote: > prefer to not link gobject/dbus into crash reporter lest someone start using it. Done. I think this had been needed for update_engine's code. http://codereview.chromium.org/6730021/diff/1/Makefile#newcode29 Makefile:29: # MTEMP: GET_PROXIES_LIBS = $(COMMON_LIBS) -lprotobuf On 2011/03/24 17:24:34, kmixter1 wrote: > remove? Done. http://codereview.chromium.org/6730021/diff/1/get_proxies.cc File get_proxies.cc (right): http://codereview.chromium.org/6730021/diff/1/get_proxies.cc#newcode8 get_proxies.cc:8: #include <chromeos/dbus/dbus.h> On 2011/03/24 17:24:34, kmixter1 wrote: > existing style is to put this in ""s with the other ChromeOS-specific includes. Done. http://codereview.chromium.org/6730021/diff/1/get_proxies.cc#newcode14 get_proxies.cc:14: #include "base/json/json_reader.h" On 2011/03/24 17:24:34, kmixter1 wrote: > abc order Done. http://codereview.chromium.org/6730021/diff/1/get_proxies.cc#newcode43 get_proxies.cc:43: // For ShowSessionProxies() On 2011/03/24 17:24:34, kmixter1 wrote: > Why do we need to get the proxy setting directly from session_manager - > shouldn't Chrome correctly decide when to use that? This is in case Chrome is unavailable/not responding. http://codereview.chromium.org/6730021/diff/1/get_proxies.cc#newcode90 get_proxies.cc:90: std::deque<std::string> ParseProxyString(const std::string &input) { On 2011/03/24 17:24:34, kmixter1 wrote: > This should eventually be moved into a library in src/common. Comment > eventually should say it validates the proxy string returned and the format of > input. Should I do anything with this function for this CL? http://codereview.chromium.org/6730021/diff/1/get_proxies.cc#newcode132 get_proxies.cc:132: class BrowserProxyResolvedSignalWatcher : public chromeos::dbus::SignalWatcher { On 2011/03/24 17:24:34, kmixter1 wrote: > I think I've run into a simpler way to do this with dbus alone (you can do a > blocking call to return the next signal and handle it in your own main loop). I can't find the simpler way to do this. Could you be thinking of being able to block for the D-Bus reply (i.e. not a signal)? If I understand D-Bus correctly, I think dbus_g_proxy_call() will invoke a method on the remote interface, and then block waiting for the reply (as opposed to dbus_g_proxy_begin_call(), which does it asynchronously). The way the proxy-resolving works, though, is not to return the result in the D-Bus reply, but instead have Chrome invoke a new D-Bus signal on our interface with the response as its arguments. http://codereview.chromium.org/6730021/diff/1/get_proxies.cc#newcode154 get_proxies.cc:154: PrinterrAndLog("Error getting url, proxy list from D-Bus signal"); On 2011/03/24 17:24:34, kmixter1 wrote: > Why isn't this just LOG(ERROR)? Done. http://codereview.chromium.org/6730021/diff/1/get_proxies.cc#newcode245 get_proxies.cc:245: bool GetProxiesForUrlWithSettings(const char *url, On 2011/03/24 17:24:34, kmixter1 wrote: > Needs docs. What is json_settings besides json? Done. http://codereview.chromium.org/6730021/diff/1/get_proxies.cc#newcode262 get_proxies.cc:262: TEST_AND_RETURN_FALSE(root->IsType(Value::TYPE_DICTIONARY)); On 2011/03/24 17:24:34, kmixter1 wrote: > I really don't like macros that have control flow embedded in them. Suggest > expanding to a one-liner if statement at least. Done. http://codereview.chromium.org/6730021/diff/1/get_proxies.cc#newcode297 get_proxies.cc:297: // Find which scheme we are. An undefined URL defaults to "http://". On 2011/03/24 17:24:34, kmixter1 wrote: > This should have been a separate function and an easy one to unit test. Done. http://codereview.chromium.org/6730021/diff/1/get_proxies.cc#newcode382 get_proxies.cc:382: chromeos::OpenLog(my_path.BaseName().value().c_str(), true); On 2011/03/24 17:24:34, kmixter1 wrote: > Doing OpenLog may not be necessary (and thus the other basename my_path stuff > goes away as well). Done. http://codereview.chromium.org/6730021/diff/1/get_proxies.cc#newcode390 get_proxies.cc:390: g_print("Resolving proxies for URL: %s\n", url); On 2011/03/24 17:24:34, kmixter1 wrote: > do you plan to leave this in? If so why not LOG(INFO)? Done.
http://codereview.chromium.org/6730021/diff/7001/get_proxies.cc File get_proxies.cc (right): http://codereview.chromium.org/6730021/diff/7001/get_proxies.cc#newcode41 get_proxies.cc:41: const char kLibCrosServiceInterface[] = "org.chromium.LibCrosServiceInterface"; Sort these if there is no specific sequence? http://codereview.chromium.org/6730021/diff/7001/get_proxies.cc#newcode60 get_proxies.cc:60: static const char* GetGErrorMessage(const GError* error) { consistently use space-star not star-space here and elsewhere http://codereview.chromium.org/6730021/diff/7001/get_proxies.cc#newcode276 get_proxies.cc:276: // TODO(mkrebs): This ignores any "bypass_rules" setting. Not sure what is to be done based on this TODO. http://codereview.chromium.org/6730021/diff/7001/get_proxies.cc#newcode422 get_proxies.cc:422: if (!ShowSessionProxies(url)) { I've heard we're moving away from global proxy settings. You should check with cmasone on this. If so, I'd rather not add the complexity to this code to query session_manager on its idea of proxy when the chrome request times out.
PTAL. And, do you have any suggestions for some tests I should write? http://codereview.chromium.org/6730021/diff/7001/get_proxies.cc File get_proxies.cc (right): http://codereview.chromium.org/6730021/diff/7001/get_proxies.cc#newcode41 get_proxies.cc:41: const char kLibCrosServiceInterface[] = "org.chromium.LibCrosServiceInterface"; On 2011/04/06 04:28:53, kmixter1 wrote: > Sort these if there is no specific sequence? They're in top-down order. You contact the service name, with a path, on an interface, using a specific method. The last two are actually opposite to this, for some reason. I've reordered the last two, and added a couple comments. If you'd rather I sort them, though, that's perfectly fine by me. http://codereview.chromium.org/6730021/diff/7001/get_proxies.cc#newcode60 get_proxies.cc:60: static const char* GetGErrorMessage(const GError* error) { On 2011/04/06 04:28:53, kmixter1 wrote: > consistently use space-star not star-space here and elsewhere Done. http://codereview.chromium.org/6730021/diff/7001/get_proxies.cc#newcode276 get_proxies.cc:276: // TODO(mkrebs): This ignores any "bypass_rules" setting. On 2011/04/06 04:28:53, kmixter1 wrote: > Not sure what is to be done based on this TODO. Moot now, because I removed the Session Manager proxy stuff per your comment below. http://codereview.chromium.org/6730021/diff/7001/get_proxies.cc#newcode422 get_proxies.cc:422: if (!ShowSessionProxies(url)) { On 2011/04/06 04:28:53, kmixter1 wrote: > I've heard we're moving away from global proxy settings. You should check with > cmasone on this. If so, I'd rather not add the complexity to this code to query > session_manager on its idea of proxy when the chrome request times out. Removed.
Mostly nits. A unit test is probably in order for ParseProxyString though most of this code lends itself more for integration/autotest testing than unit testing. If nothing else, add a TEST= set of steps that explains you tested that this is returning sane values in a variety of calling patterns (no arg, URL passed, bad URL) and situations (with proxy set without proxy set). http://codereview.chromium.org/6730021/diff/7001/get_proxies.cc File get_proxies.cc (right): http://codereview.chromium.org/6730021/diff/7001/get_proxies.cc#newcode41 get_proxies.cc:41: const char kLibCrosServiceInterface[] = "org.chromium.LibCrosServiceInterface"; On 2011/04/12 00:53:38, Michael Krebs wrote: > On 2011/04/06 04:28:53, kmixter1 wrote: > > Sort these if there is no specific sequence? > > They're in top-down order. You contact the service name, with a path, on an > interface, using a specific method. The last two are actually opposite to this, > for some reason. > > I've reordered the last two, and added a couple comments. If you'd rather I > sort them, though, that's perfectly fine by me. I'd go for abc order, to simplify adding new constants in the future - otherwise new sections need to be created for all string constants. http://codereview.chromium.org/6730021/diff/7001/get_proxies.cc#newcode67 get_proxies.cc:67: // Copied from src/update_engine/chrome_browser_proxy_resolver.cc 1 line between functions http://codereview.chromium.org/6730021/diff/7001/get_proxies.cc#newcode110 get_proxies.cc:110: class BrowserProxyResolvedSignalWatcher : public chromeos::dbus::SignalWatcher { Add comment describing purpose of class. http://codereview.chromium.org/6730021/diff/10001/get_proxies.cc#newcode1 get_proxies.cc:1: // Copyright (c) 2011 The Chromium OS Authors. All rights reserved. nit: I would probably choose print, list, or dump over "get" in the name of this command. It sounds more like a function name than a unix command, but this is admittedly a nit. http://codereview.chromium.org/6730021/diff/10001/get_proxies.cc#newcode43 get_proxies.cc:43: "ResolveNetworkProxy"; nit: I would avoid using extra vertical white space. While the style guide is not very exact about this, look around src/platform and I think you'll see 1 space is all that's used to separate sections of declarations and to separate functions. http://codereview.chromium.org/6730021/diff/10001/get_proxies.cc#newcode45 get_proxies.cc:45: const char kLibCrosProxyResolveSignalInterface[] = Should this be a DEFINE_int32 flag? http://codereview.chromium.org/6730021/diff/10001/get_proxies.cc#newcode100 get_proxies.cc:100: TrimWhitespaceASCII(host_and_port, TRIM_ALL, &host_and_port); Add a basic class docstring. http://codereview.chromium.org/6730021/diff/10001/get_proxies.cc#newcode218 get_proxies.cc:218: kProxyModePACScript = 2, Would generally say to const char *url = NULL; if (argc >= 2) { print resolving proxies for URL $argv[1] url = argv[1] } else print resolving proxies without URL avoids a pair of ?:'s and seems more readable. http://codereview.chromium.org/6730021/diff/10001/get_proxies.cc#newcode220 get_proxies.cc:220: kProxyModeProxyPerScheme = 4 add explicit comparison to NULL for local style. http://codereview.chromium.org/6730021/diff/10001/get_proxies.cc#newcode225 get_proxies.cc:225: const char *GetUrlScheme(const char *url) { Why g_print vs printf?
Also, include a TEST= test for how you test that the timeout works.
Added to "TEST=". PTAL. http://codereview.chromium.org/6730021/diff/7001/get_proxies.cc File get_proxies.cc (right): http://codereview.chromium.org/6730021/diff/7001/get_proxies.cc#newcode41 get_proxies.cc:41: const char kLibCrosServiceInterface[] = "org.chromium.LibCrosServiceInterface"; On 2011/04/12 01:17:41, kmixter1 wrote: > On 2011/04/12 00:53:38, Michael Krebs wrote: > > On 2011/04/06 04:28:53, kmixter1 wrote: > > > Sort these if there is no specific sequence? > > > > They're in top-down order. You contact the service name, with a path, on an > > interface, using a specific method. The last two are actually opposite to > this, > > for some reason. > > > > I've reordered the last two, and added a couple comments. If you'd rather I > > sort them, though, that's perfectly fine by me. > > I'd go for abc order, to simplify adding new constants in the future - otherwise > new sections need to be created for all string constants. Done. http://codereview.chromium.org/6730021/diff/7001/get_proxies.cc#newcode67 get_proxies.cc:67: // Copied from src/update_engine/chrome_browser_proxy_resolver.cc On 2011/04/12 01:17:41, kmixter1 wrote: > 1 line between functions Done. http://codereview.chromium.org/6730021/diff/7001/get_proxies.cc#newcode110 get_proxies.cc:110: class BrowserProxyResolvedSignalWatcher : public chromeos::dbus::SignalWatcher { On 2011/04/12 01:17:41, kmixter1 wrote: > Add comment describing purpose of class. Done.
PTAL. The only open question for me/you is about the class's comment. http://codereview.chromium.org/6730021/diff/10001/get_proxies.cc File get_proxies.cc (right): http://codereview.chromium.org/6730021/diff/10001/get_proxies.cc#newcode1 get_proxies.cc:1: // Copyright (c) 2011 The Chromium OS Authors. All rights reserved. On 2011/04/12 01:17:41, kmixter1 wrote: > nit: I would probably choose print, list, or dump over "get" in the name of this > command. It sounds more like a function name than a unix command, but this is > admittedly a nit. Done. http://codereview.chromium.org/6730021/diff/10001/get_proxies.cc#newcode43 get_proxies.cc:43: On 2011/04/12 01:17:41, kmixter1 wrote: > nit: I would avoid using extra vertical white space. While the style guide is > not very exact about this, look around src/platform and I think you'll see 1 > space is all that's used to separate sections of declarations and to separate > functions. Done. http://codereview.chromium.org/6730021/diff/10001/get_proxies.cc#newcode45 get_proxies.cc:45: const int kBrowserTimeout = 5; On 2011/04/12 01:17:41, kmixter1 wrote: > Should this be a DEFINE_int32 flag? Done. http://codereview.chromium.org/6730021/diff/10001/get_proxies.cc#newcode100 get_proxies.cc:100: class BrowserProxyResolvedSignalWatcher : public chromeos::dbus::SignalWatcher { On 2011/04/12 01:17:41, kmixter1 wrote: > Add a basic class docstring. Is the comment that's there now okay? I couldn't find an example of a class docstring elsewhere to mimic. http://codereview.chromium.org/6730021/diff/10001/get_proxies.cc#newcode218 get_proxies.cc:218: const char *url = (argc >= 2) ? argv[1] : NULL; On 2011/04/12 01:17:41, kmixter1 wrote: > Would generally say to > const char *url = NULL; > if (argc >= 2) { > print resolving proxies for URL $argv[1] > url = argv[1] > } else > print resolving proxies without URL > > avoids a pair of ?:'s and seems more readable. Done. http://codereview.chromium.org/6730021/diff/10001/get_proxies.cc#newcode220 get_proxies.cc:220: LOG(INFO) << "Resolving proxies for URL: " << (url ? url : "<n/a>"); On 2011/04/12 01:17:41, kmixter1 wrote: > add explicit comparison to NULL for local style. Moot after above change. http://codereview.chromium.org/6730021/diff/10001/get_proxies.cc#newcode225 get_proxies.cc:225: g_print("%s\n", kNoProxy); On 2011/04/12 01:17:41, kmixter1 wrote: > Why g_print vs printf? Done.
LGTM However - have you tried an end to end test that a crash dump is properly sent both through a proxy and direct connection? http://codereview.chromium.org/6730021/diff/15001/list_proxies.cc File list_proxies.cc (right): http://codereview.chromium.org/6730021/diff/15001/list_proxies.cc#newcode24 list_proxies.cc:24: DEFINE_int32(timeout, 5, "Set timeout for browser resolving proxies"); nit: abc order for params?
An end-to-end test of crash_sender does work, both with and without a proxy. http://codereview.chromium.org/6730021/diff/15001/list_proxies.cc File list_proxies.cc (right): http://codereview.chromium.org/6730021/diff/15001/list_proxies.cc#newcode24 list_proxies.cc:24: DEFINE_int32(timeout, 5, "Set timeout for browser resolving proxies"); On 2011/04/14 18:26:11, kmixter1 wrote: > nit: abc order for params? Done.
ship it... On Fri, Apr 15, 2011 at 4:42 PM, <mkrebs@chromium.org> wrote: > An end-to-end test of crash_sender does work, both with and without a proxy. > > > > http://codereview.chromium.org/6730021/diff/15001/list_proxies.cc > File list_proxies.cc (right): > > http://codereview.chromium.org/6730021/diff/15001/list_proxies.cc#newcode24 > list_proxies.cc:24: DEFINE_int32(timeout, 5, "Set timeout for browser > resolving proxies"); > On 2011/04/14 18:26:11, kmixter1 wrote: >> >> nit: abc order for params? > > Done. > > http://codereview.chromium.org/6730021/ > |
