Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22295002:
Base infrastructure for Networking Private API on Windows and Mac. (Closed)
|
Created:
7 years, 4 months ago by mef Modified:
7 years, 1 month ago Reviewers:
cbentzel, Finnur, stevenjb, Elliot Glaysher, cpu_(ooo_6.6-7.5), jam, tbarzic, Jorge Lucangeli Obes, palmer, James Hawkins, Chris Evans CC:
chromium-reviews, chromium-apps-reviews_chromium.org, darin-cc_chromium.org, extensions-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionBase infrastructure for Networking Private API on Windows and Mac.
Uses Utility Process to isolate calls to OS-specific WiFi API.
Defines Networking Private IPC Messages and implements Networking Private API methods via those messages.
Windows implementation is in http://crrev.com/27722003.
Mac OS X implementation is in http://crrev.com/26481006.
BUG=267667
Patch Set 1 #Patch Set 2 : Added mock SetProperties implementation. #Patch Set 3 : Added StartConnect. #Patch Set 4 : Added StartDisconnect. #Patch Set 5 : Added mock implementation of GetVisibleNetworks. #Patch Set 6 : Added RequestNetworkScan. #Patch Set 7 : Sync to r216398 #Patch Set 8 : Add GetVisibleNetworks, BSSID and correct Frequency on Windows. #Patch Set 9 : Fixed compilation error. #Patch Set 10 : Fixed unit_tests compilation error. #Patch Set 11 : Implemented GetVisibleNetworks on Windows. #Patch Set 12 : Fixed compilation error. #
Total comments: 57
Patch Set 13 : Sync up to r218508 #Patch Set 14 : Use WiFiServiceMock for browser_tests, and WiFiService otherwise. #Patch Set 15 : Plug NetworkingPrivateCrypto into NetworkingPrivateApi. #Patch Set 16 : NetworkingPrivateHandler now waits up to 10s for network to connect prior to sending networks chang… #Patch Set 17 : First successful setup run on Windows. #Patch Set 18 : Added network event observers to WiFiService interface. #Patch Set 19 : Hook notifications to WiFiServiceImpl. #Patch Set 20 : Shutdown Networking Private Utility Process when all networking events listeners are removed. #Patch Set 21 : Reset DHCP after WiFi Connect to speed up the connection after factory reset. #
Total comments: 10
Patch Set 22 : Add message_id to each NetworkingPrivateMsg* that needs reply, and use IDMap to track and run callb… #Patch Set 23 : Handle WLAN Notifications and send events from main thread. #Patch Set 24 : Use connected WiFi interface, test disconnect and reconnect in WiFiServiceTest. #Patch Set 25 : Sync up to r223127 #Patch Set 26 : Fix android link error and move NetworkingPrivateEventRouter to extensions namespace. #Patch Set 27 : Better comments and formatting. #Patch Set 28 : Sync up to r225168 #
Total comments: 32
Patch Set 29 : Addressed (some) codereview comments. #
Total comments: 20
Patch Set 30 : Sync up to r227517 #Patch Set 31 : Use onc_constants.h instead of custom WiFiService constants. #Patch Set 32 : Address codereview comments. #Patch Set 33 : Fixed compilation errors on Windows. #
Total comments: 32
Patch Set 34 : Fix browser_tests, address some codereview comments. #
Total comments: 41
Patch Set 35 : Sync up to r228517 #Patch Set 36 : Address some codereview comments. #
Total comments: 2
Patch Set 37 : Added CryptoVerify, CryptoVerifyMock and CryptoVerifyImpl to NetworkingPrivateProcessClient. #Patch Set 38 : Address codereview comments. #
Total comments: 2
Patch Set 39 : Use using chromeos:: to avoid lengthy namespace prefixes. #
Total comments: 32
Patch Set 40 : Sync to r228917 #Patch Set 41 : Fix NetworkingPrivateProcessClient lifetime tracking. #
Total comments: 8
Patch Set 42 : Address codereview comments. #Patch Set 43 : Split off Windows implementation, change gyps to compile on windows. #
Total comments: 19
Patch Set 44 : Exclude networking_private from compilation on platforms other than Windows and ChromeOS. #Patch Set 45 : Address some code review comments. #Patch Set 46 : Perform Crypto Verify* on WorkerPool, Make EventRouter a Network* events observer and sender. #
Total comments: 16
Patch Set 47 : Address codereview comments. #Patch Set 48 : Fix apitest compilation on chromeos and linux. #Patch Set 49 : Use scoped_ptr to pass Verify* args to WorkerPool. #
Total comments: 36
Patch Set 50 : Merge in https://codereview.chromium.org/34013002 #Patch Set 51 : Sync up to r230127 #
Total comments: 8
Patch Set 52 : pending message callback not #
Total comments: 6
Patch Set 53 : Fix Incognito mode - Attach NetworkPrivateProcessClient to original profile. #
Total comments: 24
Patch Set 54 : Address codereview comments. #
Total comments: 5
Patch Set 55 : Sync to r231308 #Patch Set 56 : Use crypto_verify_mock for browser_test. #Messages
Total messages: 95 (0 generated)
Hi guys, I just wanted to update you on a progress I've made so far with implementation of Networking Private API for Windows. - System wifi interface is executed from utility process running outside of sandbox. - Networking_private_messages.h defines set of messages between browser and utility process. - networking_private_api_nonchromeos uses networking_private_process_client to send those messages to utility process. - Networking_private_handler handles those messages and uses wifi_service interface to call system api. - Wifi_service_mock implementation mocks system api calls to get networkingPrivateAPI browser tests to pass while exercising the whole pipeline. - Wifi_service_win implements the same interface using actual win32 wlan api. - Wifi_test is a small test utility to interactively call wifi_service interface. - Wifi_service_test is (stub) unit test for system interface. This is work in progress, and I will continue it once I return from vacations. Please take a look at your convenience and let me know if you have any questions/comments/suggestions. thanks, -m
Just a flyby review while gspencer@ is catching up from vacation. https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_api.h (right): https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_api.h:116: void ResultCallback(const base::ListValue& network_list); Unused? https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:28: namespace { nit: WS https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:42: } nit: WS https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:72: base::DictionaryValue* network_properties = dictionary.DeepCopy(); nit: no need for local variable (and makes ownership slightly less clear) https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:256: network->GetString("Type", &network_type) && This should be defined as a constant somewhere, either using an ONC constant or something defined in NetworkingPrivateProcessClient. https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:257: network_type == params_type)) { Do we want to ensure that |v| is a dictionary? It might also make the code more clear if we do: if (!v->GetAsDictionary(&network)) { LOG(ERROR)... continue; } if (!request_all) { std::string network_type; network->GetString(kType, &network_type); if (network_type != params_type) continue; } https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:28: NetworkingPrivateProcessClient::~NetworkingPrivateProcessClient() {} nit: } on new line https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:50: // source_profile_, items_, localized_strings)); ?? https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:58: // process_importer_host_->Cancel(); ?? https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:97: // MessageBoxA(NULL, __FUNCTION__, "Debug Me", MB_OK); ?? https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:293: /* ?? https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_process_client.h (right): https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_process_client.h:130: // ExternalProcessImporterHost* process_importer_host_; ?? https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... File chrome/browser/extensions/extension_apitest.cc (right): https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/extension_apitest.cc:279: //#endif Eliminate before committing. https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... File chrome/utility/wifi/wifi_service.cc (right): https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... chrome/utility/wifi/wifi_service.cc:39: const char kWiFiSignalStrength[] = "WiFi.SignalStrength"; Probably worth defining these in a common place. Is there a translation layer between these and networkingPrivate? If not, we should use the ONC constants directly. https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... chrome/utility/wifi/wifi_service.cc:83: } One advantage to sticking with strings is not having to maintain a bunch of functions like these. I can almost guarantee you will also want the inverse of these soon if you use enums... https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... chrome/utility/wifi/wifi_service.cc:94: WiFiService::NetworkProperties::~NetworkProperties() {} nit: } on separate line here and above https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... File chrome/utility/wifi/wifi_service.h (right): https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... chrome/utility/wifi/wifi_service.h:49: }; FWIW, the original ChromeOS networking code used a bunch of enums like these, which turned out to be a huge PITA to translate and maintain across processes and subsystems. The new code just uses strings which, as near as I can tell has been a win all around, especially for debugging purposes. https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... chrome/utility/wifi/wifi_service.h:68: ~NetworkProperties(); Move to top https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... chrome/utility/wifi/wifi_service.h:70: bool UpdateFromValue(const base::DictionaryValue& value); WS https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... chrome/utility/wifi/wifi_service.h:131: static WiFiService* CreateServiceMock(); private: DISALLOW_COPY...
Actually, I did see this when I was digging out, but got distracted, so thanks for the ping Steven. As a first step why don't you get the commented out code out of this CL. (and I'll keep looking at it while you do that). https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:252: const base::Value* v = *it; Please use a more descriptive variable name than 'v'. https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:55: // if (cancelled_) Yes, remove all commented out code (or uncomment).
I've reviewed very little (and nothing in the utility process). Overall design seems to make sense, but I haven't dug deeply into details to see if there are other issues. Given the amount of LOG(INFO) and other commented code this definitely isn't in a "ship it" state - but you clearly marked that. https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:36: kNetworkingPrivateProcessClient, I'm a little confused about how the scope of the NetworkingPrivateProcessClient is tied to that of the Profile. In particular, it's a refcounted object due to UtilityProcessClient, but if I understand this code correctly the profile holds a raw pointer to it through SetUserData. It should probably use a refcounted pointer, or at least do manual calls to AddRef here and Release when the Profile tears down. https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:103: base::Bind(&NetworkingPrivateProcessClient::OnGetPropertiesStart, What happens if the utility process is killed while this is outstanding? Worried that the extension API call will stall forever rather than error back to the user in that case.
Hi guys, thanks a lot for your comments! As I've mentioned this is work in progress and I just wanted to get some validation of chosen design and see if there are any glaring omissions. I'll make sure to revisit the UtilityProcessClient lifespan and WiFiService enum constants. I'll address all the nits and do the general code cleanup prior to final review. I'll let you know when I'll make enough changes to be worth another look. thanks again, -m https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_api.h (right): https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_api.h:116: void ResultCallback(const base::ListValue& network_list); > Unused? It is used in nonchromeos implementation. https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:36: kNetworkingPrivateProcessClient, On 2013/08/19 19:46:34, cbentzel wrote: > I'm a little confused about how the scope of the NetworkingPrivateProcessClient > is tied to that of the Profile. > > In particular, it's a refcounted object due to UtilityProcessClient, but if I > understand this code correctly the profile holds a raw pointer to it through > SetUserData. It should probably use a refcounted pointer, or at least do manual > calls to AddRef here and Release when the Profile tears down. Good point. I have not think this through yet. I need NetworkingPrivateProcessClient to survive between function calls (to preserve state and broadcast events), but I haven't found a good way to determine when extension comes and goes to control its lifespan. Ben suggested to base it on registration of event subscribers, and I'm planning to do that unless there is some better way. I will make sure to provide proper refcounting at that point. https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:103: base::Bind(&NetworkingPrivateProcessClient::OnGetPropertiesStart, On 2013/08/19 19:46:34, cbentzel wrote: > What happens if the utility process is killed while this is outstanding? Worried > that the extension API call will stall forever rather than error back to the > user in that case. Good point. I guess NetworkingPrivateProcessClient::OnProcessCrashed should invoke error_callback, which also points out that there is no need to have different error_callbacks for different api calls. https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_process_client.h (right): https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_process_client.h:130: // ExternalProcessImporterHost* process_importer_host_; On 2013/08/13 23:39:04, stevenjb (chromium) wrote: > ?? Leftovers from the class that I've used for inspiration. :-) https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... File chrome/browser/extensions/extension_apitest.cc (right): https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/extension_apitest.cc:279: //#endif On 2013/08/13 23:39:04, stevenjb (chromium) wrote: > Eliminate before committing. Will do, I had to comment it out to get my tests to actually get called. https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... File chrome/utility/wifi/wifi_service.cc (right): https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... chrome/utility/wifi/wifi_service.cc:39: const char kWiFiSignalStrength[] = "WiFi.SignalStrength"; On 2013/08/13 23:39:04, stevenjb (chromium) wrote: > Probably worth defining these in a common place. > > Is there a translation layer between these and networkingPrivate? If not, we > should use the ONC constants directly. I'd love to use ONC constants, but they seem to be ChromeOS-specific. Should I move them somewhere? https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... chrome/utility/wifi/wifi_service.cc:83: } On 2013/08/13 23:39:04, stevenjb (chromium) wrote: > One advantage to sticking with strings is not having to maintain a bunch of > functions like these. I can almost guarantee you will also want the inverse of > these soon if you use enums... Good point, will follow your advise. https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... chrome/utility/wifi/wifi_service.cc:94: WiFiService::NetworkProperties::~NetworkProperties() {} On 2013/08/13 23:39:04, stevenjb (chromium) wrote: > nit: } on separate line here and above This is result of automatic clang-format, not sure if it fits the guidelines. https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... File chrome/utility/wifi/wifi_service.h (right): https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... chrome/utility/wifi/wifi_service.h:49: }; On 2013/08/13 23:39:04, stevenjb (chromium) wrote: > FWIW, the original ChromeOS networking code used a bunch of enums like these, > which turned out to be a huge PITA to translate and maintain across processes > and subsystems. The new code just uses strings which, as near as I can tell has > been a win all around, especially for debugging purposes. Thanks a lot for valuable insight! My initial intention was to decouple WiFiService from networking_private-specific constants, but I don't think there is enough benefit there. I'll change it to use strings and string constants. https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... chrome/utility/wifi/wifi_service.h:68: ~NetworkProperties(); On 2013/08/13 23:39:04, stevenjb (chromium) wrote: > Move to top Will do. https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... chrome/utility/wifi/wifi_service.h:131: static WiFiService* CreateServiceMock(); On 2013/08/13 23:39:04, stevenjb (chromium) wrote: > private: > DISALLOW_COPY... Will do.
On 2013/08/13 23:39:04, stevenjb (chromium) wrote: > Just a flyby review while gspencer@ is catching up from vacation. > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > File chrome/browser/extensions/api/networking_private/networking_private_api.h > (right): > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > chrome/browser/extensions/api/networking_private/networking_private_api.h:116: > void ResultCallback(const base::ListValue& network_list); > Unused? > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > File > chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc > (right): > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:28: > namespace { > nit: WS > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:42: > } > nit: WS > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:72: > base::DictionaryValue* network_properties = dictionary.DeepCopy(); > nit: no need for local variable (and makes ownership slightly less clear) > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:256: > network->GetString("Type", &network_type) && > This should be defined as a constant somewhere, either using an ONC constant or > something defined in NetworkingPrivateProcessClient. > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:257: > network_type == params_type)) { > Do we want to ensure that |v| is a dictionary? It might also make the code more > clear if we do: > > if (!v->GetAsDictionary(&network)) { > LOG(ERROR)... > continue; > } > if (!request_all) { > std::string network_type; > network->GetString(kType, &network_type); > if (network_type != params_type) > continue; > } > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > File > chrome/browser/extensions/api/networking_private/networking_private_process_client.cc > (right): > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:28: > NetworkingPrivateProcessClient::~NetworkingPrivateProcessClient() {} > nit: } on new line > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:50: > // source_profile_, items_, localized_strings)); > ?? > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:58: > // process_importer_host_->Cancel(); > ?? > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:97: > // MessageBoxA(NULL, __FUNCTION__, "Debug Me", MB_OK); > ?? > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:293: > /* > ?? > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > File > chrome/browser/extensions/api/networking_private/networking_private_process_client.h > (right): > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > chrome/browser/extensions/api/networking_private/networking_private_process_client.h:130: > // ExternalProcessImporterHost* process_importer_host_; > ?? > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > File chrome/browser/extensions/extension_apitest.cc (right): > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > chrome/browser/extensions/extension_apitest.cc:279: //#endif > Eliminate before committing. > > https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... > File chrome/utility/wifi/wifi_service.cc (right): > > https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... > chrome/utility/wifi/wifi_service.cc:39: const char kWiFiSignalStrength[] = > "WiFi.SignalStrength"; > Probably worth defining these in a common place. > > Is there a translation layer between these and networkingPrivate? If not, we > should use the ONC constants directly. > > https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... > chrome/utility/wifi/wifi_service.cc:83: } > One advantage to sticking with strings is not having to maintain a bunch of > functions like these. I can almost guarantee you will also want the inverse of > these soon if you use enums... > > https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... > chrome/utility/wifi/wifi_service.cc:94: > WiFiService::NetworkProperties::~NetworkProperties() {} > nit: } on separate line here and above > > https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... > File chrome/utility/wifi/wifi_service.h (right): > > https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... > chrome/utility/wifi/wifi_service.h:49: }; > FWIW, the original ChromeOS networking code used a bunch of enums like these, > which turned out to be a huge PITA to translate and maintain across processes > and subsystems. The new code just uses strings which, as near as I can tell has > been a win all around, especially for debugging purposes. > > https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... > chrome/utility/wifi/wifi_service.h:68: ~NetworkProperties(); > Move to top > > https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... > chrome/utility/wifi/wifi_service.h:70: bool UpdateFromValue(const > base::DictionaryValue& value); > WS > > https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... > chrome/utility/wifi/wifi_service.h:131: static WiFiService* CreateServiceMock(); > private: > DISALLOW_COPY... Hi Steven, is there a good way to move onc_constants.h from chromeos/network/onc/onc_constants.h, so they could be used by chrome extension api? thanks, -m
On 2013/09/03 20:19:05, mef wrote: > On 2013/08/13 23:39:04, stevenjb (chromium) wrote: > > Just a flyby review while gspencer@ is catching up from vacation. > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > > File chrome/browser/extensions/api/networking_private/networking_private_api.h > > (right): > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > > chrome/browser/extensions/api/networking_private/networking_private_api.h:116: > > void ResultCallback(const base::ListValue& network_list); > > Unused? > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > > File > > > chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc > > (right): > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > > > chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:28: > > namespace { > > nit: WS > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > > > chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:42: > > } > > nit: WS > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > > > chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:72: > > base::DictionaryValue* network_properties = dictionary.DeepCopy(); > > nit: no need for local variable (and makes ownership slightly less clear) > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > > > chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:256: > > network->GetString("Type", &network_type) && > > This should be defined as a constant somewhere, either using an ONC constant > or > > something defined in NetworkingPrivateProcessClient. > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > > > chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:257: > > network_type == params_type)) { > > Do we want to ensure that |v| is a dictionary? It might also make the code > more > > clear if we do: > > > > if (!v->GetAsDictionary(&network)) { > > LOG(ERROR)... > > continue; > > } > > if (!request_all) { > > std::string network_type; > > network->GetString(kType, &network_type); > > if (network_type != params_type) > > continue; > > } > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > > File > > > chrome/browser/extensions/api/networking_private/networking_private_process_client.cc > > (right): > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > > > chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:28: > > NetworkingPrivateProcessClient::~NetworkingPrivateProcessClient() {} > > nit: } on new line > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > > > chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:50: > > // source_profile_, items_, localized_strings)); > > ?? > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > > > chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:58: > > // process_importer_host_->Cancel(); > > ?? > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > > > chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:97: > > // MessageBoxA(NULL, __FUNCTION__, "Debug Me", MB_OK); > > ?? > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > > > chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:293: > > /* > > ?? > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > > File > > > chrome/browser/extensions/api/networking_private/networking_private_process_client.h > > (right): > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > > > chrome/browser/extensions/api/networking_private/networking_private_process_client.h:130: > > // ExternalProcessImporterHost* process_importer_host_; > > ?? > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > > File chrome/browser/extensions/extension_apitest.cc (right): > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > > chrome/browser/extensions/extension_apitest.cc:279: //#endif > > Eliminate before committing. > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... > > File chrome/utility/wifi/wifi_service.cc (right): > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... > > chrome/utility/wifi/wifi_service.cc:39: const char kWiFiSignalStrength[] = > > "WiFi.SignalStrength"; > > Probably worth defining these in a common place. > > > > Is there a translation layer between these and networkingPrivate? If not, we > > should use the ONC constants directly. > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... > > chrome/utility/wifi/wifi_service.cc:83: } > > One advantage to sticking with strings is not having to maintain a bunch of > > functions like these. I can almost guarantee you will also want the inverse of > > these soon if you use enums... > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... > > chrome/utility/wifi/wifi_service.cc:94: > > WiFiService::NetworkProperties::~NetworkProperties() {} > > nit: } on separate line here and above > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... > > File chrome/utility/wifi/wifi_service.h (right): > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... > > chrome/utility/wifi/wifi_service.h:49: }; > > FWIW, the original ChromeOS networking code used a bunch of enums like these, > > which turned out to be a huge PITA to translate and maintain across processes > > and subsystems. The new code just uses strings which, as near as I can tell > has > > been a win all around, especially for debugging purposes. > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... > > chrome/utility/wifi/wifi_service.h:68: ~NetworkProperties(); > > Move to top > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... > > chrome/utility/wifi/wifi_service.h:70: bool UpdateFromValue(const > > base::DictionaryValue& value); > > WS > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... > > chrome/utility/wifi/wifi_service.h:131: static WiFiService* > CreateServiceMock(); > > private: > > DISALLOW_COPY... > > Hi Steven, > > is there a good way to move onc_constants.h from > chromeos/network/onc/onc_constants.h, so they could be used by chrome extension > api? > > thanks, > -m What code in particular needs to include onc_constants.h? src/chromeos shouldn't depend on any extensions code, so it should be OK for extensions code to depend on src/chromeos if it needs it. We really don't want to move onc_constants.h.
On 2013/09/03 21:47:33, stevenjb (chromium) wrote: > On 2013/09/03 20:19:05, mef wrote: > > On 2013/08/13 23:39:04, stevenjb (chromium) wrote: > > > Just a flyby review while gspencer@ is catching up from vacation. > > > > > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > > > File > chrome/browser/extensions/api/networking_private/networking_private_api.h > > > (right): > > > > > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > > > > chrome/browser/extensions/api/networking_private/networking_private_api.h:116: > > > void ResultCallback(const base::ListValue& network_list); > > > Unused? > > > > > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > > > File > > > > > > chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > > > > > > chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:28: > > > namespace { > > > nit: WS > > > > > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > > > > > > chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:42: > > > } > > > nit: WS > > > > > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > > > > > > chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:72: > > > base::DictionaryValue* network_properties = dictionary.DeepCopy(); > > > nit: no need for local variable (and makes ownership slightly less clear) > > > > > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > > > > > > chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:256: > > > network->GetString("Type", &network_type) && > > > This should be defined as a constant somewhere, either using an ONC constant > > or > > > something defined in NetworkingPrivateProcessClient. > > > > > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > > > > > > chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:257: > > > network_type == params_type)) { > > > Do we want to ensure that |v| is a dictionary? It might also make the code > > more > > > clear if we do: > > > > > > if (!v->GetAsDictionary(&network)) { > > > LOG(ERROR)... > > > continue; > > > } > > > if (!request_all) { > > > std::string network_type; > > > network->GetString(kType, &network_type); > > > if (network_type != params_type) > > > continue; > > > } > > > > > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > > > File > > > > > > chrome/browser/extensions/api/networking_private/networking_private_process_client.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > > > > > > chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:28: > > > NetworkingPrivateProcessClient::~NetworkingPrivateProcessClient() {} > > > nit: } on new line > > > > > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > > > > > > chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:50: > > > // source_profile_, items_, localized_strings)); > > > ?? > > > > > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > > > > > > chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:58: > > > // process_importer_host_->Cancel(); > > > ?? > > > > > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > > > > > > chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:97: > > > // MessageBoxA(NULL, __FUNCTION__, "Debug Me", MB_OK); > > > ?? > > > > > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > > > > > > chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:293: > > > /* > > > ?? > > > > > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > > > File > > > > > > chrome/browser/extensions/api/networking_private/networking_private_process_client.h > > > (right): > > > > > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > > > > > > chrome/browser/extensions/api/networking_private/networking_private_process_client.h:130: > > > // ExternalProcessImporterHost* process_importer_host_; > > > ?? > > > > > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > > > File chrome/browser/extensions/extension_apitest.cc (right): > > > > > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... > > > chrome/browser/extensions/extension_apitest.cc:279: //#endif > > > Eliminate before committing. > > > > > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... > > > File chrome/utility/wifi/wifi_service.cc (right): > > > > > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... > > > chrome/utility/wifi/wifi_service.cc:39: const char kWiFiSignalStrength[] = > > > "WiFi.SignalStrength"; > > > Probably worth defining these in a common place. > > > > > > Is there a translation layer between these and networkingPrivate? If not, we > > > should use the ONC constants directly. > > > > > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... > > > chrome/utility/wifi/wifi_service.cc:83: } > > > One advantage to sticking with strings is not having to maintain a bunch of > > > functions like these. I can almost guarantee you will also want the inverse > of > > > these soon if you use enums... > > > > > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... > > > chrome/utility/wifi/wifi_service.cc:94: > > > WiFiService::NetworkProperties::~NetworkProperties() {} > > > nit: } on separate line here and above > > > > > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... > > > File chrome/utility/wifi/wifi_service.h (right): > > > > > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... > > > chrome/utility/wifi/wifi_service.h:49: }; > > > FWIW, the original ChromeOS networking code used a bunch of enums like > these, > > > which turned out to be a huge PITA to translate and maintain across > processes > > > and subsystems. The new code just uses strings which, as near as I can tell > > has > > > been a win all around, especially for debugging purposes. > > > > > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... > > > chrome/utility/wifi/wifi_service.h:68: ~NetworkProperties(); > > > Move to top > > > > > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... > > > chrome/utility/wifi/wifi_service.h:70: bool UpdateFromValue(const > > > base::DictionaryValue& value); > > > WS > > > > > > > > > https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... > > > chrome/utility/wifi/wifi_service.h:131: static WiFiService* > > CreateServiceMock(); > > > private: > > > DISALLOW_COPY... > > > > Hi Steven, > > > > is there a good way to move onc_constants.h from > > chromeos/network/onc/onc_constants.h, so they could be used by chrome > extension > > api? > > > > thanks, > > -m > > What code in particular needs to include onc_constants.h? src/chromeos shouldn't > depend on any extensions code, so it should be OK for extensions code to depend > on src/chromeos if it needs it. We really don't want to move onc_constants.h. Oh, right, this is Windows code... could you start an email thread on this and include gspencer@ and pneubeck@? We should decide whether we want to move onc_constants.h (maybe to src/net/onc?) so that we can use this outside of ChromeOS.
Just a couple of drive-by comments I caught while trying to understand the API and security implications. https://chromiumcodereview.appspot.com/22295002/diff/157001/chrome/browser/ex... File chrome/browser/extensions/api/networking_private/networking_private_process_client.h (right): https://chromiumcodereview.appspot.com/22295002/diff/157001/chrome/browser/ex... chrome/browser/extensions/api/networking_private/networking_private_process_client.h:23: // This class is the client for the out of process networking private handler. Nit: "out-of-process", "networkingPrivate" https://chromiumcodereview.appspot.com/22295002/diff/157001/chrome/chrome_dll... File chrome/chrome_dll.gypi (right): https://chromiumcodereview.appspot.com/22295002/diff/157001/chrome/chrome_dll... chrome/chrome_dll.gypi:196: 'Wlanapi.dll', Why capital 'W'? https://chromiumcodereview.appspot.com/22295002/diff/157001/chrome/common/net... File chrome/common/networking_private_messages.h (right): https://chromiumcodereview.appspot.com/22295002/diff/157001/chrome/common/net... chrome/common/networking_private_messages.h:55: // |properties| are properties convertable to NetworkProperties API object. Nit: "convertible" https://chromiumcodereview.appspot.com/22295002/diff/157001/chrome/utility/ne... File chrome/utility/networking_private_handler.h (right): https://chromiumcodereview.appspot.com/22295002/diff/157001/chrome/utility/ne... chrome/utility/networking_private_handler.h:24: // Dispatches IPCs for out of process profile import. Outdated comment. https://chromiumcodereview.appspot.com/22295002/diff/157001/chrome/utility/wi... File chrome/utility/wifi/wifi_service.h (right): https://chromiumcodereview.appspot.com/22295002/diff/157001/chrome/utility/wi... chrome/utility/wifi/wifi_service.h:5: // This file defiles the WiFiService interface used by chrome.networkingPrivate defines.
Hi Jorge, thanks for your comments! I've updated networking_private_messages, networking_private_handler and networking_private_process_client to add message_id to each NetworkingPrivateMsg* that needs reply, and use IDMap to track and run callbacks. Please take a look and let me know if this makes sense and whether I need to add something from the security perspective. Please note that while I've made some progress in my implementation this CL is not yet quite ready for formal review, but I'll appreciate any input that I can get. :-) thanks, -m https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:72: base::DictionaryValue* network_properties = dictionary.DeepCopy(); On 2013/08/13 23:39:04, stevenjb (chromium) wrote: > nit: no need for local variable (and makes ownership slightly less clear) Done. https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:256: network->GetString("Type", &network_type) && On 2013/08/13 23:39:04, stevenjb (chromium) wrote: > This should be defined as a constant somewhere, either using an ONC constant or > something defined in NetworkingPrivateProcessClient. Done. https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:257: network_type == params_type)) { On 2013/08/13 23:39:04, stevenjb (chromium) wrote: > Do we want to ensure that |v| is a dictionary? It might also make the code more > clear if we do: > > if (!v->GetAsDictionary(&network)) { > LOG(ERROR)... > continue; > } > if (!request_all) { > std::string network_type; > network->GetString(kType, &network_type); > if (network_type != params_type) > continue; > } > Done. https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... File chrome/browser/extensions/extension_apitest.cc (right): https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/extension_apitest.cc:279: //#endif On 2013/08/13 23:39:04, stevenjb (chromium) wrote: > Eliminate before committing. Done. https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... File chrome/utility/wifi/wifi_service.h (right): https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... chrome/utility/wifi/wifi_service.h:68: ~NetworkProperties(); On 2013/08/21 17:25:34, mef wrote: > On 2013/08/13 23:39:04, stevenjb (chromium) wrote: > > Move to top > Will do. Done. https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... chrome/utility/wifi/wifi_service.h:70: bool UpdateFromValue(const base::DictionaryValue& value); On 2013/08/13 23:39:04, stevenjb (chromium) wrote: > WS Done. https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... chrome/utility/wifi/wifi_service.h:131: static WiFiService* CreateServiceMock(); On 2013/08/13 23:39:04, stevenjb (chromium) wrote: > private: > DISALLOW_COPY... Done. https://codereview.chromium.org/22295002/diff/157001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.h (right): https://codereview.chromium.org/22295002/diff/157001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.h:23: // This class is the client for the out of process networking private handler. On 2013/09/11 17:30:02, Jorge Lucangeli Obes wrote: > Nit: "out-of-process", "networkingPrivate" Done. https://codereview.chromium.org/22295002/diff/157001/chrome/chrome_dll.gypi File chrome/chrome_dll.gypi (right): https://codereview.chromium.org/22295002/diff/157001/chrome/chrome_dll.gypi#n... chrome/chrome_dll.gypi:196: 'Wlanapi.dll', On 2013/09/11 17:30:02, Jorge Lucangeli Obes wrote: > Why capital 'W'? Done. https://codereview.chromium.org/22295002/diff/157001/chrome/common/networking... File chrome/common/networking_private_messages.h (right): https://codereview.chromium.org/22295002/diff/157001/chrome/common/networking... chrome/common/networking_private_messages.h:55: // |properties| are properties convertable to NetworkProperties API object. On 2013/09/11 17:30:02, Jorge Lucangeli Obes wrote: > Nit: "convertible" Done. https://codereview.chromium.org/22295002/diff/157001/chrome/utility/networkin... File chrome/utility/networking_private_handler.h (right): https://codereview.chromium.org/22295002/diff/157001/chrome/utility/networkin... chrome/utility/networking_private_handler.h:24: // Dispatches IPCs for out of process profile import. On 2013/09/11 17:30:02, Jorge Lucangeli Obes wrote: > Outdated comment. Done. https://codereview.chromium.org/22295002/diff/157001/chrome/utility/wifi/wifi... File chrome/utility/wifi/wifi_service.h (right): https://codereview.chromium.org/22295002/diff/157001/chrome/utility/wifi/wifi... chrome/utility/wifi/wifi_service.h:5: // This file defiles the WiFiService interface used by chrome.networkingPrivate On 2013/09/11 17:30:02, Jorge Lucangeli Obes wrote: > defines. Done.
Hi, please take a look at the functionally complete implementation. It passes unit_tests and browser_tests, and is reliably working with Google Cast extension during manual developer testing. I have not yet followed up on suggestion to convert WiFiService::NetworkProperties to a string container as I'm working on moving onc_constants from chromeos/ to components/ and plan to use them once that CL is landed. I'm also working on Mac OS X implementation, but that will be done as a separate CL based on this one. thanks, -m https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:28: namespace { On 2013/08/13 23:39:04, stevenjb wrote: > nit: WS Done. https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:36: kNetworkingPrivateProcessClient, On 2013/08/21 17:25:34, mef wrote: > On 2013/08/19 19:46:34, cbentzel wrote: > > I'm a little confused about how the scope of the > NetworkingPrivateProcessClient > > is tied to that of the Profile. > > > > In particular, it's a refcounted object due to UtilityProcessClient, but if I > > understand this code correctly the profile holds a raw pointer to it through > > SetUserData. It should probably use a refcounted pointer, or at least do > manual > > calls to AddRef here and Release when the Profile tears down. > > Good point. I have not think this through yet. > > I need NetworkingPrivateProcessClient to survive between function calls (to > preserve state and broadcast events), but I haven't found a good way to > determine when extension comes and goes to control its lifespan. Ben suggested > to base it on registration of event subscribers, and I'm planning to do that > unless there is some better way. > > I will make sure to provide proper refcounting at that point. > Done. https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:42: } On 2013/08/13 23:39:04, stevenjb wrote: > nit: WS Done. https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:252: const base::Value* v = *it; On 2013/08/13 23:59:30, Greg Spencer (Chromium) wrote: > Please use a more descriptive variable name than 'v'. Done. https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:28: NetworkingPrivateProcessClient::~NetworkingPrivateProcessClient() {} On 2013/08/13 23:39:04, stevenjb wrote: > nit: } on new line git cl format insists otherwise. https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:50: // source_profile_, items_, localized_strings)); On 2013/08/13 23:39:04, stevenjb wrote: > ?? Done. https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:55: // if (cancelled_) On 2013/08/13 23:59:30, Greg Spencer (Chromium) wrote: > Yes, remove all commented out code (or uncomment). Done. I've added proper logic to handle utility process crash. https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:58: // process_importer_host_->Cancel(); On 2013/08/13 23:39:04, stevenjb wrote: > ?? Done. https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:97: // MessageBoxA(NULL, __FUNCTION__, "Debug Me", MB_OK); On 2013/08/13 23:39:04, stevenjb wrote: > ?? Done. https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:103: base::Bind(&NetworkingPrivateProcessClient::OnGetPropertiesStart, On 2013/08/21 17:25:34, mef wrote: > On 2013/08/19 19:46:34, cbentzel wrote: > > What happens if the utility process is killed while this is outstanding? > Worried > > that the extension API call will stall forever rather than error back to the > > user in that case. > Good point. I guess NetworkingPrivateProcessClient::OnProcessCrashed should > invoke error_callback, > which also points out that there is no need to have different error_callbacks > for different api calls. Done. https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:293: /* On 2013/08/13 23:39:04, stevenjb wrote: > ?? Done. https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... File chrome/browser/extensions/api/networking_private/networking_private_process_client.h (right): https://codereview.chromium.org/22295002/diff/49001/chrome/browser/extensions... chrome/browser/extensions/api/networking_private/networking_private_process_client.h:130: // ExternalProcessImporterHost* process_importer_host_; On 2013/08/21 17:25:34, mef wrote: > On 2013/08/13 23:39:04, stevenjb (chromium) wrote: > > ?? > Leftovers from the class that I've used for inspiration. :-) Done.
The API changes are looking pretty good. I didn't look at the windows specific code, hopefully someone more familiar with Windows code can review that. https://codereview.chromium.org/22295002/diff/233001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_event_router.h (right): https://codereview.chromium.org/22295002/diff/233001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_event_router.h:22: public chromeos::NetworkStateHandlerObserver, We should make this a pure virtual class and declare NetworkingPrivateEventRouterImpl in networking_private_event_router_chrmeos.cc, moving the Chrome OS (and other) implementation specific details there. https://codereview.chromium.org/22295002/diff/233001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_event_router_chromeos.cc (right): https://codereview.chromium.org/22295002/diff/233001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_event_router_chromeos.cc:39: extensions::api::networking_private::OnNetworksChanged::kEventName); extensions:: not needed, here and below https://codereview.chromium.org/22295002/diff/233001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_event_router_factory.cc (right): https://codereview.chromium.org/22295002/diff/233001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_event_router_factory.cc:41: return new NetworkingPrivateEventRouter(static_cast<Profile*>(profile)); This should be a static, NetworkingPrivateEventRouter::Create(), implemented in the os-specific .cc files. https://codereview.chromium.org/22295002/diff/233001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/233001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:15: namespace api = extensions::api::networking_private; None of this should be necessary since we are now using the extensions namespace. https://codereview.chromium.org/22295002/diff/233001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:33: StartOrStopListeningForNetworkChanges(); This looks the same as the ChromeOS specific implementation. Rather than making NetworkingPrivateEventRouter a pure virtual, maybe it makes sense to have a base implementation with OS specific derived implementations? https://codereview.chromium.org/22295002/diff/233001/chrome/utility/chrome_co... File chrome/utility/chrome_content_utility_client.cc (right): https://codereview.chromium.org/22295002/diff/233001/chrome/utility/chrome_co... chrome/utility/chrome_content_utility_client.cc:87: handlers_.push_back(new NetworkingPrivateHandler()); I know the code for this is not OS specific, but does it incur any overhead? Maybe we should only create it for platforms that need it, i.e. just WIN for now? https://codereview.chromium.org/22295002/diff/233001/chrome/utility/networkin... File chrome/utility/networking_private_handler.cc (right): https://codereview.chromium.org/22295002/diff/233001/chrome/utility/networkin... chrome/utility/networking_private_handler.cc:212: } WS https://codereview.chromium.org/22295002/diff/233001/chrome/utility/networkin... File chrome/utility/networking_private_handler.h (right): https://codereview.chromium.org/22295002/diff/233001/chrome/utility/networkin... chrome/utility/networking_private_handler.h:31: virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE; WS https://codereview.chromium.org/22295002/diff/233001/chrome/utility/networkin... chrome/utility/networking_private_handler.h:39: void OnUseWiFiServiceMock(const base::DictionaryValue& parameters); Append ForTest to the end of this if only used for testing. https://codereview.chromium.org/22295002/diff/233001/chrome/utility/networkin... chrome/utility/networking_private_handler.h:50: void OnGetPropertiesStart(int message_id, const std::string& network_guid); WS https://codereview.chromium.org/22295002/diff/233001/chrome/utility/networkin... chrome/utility/networking_private_handler.h:70: void OnStartConnectStart(int message_id, const std::string& network_guid); WS https://codereview.chromium.org/22295002/diff/233001/chrome/utility/networkin... chrome/utility/networking_private_handler.h:78: void OnStartDisconnectStart(int message_id, const std::string& network_guid); WS https://codereview.chromium.org/22295002/diff/233001/chrome/utility/networkin... chrome/utility/networking_private_handler.h:86: void OnRequestNetworkScan(); WS https://codereview.chromium.org/22295002/diff/233001/chrome/utility/networkin... chrome/utility/networking_private_handler.h:92: void OnGetVisibleNetworks(int message_id); WS https://codereview.chromium.org/22295002/diff/233001/chrome/utility/networkin... chrome/utility/networking_private_handler.h:107: scoped_ptr<WiFiService> wifi_service_; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/22295002/diff/233001/chrome/utility/wifi/wifi... File chrome/utility/wifi/wifi_service_test.cc (right): https://codereview.chromium.org/22295002/diff/233001/chrome/utility/wifi/wifi... chrome/utility/wifi/wifi_service_test.cc:13: namespace wifi { We should be consistent and either use 'namespace wifi' for all of the classes in this directory, or none. I slightly prefer using the namespace.
https://codereview.chromium.org/22295002/diff/248001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/248001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:27: const char kType[] = "Type"; anonymous namespace? https://codereview.chromium.org/22295002/diff/248001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:291: void NetworkingPrivateStartConnectFunction::ConnectionStartSuccess() { nit: put callbacks after RunImpl https://codereview.chromium.org/22295002/diff/248001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:355: namespace { can you move this at the beginning of the file? https://codereview.chromium.org/22295002/diff/248001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_event_router.h (right): https://codereview.chromium.org/22295002/diff/248001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_event_router.h:21: #ifdef OS_CHROMEOS #if defined(OS_CHROMEOS) https://codereview.chromium.org/22295002/diff/248001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/248001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:72: process_client->ShutdownProcessClient(); this looks fishy.. wouldn't shutting down the whole process client break networkingPrivate function calls? the extension code could have something like this: chrome.networkingPrivate.addObserver(); chrome.networkingPrivate.removeObserver(); chrome.networkingPrivate.getVisibleNetworks(); https://codereview.chromium.org/22295002/diff/248001/chrome/common/extensions... File chrome/common/extensions/api/_api_features.json (left): https://codereview.chromium.org/22295002/diff/248001/chrome/common/extensions... chrome/common/extensions/api/_api_features.json:358: "platform": "chromeos", can we specify multiple platforms here? it would be nice to enable the api only where's implemented
Hi guys, PTAL. thanks, -m https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... File chrome/utility/wifi/wifi_service.cc (right): https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... chrome/utility/wifi/wifi_service.cc:39: const char kWiFiSignalStrength[] = "WiFi.SignalStrength"; On 2013/08/21 17:25:34, mef wrote: > On 2013/08/13 23:39:04, stevenjb (chromium) wrote: > > Probably worth defining these in a common place. > > > > Is there a translation layer between these and networkingPrivate? If not, we > > should use the ONC constants directly. > > I'd love to use ONC constants, but they seem to be ChromeOS-specific. Should I > move them somewhere? Done. https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... chrome/utility/wifi/wifi_service.cc:83: } On 2013/08/21 17:25:34, mef wrote: > On 2013/08/13 23:39:04, stevenjb (chromium) wrote: > > One advantage to sticking with strings is not having to maintain a bunch of > > functions like these. I can almost guarantee you will also want the inverse of > > these soon if you use enums... > Good point, will follow your advise. Done. https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... File chrome/utility/wifi/wifi_service.h (right): https://codereview.chromium.org/22295002/diff/49001/chrome/utility/wifi/wifi_... chrome/utility/wifi/wifi_service.h:49: }; On 2013/08/21 17:25:34, mef wrote: > On 2013/08/13 23:39:04, stevenjb (chromium) wrote: > > FWIW, the original ChromeOS networking code used a bunch of enums like these, > > which turned out to be a huge PITA to translate and maintain across processes > > and subsystems. The new code just uses strings which, as near as I can tell > has > > been a win all around, especially for debugging purposes. > > Thanks a lot for valuable insight! My initial intention was to decouple > WiFiService from networking_private-specific constants, but I don't think there > is enough benefit there. I'll change it to use strings and string constants. Done. https://codereview.chromium.org/22295002/diff/233001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_event_router.h (right): https://codereview.chromium.org/22295002/diff/233001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_event_router.h:22: public chromeos::NetworkStateHandlerObserver, On 2013/09/25 19:09:15, stevenjb wrote: > We should make this a pure virtual class and declare > NetworkingPrivateEventRouterImpl in networking_private_event_router_chrmeos.cc, > moving the Chrome OS (and other) implementation specific details there. Done. https://codereview.chromium.org/22295002/diff/233001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_event_router_chromeos.cc (right): https://codereview.chromium.org/22295002/diff/233001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_event_router_chromeos.cc:39: extensions::api::networking_private::OnNetworksChanged::kEventName); On 2013/09/25 19:09:15, stevenjb wrote: > extensions:: not needed, here and below Done. https://codereview.chromium.org/22295002/diff/233001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_event_router_factory.cc (right): https://codereview.chromium.org/22295002/diff/233001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_event_router_factory.cc:41: return new NetworkingPrivateEventRouter(static_cast<Profile*>(profile)); On 2013/09/25 19:09:15, stevenjb wrote: > This should be a static, NetworkingPrivateEventRouter::Create(), implemented in > the os-specific .cc files. Done. https://codereview.chromium.org/22295002/diff/233001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/233001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:15: namespace api = extensions::api::networking_private; On 2013/09/25 19:09:15, stevenjb wrote: > None of this should be necessary since we are now using the extensions > namespace. Done. https://codereview.chromium.org/22295002/diff/233001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:33: StartOrStopListeningForNetworkChanges(); On 2013/09/25 19:09:15, stevenjb wrote: > This looks the same as the ChromeOS specific implementation. Rather than making > NetworkingPrivateEventRouter a pure virtual, maybe it makes sense to have a base > implementation with OS specific derived implementations? It seems that common parts are trivial, but differences are significant, so I've done 2 separate implementations. https://codereview.chromium.org/22295002/diff/233001/chrome/utility/chrome_co... File chrome/utility/chrome_content_utility_client.cc (right): https://codereview.chromium.org/22295002/diff/233001/chrome/utility/chrome_co... chrome/utility/chrome_content_utility_client.cc:87: handlers_.push_back(new NetworkingPrivateHandler()); On 2013/09/25 19:09:15, stevenjb wrote: > I know the code for this is not OS specific, but does it incur any overhead? > Maybe we should only create it for platforms that need it, i.e. just WIN for > now? Done. https://codereview.chromium.org/22295002/diff/233001/chrome/utility/networkin... File chrome/utility/networking_private_handler.cc (right): https://codereview.chromium.org/22295002/diff/233001/chrome/utility/networkin... chrome/utility/networking_private_handler.cc:212: } On 2013/09/25 19:09:15, stevenjb wrote: > WS Done. https://codereview.chromium.org/22295002/diff/233001/chrome/utility/networkin... File chrome/utility/networking_private_handler.h (right): https://codereview.chromium.org/22295002/diff/233001/chrome/utility/networkin... chrome/utility/networking_private_handler.h:31: virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE; On 2013/09/25 19:09:15, stevenjb wrote: > WS Done. https://codereview.chromium.org/22295002/diff/233001/chrome/utility/networkin... chrome/utility/networking_private_handler.h:39: void OnUseWiFiServiceMock(const base::DictionaryValue& parameters); On 2013/09/25 19:09:15, stevenjb wrote: > Append ForTest to the end of this if only used for testing. Done. https://codereview.chromium.org/22295002/diff/233001/chrome/utility/networkin... chrome/utility/networking_private_handler.h:50: void OnGetPropertiesStart(int message_id, const std::string& network_guid); On 2013/09/25 19:09:15, stevenjb wrote: > WS Done. https://codereview.chromium.org/22295002/diff/233001/chrome/utility/networkin... chrome/utility/networking_private_handler.h:70: void OnStartConnectStart(int message_id, const std::string& network_guid); On 2013/09/25 19:09:15, stevenjb wrote: > WS Done. https://codereview.chromium.org/22295002/diff/233001/chrome/utility/networkin... chrome/utility/networking_private_handler.h:78: void OnStartDisconnectStart(int message_id, const std::string& network_guid); On 2013/09/25 19:09:15, stevenjb wrote: > WS Done. https://codereview.chromium.org/22295002/diff/233001/chrome/utility/networkin... chrome/utility/networking_private_handler.h:86: void OnRequestNetworkScan(); On 2013/09/25 19:09:15, stevenjb wrote: > WS Done. https://codereview.chromium.org/22295002/diff/233001/chrome/utility/networkin... chrome/utility/networking_private_handler.h:92: void OnGetVisibleNetworks(int message_id); On 2013/09/25 19:09:15, stevenjb wrote: > WS Done. https://codereview.chromium.org/22295002/diff/233001/chrome/utility/networkin... chrome/utility/networking_private_handler.h:107: scoped_ptr<WiFiService> wifi_service_; On 2013/09/25 19:09:15, stevenjb wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/22295002/diff/233001/chrome/utility/wifi/wifi... File chrome/utility/wifi/wifi_service_test.cc (right): https://codereview.chromium.org/22295002/diff/233001/chrome/utility/wifi/wifi... chrome/utility/wifi/wifi_service_test.cc:13: namespace wifi { On 2013/09/25 19:09:15, stevenjb wrote: > We should be consistent and either use 'namespace wifi' for all of the classes > in this directory, or none. I slightly prefer using the namespace. Done. https://codereview.chromium.org/22295002/diff/248001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/248001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:27: const char kType[] = "Type"; On 2013/10/03 20:19:00, tbarzic wrote: > anonymous namespace? Done. Used onc constants instead. https://codereview.chromium.org/22295002/diff/248001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:291: void NetworkingPrivateStartConnectFunction::ConnectionStartSuccess() { On 2013/10/03 20:19:00, tbarzic wrote: > nit: put callbacks after RunImpl Done. https://codereview.chromium.org/22295002/diff/248001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:355: namespace { On 2013/10/03 20:19:00, tbarzic wrote: > can you move this at the beginning of the file? Done. https://codereview.chromium.org/22295002/diff/248001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_event_router.h (right): https://codereview.chromium.org/22295002/diff/248001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_event_router.h:21: #ifdef OS_CHROMEOS On 2013/10/03 20:19:00, tbarzic wrote: > #if defined(OS_CHROMEOS) Done. Re-factored out. https://codereview.chromium.org/22295002/diff/248001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/248001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:72: process_client->ShutdownProcessClient(); On 2013/10/03 20:19:00, tbarzic wrote: > this looks fishy.. > wouldn't shutting down the whole process client break networkingPrivate function > calls? > the extension code could have something like this: > > chrome.networkingPrivate.addObserver(); > chrome.networkingPrivate.removeObserver(); > chrome.networkingPrivate.getVisibleNetworks(); I agree, however I've been told that this is the only indication of the extension lifespan. I'll be glad to use something else if it exists. Luckily this will not break consecutive calls as they will just start a new instance of Utility Process. https://codereview.chromium.org/22295002/diff/248001/chrome/common/extensions... File chrome/common/extensions/api/_api_features.json (left): https://codereview.chromium.org/22295002/diff/248001/chrome/common/extensions... chrome/common/extensions/api/_api_features.json:358: "platform": "chromeos", On 2013/10/03 20:19:00, tbarzic wrote: > can we specify multiple platforms here? > it would be nice to enable the api only where's implemented AFAIU currently "platform" could only specify "chromeos" to limit it to Chrome OS, no other platforms are recognized.
I didn't look super closely at networking_private_process_client.cc or the code in chrome/utility/, we'll need to find someone more familiar with Windows and IPC to look at that, but other than a few small things this looks pretty good. https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:28: namespace { nit: WS https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:29: bool VerifyDestionationProperties( Destination https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:47: }; // namespace nit: WS above, no ; https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:385: if (process_client->using_wifi_service_mock()) { Could we make VerifyDestinationProperties a member of process_client, rather than exposing using_wifi_service_mock() here (and below)? https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:445: } Shouldn't we SetResult(false) if Verify fails? https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_event_router_chromeos.cc (right): https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_event_router_chromeos.cc:118: chromeos::NetworkHandler::Get()->network_state_handler()->AddObserver( If you add 'using chromeos::NetworkHandler', etc, at the top of this .cc file we can avoid the extra diffs and keep the code shoreter. https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:46: : profile_(profile), listening_(false) { One line per arg
https://codereview.chromium.org/22295002/diff/248001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/248001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:72: process_client->ShutdownProcessClient(); On 2013/10/08 21:46:26, mef wrote: > On 2013/10/03 20:19:00, tbarzic wrote: > > this looks fishy.. > > wouldn't shutting down the whole process client break networkingPrivate > function > > calls? > > the extension code could have something like this: > > > > chrome.networkingPrivate.addObserver(); > > chrome.networkingPrivate.removeObserver(); > > chrome.networkingPrivate.getVisibleNetworks(); > > I agree, however I've been told that this is the only indication of the > extension lifespan. > I'll be glad to use something else if it exists. > > Luckily this will not break consecutive calls as they will just start a new > instance of Utility Process. > Can it break API calls (possibly from another extension) that are in progress when the client is shutdown? Also, if an API function is called after all observers have been removed, the process_client may not shutdown before Chrome exit. https://codereview.chromium.org/22295002/diff/248001/chrome/common/extensions... File chrome/common/extensions/api/_api_features.json (left): https://codereview.chromium.org/22295002/diff/248001/chrome/common/extensions... chrome/common/extensions/api/_api_features.json:358: "platform": "chromeos", On 2013/10/08 21:46:26, mef wrote: > On 2013/10/03 20:19:00, tbarzic wrote: > > can we specify multiple platforms here? > > it would be nice to enable the api only where's implemented > > AFAIU currently "platform" could only specify "chromeos" to limit it to Chrome > OS, no other platforms are recognized. ok, we could add "platforms": ["windows", "chromeos"] to c/c/e/api/networking_private.json It would be nice to keep the API detection by checking !!chrome.networkingPrivate https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:445: } On 2013/10/10 18:55:09, stevenjb wrote: > Shouldn't we SetResult(false) if Verify fails? I think leaving undefined result is OK; but we may want to set error and return false. https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:264: extensions::ExtensionSystem::Get(profile_)->event_router(); afaik, ExtensionSystem::Get has to be called on the UI thread. I think this is on IO thread? also, I'd prefer if this didn't depend on the extension system at all. Why don't you add an observer interface the networking private event router could use to listen for network (list) changes (and raise the extension events from there) with this approach you could also move the logic that decides if the process client can shut down from the event router here. That way you can better handle api calls from extensions that don't listen to networking private events (e.g. shutdown the client if there are no observers and no pending callbacks) https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:310: if (!message_callbacks->start_disconnect_callback.is_null()) should this be a DCHECK?
Thank you for your comments! I've addressed some of them, but I have a few followup questions to help me with others. thanks, -m https://codereview.chromium.org/22295002/diff/248001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/248001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:72: process_client->ShutdownProcessClient(); On 2013/10/10 20:58:39, tbarzic wrote: > On 2013/10/08 21:46:26, mef wrote: > > On 2013/10/03 20:19:00, tbarzic wrote: > > > this looks fishy.. > > > wouldn't shutting down the whole process client break networkingPrivate > > function > > > calls? > > > the extension code could have something like this: > > > > > > chrome.networkingPrivate.addObserver(); > > > chrome.networkingPrivate.removeObserver(); > > > chrome.networkingPrivate.getVisibleNetworks(); > > > > I agree, however I've been told that this is the only indication of the > > extension lifespan. > > I'll be glad to use something else if it exists. > > > > Luckily this will not break consecutive calls as they will just start a new > > instance of Utility Process. > > > > Can it break API calls (possibly from another extension) that are in progress > when the client is shutdown? It shouldn't break networking Private API calls, even from another extension, and I believe that it gets its own designated instance of Utility Process. > Also, if an API function is called after all observers have been removed, the > process_client may not shutdown before Chrome exit. > https://codereview.chromium.org/22295002/diff/248001/chrome/common/extensions... File chrome/common/extensions/api/_api_features.json (left): https://codereview.chromium.org/22295002/diff/248001/chrome/common/extensions... chrome/common/extensions/api/_api_features.json:358: "platform": "chromeos", On 2013/10/10 20:58:39, tbarzic wrote: > On 2013/10/08 21:46:26, mef wrote: > > On 2013/10/03 20:19:00, tbarzic wrote: > > > can we specify multiple platforms here? > > > it would be nice to enable the api only where's implemented > > > > AFAIU currently "platform" could only specify "chromeos" to limit it to Chrome > > OS, no other platforms are recognized. > > ok, we could add "platforms": ["windows", "chromeos"] to > c/c/e/api/networking_private.json > It would be nice to keep the API detection by checking > !!chrome.networkingPrivate > > Fair point. I'll add other platforms handling as separate CL. https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:28: namespace { On 2013/10/10 18:55:09, stevenjb wrote: > nit: WS Done. https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:29: bool VerifyDestionationProperties( On 2013/10/10 18:55:09, stevenjb wrote: > Destination Done. https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:47: }; // namespace On 2013/10/10 18:55:09, stevenjb wrote: > nit: WS above, no ; Done. https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:385: if (process_client->using_wifi_service_mock()) { On 2013/10/10 18:55:09, stevenjb wrote: > Could we make VerifyDestinationProperties a member of process_client, rather > than exposing using_wifi_service_mock() here (and below)? Actually, there is more fundamental question, for which I don't have a good answer. The expected result from this function is drastically different in browser_tests and in production chrome. Is there some way to make this switch at compile rather than run time? Alternatively, we could switch ChromeOS implementation to use the same NetworkingPrivateCrypto implementation instead of current shims and shills, and update the test to use and expect valid data instead of fake. I'm looking for suggestions. https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:445: } On 2013/10/10 20:58:40, tbarzic wrote: > On 2013/10/10 18:55:09, stevenjb wrote: > > Shouldn't we SetResult(false) if Verify fails? > > I think leaving undefined result is OK; > but we may want to set error and return false. Done. https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:264: extensions::ExtensionSystem::Get(profile_)->event_router(); On 2013/10/10 20:58:40, tbarzic wrote: > afaik, ExtensionSystem::Get has to be called on the UI thread. > I think this is on IO thread? I think so. Should I post it to UI thread from here? > also, I'd prefer if this didn't depend on the extension system at all. > Why don't you add an observer interface the networking private event router > could use to listen for network (list) changes (and raise the extension events > from there) > > with this approach you could also move the logic that decides if the process > client can shut down from the event router here. That way you can better handle > api calls from extensions that don't listen to networking private events (e.g. > shutdown the client if there are no observers and no pending callbacks) Could you elaborate a little? It sounds like a right idea, I just want to understand it better. Should I get rid of NetworkingPrivateEventRouterImpl in Windows and instead make NetworkingPrivateProcessClient an observer to subscriptions? https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:310: if (!message_callbacks->start_disconnect_callback.is_null()) On 2013/10/10 20:58:40, tbarzic wrote: > should this be a DCHECK? Wouldn't start_disconnect_callback be NULL if extension unloaded between start and 'succeeded'?
https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:385: if (process_client->using_wifi_service_mock()) { On 2013/10/11 22:16:29, mef wrote: > On 2013/10/10 18:55:09, stevenjb wrote: > > Could we make VerifyDestinationProperties a member of process_client, rather > > than exposing using_wifi_service_mock() here (and below)? > > Actually, there is more fundamental question, for which I don't have a good > answer. > The expected result from this function is drastically different in browser_tests > and in production chrome. > Is there some way to make this switch at compile rather than run time? > > Alternatively, we could switch ChromeOS implementation to use the same > NetworkingPrivateCrypto implementation instead of current shims and shills, and > update the test to use and expect valid data instead of fake. > > I'm looking for suggestions. We avoid changing the compile behavior between chrome and browser_tests for all sorts of good reasons. You want the code being tested to be the same, even if mocks and stubs are being used to simulate runtime behavior, which is why I don't think we should be testing using_wifi_service_mock() here. I'm not sure I understand your proposal for the ChromeOS implementation, but I'm also not especially familiar with this particular function. Is there a specific objection to moving VerifyDestionationProperties to NetworkingPrivateProcessClient?
https://codereview.chromium.org/22295002/diff/248001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/248001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:68: Shouldn't you create the NetworkingPrivateProcessClient in case (should_listen && !listenning_). https://codereview.chromium.org/22295002/diff/248001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:72: process_client->ShutdownProcessClient(); On 2013/10/11 22:16:29, mef wrote: > On 2013/10/10 20:58:39, tbarzic wrote: > > On 2013/10/08 21:46:26, mef wrote: > > > On 2013/10/03 20:19:00, tbarzic wrote: > > > > this looks fishy.. > > > > wouldn't shutting down the whole process client break networkingPrivate > > > function > > > > calls? > > > > the extension code could have something like this: > > > > > > > > chrome.networkingPrivate.addObserver(); > > > > chrome.networkingPrivate.removeObserver(); > > > > chrome.networkingPrivate.getVisibleNetworks(); > > > > > > I agree, however I've been told that this is the only indication of the > > > extension lifespan. > > > I'll be glad to use something else if it exists. > > > > > > Luckily this will not break consecutive calls as they will just start a new > > > instance of Utility Process. > > > > > > > Can it break API calls (possibly from another extension) that are in progress > > when the client is shutdown? > It shouldn't break networking Private API calls, even from another extension, > and I believe that it gets its own designated instance of Utility Process. > wait, isn't NetworkingPrivateProcessClient reused inside the same profile (and thus also the utility process since the process client has exactly on utility process)? It looks as if you're using NetworkingPrivateProcessClient::GetProcessClientForProfile(profile_) everywhere you need the utility process. I was thinking about the following (actually we could have the same race with extension A == extension B): extension A adds a listener for networkListChanged extension B calls getVisibleNetworks (doesn't receive callback yet) extension A removes the listener for networkListChanged (and thus shuts down the NetworkPrivateProcessClient) before visible networks requested by extension B are calculated by the utility process. Would extension B get a callback getVisibleNetworks (as the process client is shutdown and its utility_process_host_ is reset). https://codereview.chromium.org/22295002/diff/248001/chrome/common/extensions... File chrome/common/extensions/api/_api_features.json (left): https://codereview.chromium.org/22295002/diff/248001/chrome/common/extensions... chrome/common/extensions/api/_api_features.json:358: "platform": "chromeos", On 2013/10/11 22:16:29, mef wrote: > On 2013/10/10 20:58:39, tbarzic wrote: > > On 2013/10/08 21:46:26, mef wrote: > > > On 2013/10/03 20:19:00, tbarzic wrote: > > > > can we specify multiple platforms here? > > > > it would be nice to enable the api only where's implemented > > > > > > AFAIU currently "platform" could only specify "chromeos" to limit it to > Chrome > > > OS, no other platforms are recognized. > > > > ok, we could add "platforms": ["windows", "chromeos"] to > > c/c/e/api/networking_private.json > > It would be nice to keep the API detection by checking > > !!chrome.networkingPrivate > > > > > Fair point. I'll add other platforms handling as separate CL. sg https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:101: utility_process_host_->Send(message); if (utility_process_host_)? https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:120: utility_process_host_.reset(); I don't think you need this (utility_process_host_ should get deleted when process crashes, which should invalidate the weak ptr). https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:185: Send(new NetworkingPrivateMsg_SetProperties( please break this into: IDMap::KeyType callbacks_id = callbacks_map_.Add(message_callback); Send(new NetworkingPrivateMsg_SetProperties( callbacks_id, service_path, *properties_ptr)); https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:264: extensions::ExtensionSystem::Get(profile_)->event_router(); On 2013/10/11 22:16:29, mef wrote: > On 2013/10/10 20:58:40, tbarzic wrote: > > afaik, ExtensionSystem::Get has to be called on the UI thread. > > I think this is on IO thread? > I think so. Should I post it to UI thread from here? > Actually, looking at UtilityProcessHost, it seems that OnMessageReceived is called on the thread passed to it in UtilityProcessHost::Create; in which case this is ok. Can you double-check this? > > also, I'd prefer if this didn't depend on the extension system at all. > > Why don't you add an observer interface the networking private event router > > could use to listen for network (list) changes (and raise the extension events > > from there) > > > > with this approach you could also move the logic that decides if the process > > client can shut down from the event router here. That way you can better > handle > > api calls from extensions that don't listen to networking private events (e.g. > > shutdown the client if there are no observers and no pending callbacks) > > Could you elaborate a little? It sounds like a right idea, I just want to > understand it better. > Should I get rid of NetworkingPrivateEventRouterImpl in Windows and instead > make NetworkingPrivateProcessClient an observer to subscriptions? what I meant was to introduce an observer interface for the NetworkingPrivateProcessClient: Observer { virtual void OnNetworksChanged(vector<string>guids) = 0; virtual void OnNetworkListChanged(vector<string>guids) = 0; } NetworkingPrivateEventRouterImpl would implement this interface and the overrides would do the same that's currently done here (broadcast the extension events). This function would change to something like OnNetworksChangedEvent() {FOR_EACH_OBSERVER(Observer, observer_list_, OnNetwroksChanged(network_guids))}; NetworkingPrivateEventRouterImpl would add itself as an observer in StartOrStopListeningForNetworkChanges(). basically the same thing what we have in the chromeos implementation, but instead of observing NetworkStateHandlerObserver, you'd be observing NetworkingPrivateProcessClient. https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:310: if (!message_callbacks->start_disconnect_callback.is_null()) On 2013/10/11 22:16:29, mef wrote: > On 2013/10/10 20:58:40, tbarzic wrote: > > should this be a DCHECK? > Wouldn't start_disconnect_callback be NULL if extension unloaded between start > and 'succeeded'? I can't see a reason it would.. Am I missing something obvious? It doesn't seem like you're resetting the callback anywhere on extension unload. https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:267: (params->type == api::GetVisibleNetworks::Params::TYPE_ALL); nit: I don't think you need () https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:435: if (VerifyDestinationProperties(params->properties)) { I'd write this as: if (!VerifyDestinationProperties(params->properties)) { error_ = "VerifyDestinationError"; return false; // no need for SendResponse(false) if you return false. } // do encrypting https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:437: if (base::Base64Decode(params->properties.public_key, &public_key)) { you should also set errors for other potential failures here (invalid key, failure to encrypt, unable to encode data). https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:188: *properties_ptr.Pass())); Wouldn't this work without Pass()
This is a really big CL at this point. Is it possible to break it up or will that just add more time for you and reviewers? Taking a look now.
On 2013/10/14 09:30:04, cbentzel wrote: > This is a really big CL at this point. > > Is it possible to break it up or will that just add more time for you and > reviewers? > > Taking a look now. I could probably split off wifi_service* as it doesn't depend on IPC messaging, utility process and Networking Private API itself, but those 3 parts are pretty interdependent.
On 2013/10/14 20:42:53, mef wrote: > On 2013/10/14 09:30:04, cbentzel wrote: > > This is a really big CL at this point. > > > > Is it possible to break it up or will that just add more time for you and > > reviewers? > > > > Taking a look now. > > I could probably split off wifi_service* as it doesn't depend on IPC messaging, > utility process and Networking Private API itself, > but those 3 parts are pretty interdependent. That's what I was thinking. Just have a CL which takes care of the utility process with stubbed out methods, and a separate CL for the actual implementation of the methods.
A few possibly stale comments. Ignore the "Note to self:" ones. https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:64: NetworkingPrivateProcessClient::GetProcessClientForProfile(profile_)); Note to self: Make sure that the refcouunting is needed for the process client. https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:84: scoped_ptr<base::DictionaryValue> error_data) { Should you be using error_data here to help populate the error string? https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:228: bool NetworkingPrivateCreateNetworkFunction::RunImpl() { It looks like this is still stubbed out? https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:281: network_dict->GetString(onc::network_config::kType, &network_type); Why do you have to do string comparisons here instead of enum comparisons? Is this because the returned data from the child process uses strings rather than numbers to indicate the type? Alternately, could you just pass the type to GetVisibleNetworks and do the filtering in the utility process? https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:384: // Don't do actual verification during browser test. Note to self: see if there's a better way to do this. https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Nit: 2013 https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:42: Start(); I wonder if this should be called after construction in the GetProcessClientForProfile() method. I always get worried about methods called in the constructor while the object may only be partially constructed. https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:72: void NetworkingPrivateProcessClient::ShutdownProcessClient() { Note to self: When is shutdown called? https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:101: utility_process_host_->Send(message); Should this check that utility_process_host_ is non-null? https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:105: DLOG(ERROR) << __FUNCTION__; Remove this DLOG https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:112: scoped_ptr<DictionaryValue> error_data_ptr(new DictionaryValue()); Can you just pass a NULL in rather than construct a new DictionaryValue() for each outstanding callback? Not a huge deal performance-wise, but it doesn't even seem like there are any consumers which use the DictionaryValue at all - they just the error string. https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:113: iter.GetCurrentValue()->error_callback.Run(error_code, Is it possible that invoking any of these callbacks will cause additional callbacks to be added to callbacks_map_ (so mutating the map while iterating over it)? I don't believe this is the case since it would require doing a round trip to the extension process to get these added back, but want to confirm. https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:242: MessageCallbacks* message_callbacks = callbacks_map_.Lookup(message_id); Probably not a big deal - but there's no guarantee that the KeyType for the IDMap will be the same as an int (in fact, it's explicitly specified as an int32). I _think_ all of our toolchains use LP64/LLP64 for 64-bit builds, but there is perhaps a chance of truncation here. It's not doing a direct offset into an array so I'm not too worried. https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.h (right): https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. Nit: 2013 https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.h:25: class NetworkingPrivateProcessClient Should this be within a namespace (such as extensions) instead of the top-level namespace? https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.h:32: typedef base::Callback< Nit: typedefs come before constructor in ordering. https://codereview.chromium.org/22295002/diff/321001/chrome/common/networking... File chrome/common/networking_private_messages.h (right): https://codereview.chromium.org/22295002/diff/321001/chrome/common/networking... chrome/common/networking_private_messages.h:76: std::string /* network_guid */) Nit: inconsistent between Network GUID and network_guid
Hi guys, thanks for your comments, PTAL! I've addressed most of them except for lifetime of NetworkingPrivateProcessClient (and related event router), which will need more thought and chromeos namespace, which I'll address shortly. https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:385: if (process_client->using_wifi_service_mock()) { On 2013/10/11 22:35:46, stevenjb wrote: > On 2013/10/11 22:16:29, mef wrote: > > On 2013/10/10 18:55:09, stevenjb wrote: > > > Could we make VerifyDestinationProperties a member of process_client, rather > > > than exposing using_wifi_service_mock() here (and below)? > > > > Actually, there is more fundamental question, for which I don't have a good > > answer. > > The expected result from this function is drastically different in > browser_tests > > and in production chrome. > > Is there some way to make this switch at compile rather than run time? > > > > Alternatively, we could switch ChromeOS implementation to use the same > > NetworkingPrivateCrypto implementation instead of current shims and shills, > and > > update the test to use and expect valid data instead of fake. > > > > I'm looking for suggestions. > > We avoid changing the compile behavior between chrome and browser_tests for all > sorts of good reasons. You want the code being tested to be the same, even if > mocks and stubs are being used to simulate runtime behavior, which is why I > don't think we should be testing using_wifi_service_mock() here. > > I'm not sure I understand your proposal for the ChromeOS implementation, but I'm > also not especially familiar with this particular function. > > Is there a specific objection to moving VerifyDestionationProperties to > NetworkingPrivateProcessClient? Done. https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_event_router_chromeos.cc (right): https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_event_router_chromeos.cc:118: chromeos::NetworkHandler::Get()->network_state_handler()->AddObserver( On 2013/10/10 18:55:09, stevenjb wrote: > If you add 'using chromeos::NetworkHandler', etc, at the top of this .cc file we > can avoid the extra diffs and keep the code shoreter. Will do. https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:46: : profile_(profile), listening_(false) { On 2013/10/10 18:55:09, stevenjb wrote: > One line per arg Done. https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:101: utility_process_host_->Send(message); On 2013/10/12 02:14:09, tbarzic wrote: > if (utility_process_host_)? Done. https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:120: utility_process_host_.reset(); On 2013/10/12 02:14:09, tbarzic wrote: > I don't think you need this (utility_process_host_ should get deleted when > process crashes, which should invalidate the weak ptr). Done. https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:185: Send(new NetworkingPrivateMsg_SetProperties( On 2013/10/12 02:14:09, tbarzic wrote: > please break this into: > IDMap::KeyType callbacks_id = callbacks_map_.Add(message_callback); > Send(new NetworkingPrivateMsg_SetProperties( > callbacks_id, service_path, *properties_ptr)); Done. https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:310: if (!message_callbacks->start_disconnect_callback.is_null()) On 2013/10/12 02:14:09, tbarzic wrote: > On 2013/10/11 22:16:29, mef wrote: > > On 2013/10/10 20:58:40, tbarzic wrote: > > > should this be a DCHECK? > > Wouldn't start_disconnect_callback be NULL if extension unloaded between start > > and 'succeeded'? > > I can't see a reason it would.. Am I missing something obvious? > It doesn't seem like you're resetting the callback anywhere on extension unload. Done. Just to make sure I understand, what happens if extension unloads (BTW, is there some notification I can observe?) between StartDisconnect and OnStartDisconnectSucceeded? Is NetworkingPrivateStartDisconnectFunction instance still valid or does it get deleted? https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:84: scoped_ptr<base::DictionaryValue> error_data) { On 2013/10/15 10:49:10, cbentzel wrote: > Should you be using error_data here to help populate the error string? I should probably get rid of error_data as I haven't found a way to make it useful. https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:228: bool NetworkingPrivateCreateNetworkFunction::RunImpl() { On 2013/10/15 10:49:10, cbentzel wrote: > It looks like this is still stubbed out? Yes, methods not used for ChromeCast setup are still stubbed out. https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:267: (params->type == api::GetVisibleNetworks::Params::TYPE_ALL); On 2013/10/12 02:14:09, tbarzic wrote: > nit: > I don't think you need () Done. https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:281: network_dict->GetString(onc::network_config::kType, &network_type); On 2013/10/15 10:49:10, cbentzel wrote: > Why do you have to do string comparisons here instead of enum comparisons? Is > this because the returned data from the child process uses strings rather than > numbers to indicate the type? Yes. > Alternately, could you just pass the type to GetVisibleNetworks and do the > filtering in the utility process? Will do. https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:384: // Don't do actual verification during browser test. On 2013/10/15 10:49:10, cbentzel wrote: > Note to self: see if there's a better way to do this. I've changed it per discussion with stevenjb@, see new patchset. https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:435: if (VerifyDestinationProperties(params->properties)) { On 2013/10/12 02:14:09, tbarzic wrote: > I'd write this as: > if (!VerifyDestinationProperties(params->properties)) { > error_ = "VerifyDestinationError"; > return false; // no need for SendResponse(false) if you return false. > } > > // do encrypting > Done. https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:437: if (base::Base64Decode(params->properties.public_key, &public_key)) { On 2013/10/12 02:14:09, tbarzic wrote: > you should also set errors for other potential failures here (invalid key, > failure to encrypt, unable to encode data). Done. https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/10/15 10:49:10, cbentzel wrote: > Nit: 2013 Done. https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:42: Start(); On 2013/10/15 10:49:10, cbentzel wrote: > I wonder if this should be called after construction in the > GetProcessClientForProfile() method. I always get worried about methods called > in the constructor while the object may only be partially constructed. Done. https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:72: void NetworkingPrivateProcessClient::ShutdownProcessClient() { On 2013/10/15 10:49:10, cbentzel wrote: > Note to self: When is shutdown called? Currently it is called from NetworkingPrivateEventRouterImpl::StartOrStopListeningForNetworkChanges, when there are no event listeners, but this will break calls that are in flight, so this needs better solution. https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:101: utility_process_host_->Send(message); On 2013/10/15 10:49:10, cbentzel wrote: > Should this check that utility_process_host_ is non-null? Done. https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:105: DLOG(ERROR) << __FUNCTION__; On 2013/10/15 10:49:10, cbentzel wrote: > Remove this DLOG Done. https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:112: scoped_ptr<DictionaryValue> error_data_ptr(new DictionaryValue()); On 2013/10/15 10:49:10, cbentzel wrote: > Can you just pass a NULL in rather than construct a new DictionaryValue() for > each outstanding callback? Not a huge deal performance-wise, but it doesn't even > seem like there are any consumers which use the DictionaryValue at all - they > just the error string. The error_callback takes scoped_ptr<DictionaryValue> based on API function definitions in networking_private_api.h. https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:113: iter.GetCurrentValue()->error_callback.Run(error_code, On 2013/10/15 10:49:10, cbentzel wrote: > Is it possible that invoking any of these callbacks will cause additional > callbacks to be added to callbacks_map_ (so mutating the map while iterating > over it)? > > I don't believe this is the case since it would require doing a round trip to > the extension process to get these added back, but want to confirm. Good point. It probably might. I'll see how to harden this. https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:188: *properties_ptr.Pass())); On 2013/10/12 02:14:09, tbarzic wrote: > Wouldn't this work without Pass() Nope, I get compilation error: cannot convert parameter 3 from 'scoped_ptr<T>' to 'const base::DictionaryValue &' https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:242: MessageCallbacks* message_callbacks = callbacks_map_.Lookup(message_id); On 2013/10/15 10:49:10, cbentzel wrote: > Probably not a big deal - but there's no guarantee that the KeyType for the > IDMap will be the same as an int (in fact, it's explicitly specified as an > int32). I _think_ all of our toolchains use LP64/LLP64 for 64-bit builds, but > there is perhaps a chance of truncation here. It's not doing a direct offset > into an array so I'm not too worried. Done. https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.h (right): https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/10/15 10:49:10, cbentzel wrote: > Nit: 2013 Done. https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.h:25: class NetworkingPrivateProcessClient On 2013/10/15 10:49:10, cbentzel wrote: > Should this be within a namespace (such as extensions) instead of the top-level > namespace? Done. https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.h:32: typedef base::Callback< On 2013/10/15 10:49:10, cbentzel wrote: > Nit: typedefs come before constructor in ordering. Done. https://codereview.chromium.org/22295002/diff/321001/chrome/common/networking... File chrome/common/networking_private_messages.h (right): https://codereview.chromium.org/22295002/diff/321001/chrome/common/networking... chrome/common/networking_private_messages.h:76: std::string /* network_guid */) On 2013/10/15 10:49:10, cbentzel wrote: > Nit: inconsistent between Network GUID and network_guid Done.
https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:310: if (!message_callbacks->start_disconnect_callback.is_null()) On 2013/10/15 19:47:17, mef wrote: > On 2013/10/12 02:14:09, tbarzic wrote: > > On 2013/10/11 22:16:29, mef wrote: > > > On 2013/10/10 20:58:40, tbarzic wrote: > > > > should this be a DCHECK? > > > Wouldn't start_disconnect_callback be NULL if extension unloaded between > start > > > and 'succeeded'? > > > > I can't see a reason it would.. Am I missing something obvious? > > It doesn't seem like you're resetting the callback anywhere on extension > unload. > > Done. Just to make sure I understand, what happens if extension unloads (BTW, is > there some notification I can observe?) between StartDisconnect and > OnStartDisconnectSucceeded? Is NetworkingPrivateStartDisconnectFunction instance > still valid or does it get deleted? There is a notification (chrome::NOTIFICATION_EXTENSION_UNLOADED), I don't think you need it, though. The ExtensionFunction is refcounted, so yes, it's still valid. When the extension is unloaded the callback from the ExtensionFunction to the extension gets invalidated (so SendResponse calls from the ExtensionFunction don't have any effect), but the ExtensionFunction object stays around. https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:188: *properties_ptr.Pass())); On 2013/10/15 19:47:18, mef wrote: > On 2013/10/12 02:14:09, tbarzic wrote: > > Wouldn't this work without Pass() > Nope, I get compilation error: cannot convert parameter 3 from 'scoped_ptr<T>' > to 'const base::DictionaryValue &' I'd expect that error when you use properties_ptr, *properties_ptr should work https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.h (right): https://codereview.chromium.org/22295002/diff/299001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.h:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/10/15 19:47:18, mef wrote: > On 2013/10/15 10:49:10, cbentzel wrote: > > Nit: 2013 > > Done. also, no (c) https://codereview.chromium.org/22295002/diff/331001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/331001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:27: remove the lines https://codereview.chromium.org/22295002/diff/334001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/334001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:385: if (!message_callbacks->set_properties_callback.is_null()) you don't need if when you have DCHECK
Almost finished a full review - this is a big patch. https://codereview.chromium.org/22295002/diff/334001/chrome/browser/profiles/... File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc (right): https://codereview.chromium.org/22295002/diff/334001/chrome/browser/profiles/... chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:262: extensions::NetworkingPrivateEventRouterFactory::GetInstance(); Should this only be on if #ENABLE_EXTENSIONS is set? https://codereview.chromium.org/22295002/diff/334001/chrome/chrome_common.gypi File chrome/chrome_common.gypi (right): https://codereview.chromium.org/22295002/diff/334001/chrome/chrome_common.gyp... chrome/chrome_common.gypi:354: 'common/networking_private_messages.h', Should these be excluded if enable_extensions==0? https://codereview.chromium.org/22295002/diff/334001/chrome/chrome_dll.gypi File chrome/chrome_dll.gypi (right): https://codereview.chromium.org/22295002/diff/334001/chrome/chrome_dll.gypi#n... chrome/chrome_dll.gypi:196: 'wlanapi.dll', Why is the wlanapi delay load needed for the chrome_main_dll target as well as chrome targets? https://codereview.chromium.org/22295002/diff/334001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/22295002/diff/334001/chrome/chrome_tests.gypi... chrome/chrome_tests.gypi:486: 'target_name': 'wifi_test', Why did you need to introduce a completely new test target? Please do this only if completely necessary. You'll need to modify bots to run it, perhaps set up isolate changes, and more. If you can bundle this as part of an existing test target it will be much easier to get it deployed on continuous build bots. https://codereview.chromium.org/22295002/diff/334001/chrome/common/extensions... File chrome/common/extensions/api/_api_features.json (left): https://codereview.chromium.org/22295002/diff/334001/chrome/common/extensions... chrome/common/extensions/api/_api_features.json:394: "platform": "chromeos", Should you leave an exclusion for Linux? https://codereview.chromium.org/22295002/diff/334001/chrome/common/networking... File chrome/common/networking_private_messages.h (right): https://codereview.chromium.org/22295002/diff/334001/chrome/common/networking... chrome/common/networking_private_messages.h:83: // Reply when the utility process is done getting network properties. Nit: s/getting/setting https://codereview.chromium.org/22295002/diff/334001/chrome/common/networking... chrome/common/networking_private_messages.h:93: // Event generated when networks change is detected.. Nit: extra . at the end. Is this the list of guids which have changed, or all guids? https://codereview.chromium.org/22295002/diff/334001/chrome/common/networking... chrome/common/networking_private_messages.h:94: // |network_guids| is list of std::string objects containing network guids. Nit: I think the comment about |network_guids| is not required - pretty self-evident from the type as well as the comment on the same line. https://codereview.chromium.org/22295002/diff/334001/chrome/common/networking... chrome/common/networking_private_messages.h:98: // Event generated when list of visible networks is changed. Why does this send guids rather than a ListValue with details of all the networks (similar to GetVisibleNetworksSucceeded)? https://codereview.chromium.org/22295002/diff/334001/chrome/utility/chrome_co... File chrome/utility/chrome_content_utility_client.cc (right): https://codereview.chromium.org/22295002/diff/334001/chrome/utility/chrome_co... chrome/utility/chrome_content_utility_client.cc:86: #if defined(OS_WIN) Should this be OSX as well? https://codereview.chromium.org/22295002/diff/334001/chrome/utility/networkin... File chrome/utility/networking_private_handler.cc (right): https://codereview.chromium.org/22295002/diff/334001/chrome/utility/networkin... chrome/utility/networking_private_handler.cc:153: wifi_service_->GetVisibleNetworks( Why does this call GetVisibleNetworks followed by a scan? Is this consistent with the ChromeOS behavior? https://codereview.chromium.org/22295002/diff/334001/chrome/utility/wifi/wifi... File chrome/utility/wifi/wifi_service_test.cc (right): https://codereview.chromium.org/22295002/diff/334001/chrome/utility/wifi/wifi... chrome/utility/wifi/wifi_service_test.cc:130: wifi_service_.reset(WiFiService::CreateServiceMock()); I don't think we should be testing mock implementations which are used specifically for tests. https://codereview.chromium.org/22295002/diff/334001/chrome/utility/wifi/wifi... chrome/utility/wifi/wifi_service_test.cc:139: wifi_service_->GetVisibleNetworks( Will this work? It doesn't instantiate wifi_service_.
https://codereview.chromium.org/22295002/diff/334001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_apitest.cc (right): https://codereview.chromium.org/22295002/diff/334001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_apitest.cc:292: #if defined(OS_CHROMEOS) Does this test need to be ChromeOS only? https://codereview.chromium.org/22295002/diff/334001/chrome/common/extensions... File chrome/common/extensions/api/_api_features.json (left): https://codereview.chromium.org/22295002/diff/334001/chrome/common/extensions... chrome/common/extensions/api/_api_features.json:394: "platform": "chromeos", On 2013/10/16 12:51:36, cbentzel wrote: > Should you leave an exclusion for Linux? Actually, restrict this to only the platforms where it really works (ChromeOS and Windows at this point). https://codereview.chromium.org/22295002/diff/334001/chrome/utility/chrome_co... File chrome/utility/chrome_content_utility_client.cc (right): https://codereview.chromium.org/22295002/diff/334001/chrome/utility/chrome_co... chrome/utility/chrome_content_utility_client.cc:86: #if defined(OS_WIN) On 2013/10/16 12:51:36, cbentzel wrote: > Should this be OSX as well? Actually - it shouldn't be for run-time issues since we can't create a NetworkService. However, we can run the apitest which uses the mock WiFi Service on OSX (as well as Linux), but it's dangerous/crashy for us to have the hooks in place where the utility process can do NULL pointer deref assuming that it calls this. https://codereview.chromium.org/22295002/diff/334001/chrome/utility/wifi/wifi... File chrome/utility/wifi/wifi_service.h (right): https://codereview.chromium.org/22295002/diff/334001/chrome/utility/wifi/wifi... chrome/utility/wifi/wifi_service.h:46: int32 signal_strength; uint?
A few more details. I think the big thing that needs to be addressed is that we should only be compiling the files and running the tests related to networkingPrivate API on the platforms which actually support it. At this point it's ChromeOS and Windows only. I don't like the solution of instantiating the mock class for the WifiService on the others, and we shouldn't be spending time compiling code and running tests on those platforms when the feature won't be enabled yet. https://codereview.chromium.org/22295002/diff/334001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/334001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:100: process_client->ShutdownProcessClient(); I think we should only shutdown if there are no event handlers and there are no outstanding API calls - this only catches the case of transitioning to no event handlers. https://codereview.chromium.org/22295002/diff/334001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/334001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:385: if (!message_callbacks->set_properties_callback.is_null()) On 2013/10/15 21:06:52, tbarzic wrote: > you don't need if when you have DCHECK I think it's safer to do the runtime null-check code (and remove the DCHECK) - not really any security guarantees for IPC so it may have an invalid message_id or one not associated with set_properties_callback. So - I'd just remove the DHCECK's. https://codereview.chromium.org/22295002/diff/334001/chrome/chrome_tests_unit... File chrome/chrome_tests_unit.gypi (right): https://codereview.chromium.org/22295002/diff/334001/chrome/chrome_tests_unit... chrome/chrome_tests_unit.gypi:2419: ['OS=="win" or OS=="mac"', { How does this work on mac? It doesn't even look like we can create a non-mock wifi service?
Hi Toni, PTAL. I think I've fixed issues with lifetime of NetworkingPrivateProcessClient and addressed some other comments. I'll cleanup gyps and split-off windows-specific implementation tomorrow. thanks, -m https://codereview.chromium.org/22295002/diff/248001/chrome/common/extensions... File chrome/common/extensions/api/_api_features.json (left): https://codereview.chromium.org/22295002/diff/248001/chrome/common/extensions... chrome/common/extensions/api/_api_features.json:358: "platform": "chromeos", On 2013/10/12 02:14:09, tbarzic wrote: > On 2013/10/11 22:16:29, mef wrote: > > On 2013/10/10 20:58:39, tbarzic wrote: > > > On 2013/10/08 21:46:26, mef wrote: > > > > On 2013/10/03 20:19:00, tbarzic wrote: > > > > > can we specify multiple platforms here? > > > > > it would be nice to enable the api only where's implemented > > > > > > > > AFAIU currently "platform" could only specify "chromeos" to limit it to > > Chrome > > > > OS, no other platforms are recognized. > > > > > > ok, we could add "platforms": ["windows", "chromeos"] to > > > c/c/e/api/networking_private.json > > > It would be nice to keep the API detection by checking > > > !!chrome.networkingPrivate > > > > > > > > Fair point. I'll add other platforms handling as separate CL. > > sg Done. https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_event_router_chromeos.cc (right): https://codereview.chromium.org/22295002/diff/266001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_event_router_chromeos.cc:118: chromeos::NetworkHandler::Get()->network_state_handler()->AddObserver( On 2013/10/10 18:55:09, stevenjb wrote: > If you add 'using chromeos::NetworkHandler', etc, at the top of this .cc file we > can avoid the extra diffs and keep the code shoreter. Done. https://codereview.chromium.org/22295002/diff/331001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/331001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:27: On 2013/10/15 21:06:52, tbarzic wrote: > remove the lines Done. https://codereview.chromium.org/22295002/diff/334001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_apitest.cc (right): https://codereview.chromium.org/22295002/diff/334001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_apitest.cc:292: #if defined(OS_CHROMEOS) On 2013/10/16 13:09:26, cbentzel wrote: > Does this test need to be ChromeOS only? I believe so. There is no 'CreateNetwork' implementation for non-ChromeOS. https://codereview.chromium.org/22295002/diff/334001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/334001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:100: process_client->ShutdownProcessClient(); On 2013/10/16 14:17:40, cbentzel wrote: > I think we should only shutdown if there are no event handlers and there are no > outstanding API calls - this only catches the case of transitioning to no event > handlers. Done. https://codereview.chromium.org/22295002/diff/334001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/334001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:385: if (!message_callbacks->set_properties_callback.is_null()) On 2013/10/16 14:17:40, cbentzel wrote: > On 2013/10/15 21:06:52, tbarzic wrote: > > you don't need if when you have DCHECK > > I think it's safer to do the runtime null-check code (and remove the DCHECK) - > not really any security guarantees for IPC so it may have an invalid message_id > or one not associated with set_properties_callback. > > So - I'd just remove the DHCECK's. Done. https://codereview.chromium.org/22295002/diff/334001/chrome/browser/profiles/... File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc (right): https://codereview.chromium.org/22295002/diff/334001/chrome/browser/profiles/... chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:262: extensions::NetworkingPrivateEventRouterFactory::GetInstance(); On 2013/10/16 12:51:36, cbentzel wrote: > Should this only be on if #ENABLE_EXTENSIONS is set? Done. https://codereview.chromium.org/22295002/diff/334001/chrome/chrome_tests.gypi File chrome/chrome_tests.gypi (right): https://codereview.chromium.org/22295002/diff/334001/chrome/chrome_tests.gypi... chrome/chrome_tests.gypi:486: 'target_name': 'wifi_test', On 2013/10/16 12:51:36, cbentzel wrote: > Why did you need to introduce a completely new test target? Please do this only > if completely necessary. You'll need to modify bots to run it, perhaps set up > isolate changes, and more. If you can bundle this as part of an existing test > target it will be much easier to get it deployed on continuous build bots. This is a test utility to manually exercise the WiFi Service interface (get list of WiFi networks, connect, disconnect). It doesn't have to be there. https://codereview.chromium.org/22295002/diff/334001/chrome/common/extensions... File chrome/common/extensions/api/_api_features.json (left): https://codereview.chromium.org/22295002/diff/334001/chrome/common/extensions... chrome/common/extensions/api/_api_features.json:394: "platform": "chromeos", On 2013/10/16 13:09:26, cbentzel wrote: > On 2013/10/16 12:51:36, cbentzel wrote: > > Should you leave an exclusion for Linux? > > Actually, restrict this to only the platforms where it really works (ChromeOS > and Windows at this point). Done. https://codereview.chromium.org/22295002/diff/334001/chrome/common/networking... File chrome/common/networking_private_messages.h (right): https://codereview.chromium.org/22295002/diff/334001/chrome/common/networking... chrome/common/networking_private_messages.h:83: // Reply when the utility process is done getting network properties. On 2013/10/16 12:51:36, cbentzel wrote: > Nit: s/getting/setting Done. https://codereview.chromium.org/22295002/diff/334001/chrome/common/networking... chrome/common/networking_private_messages.h:93: // Event generated when networks change is detected.. On 2013/10/16 12:51:36, cbentzel wrote: > Nit: extra . at the end. > > Is this the list of guids which have changed, or all guids? Done. Just the changed networks (comment updated). https://codereview.chromium.org/22295002/diff/334001/chrome/common/networking... chrome/common/networking_private_messages.h:98: // Event generated when list of visible networks is changed. On 2013/10/16 12:51:36, cbentzel wrote: > Why does this send guids rather than a ListValue with details of all the > networks (similar to GetVisibleNetworksSucceeded)? Good question. The NetworkListChanged event only needs guids, but there is no hard reason to limit this message. https://codereview.chromium.org/22295002/diff/334001/chrome/utility/chrome_co... File chrome/utility/chrome_content_utility_client.cc (right): https://codereview.chromium.org/22295002/diff/334001/chrome/utility/chrome_co... chrome/utility/chrome_content_utility_client.cc:86: #if defined(OS_WIN) On 2013/10/16 13:09:26, cbentzel wrote: > On 2013/10/16 12:51:36, cbentzel wrote: > > Should this be OSX as well? > > Actually - it shouldn't be for run-time issues since we can't create a > NetworkService. > > However, we can run the apitest which uses the mock WiFi Service on OSX (as well > as Linux), but it's dangerous/crashy for us to have the hooks in place where the > utility process can do NULL pointer deref assuming that it calls this. So, I leave it OS_WIN only for now, right? https://codereview.chromium.org/22295002/diff/334001/chrome/utility/wifi/wifi... File chrome/utility/wifi/wifi_service.h (right): https://codereview.chromium.org/22295002/diff/334001/chrome/utility/wifi/wifi... chrome/utility/wifi/wifi_service.h:46: int32 signal_strength; On 2013/10/16 13:09:26, cbentzel wrote: > uint? Done.
https://codereview.chromium.org/22295002/diff/357001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/357001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:149: void NetworkingPrivateProcessClient::HaveEventsListeners( Nit: SetHaveEventListeners() https://codereview.chromium.org/22295002/diff/357001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:370: ShutdownIfDone(); I'd move the callbacks_map_.Remove(message_id) and ShutdownIfDone() into a helper function.
Hi Chris, thanks for your latest comments, I've addressed them! I'll split off wifi_service_win and fix gyps today. thanks, -m https://codereview.chromium.org/22295002/diff/357001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/357001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:149: void NetworkingPrivateProcessClient::HaveEventsListeners( On 2013/10/17 09:46:34, cbentzel wrote: > Nit: SetHaveEventListeners() Done. https://codereview.chromium.org/22295002/diff/357001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:370: ShutdownIfDone(); On 2013/10/17 09:46:34, cbentzel wrote: > I'd move the callbacks_map_.Remove(message_id) and ShutdownIfDone() into a > helper function. Done.
https://codereview.chromium.org/22295002/diff/357001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/357001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:155: profile_->RemoveUserData(kNetworkingPrivateProcessClient); shouldn't this be in ShutdownIfDone? https://codereview.chromium.org/22295002/diff/357001/chrome/browser/profiles/... File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc (right): https://codereview.chromium.org/22295002/diff/357001/chrome/browser/profiles/... chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:262: #if defined(ENABLE_EXTENSIONS) && defined(OS_CHROMEOS) || defined(OS_WIN) you don't need ENABLE_EXTENSIONS here (the block already is under if defined(ENABLE_EXTENSIONS; see line 210) https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:243: for (base::ListValue::const_iterator it = network_list.begin(); can the filtering be done in the wifi service (sorry if you've already answered this question)? https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:43: class CryptoVerifyMock : public NetworkingPrivateProcessClient::CryptoVerify { I'd prefer having this class defined in the test than here. https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:72: return crypto.VerifyCredentials(properties.certificate, how expensive is this? Is it ok to call it from the UI thread? https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:391: api::networking_private::OnNetworkListChanged::Create(network_guids)); I still think this belongs to NetworkingPrivateEventRouter (instead of setting have_event_listener, the event router should set itself to listen to the ProcessClient (which should have an observe interface)) https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.h (right): https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.h:101: GetProcessClientForProfile(Profile* profile); I'd rename this to GetForProfile (or even Get) it's clear what's you getting when you call NetworkingPrivateProcessClient::GetForProfile
This generally looks good now - thanks for cleaning up the build rules and for isolating out the actual WiFi implementation on Windows to a separate CL. https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:37: scoped_refptr<NetworkingPrivateProcessClient> process_client( Note to self: convince yourself that lifetime works correctly. https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:41: params->network_guid, // service path Nit: why are these mentioning // service path? Can you just get rid of those comments? https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:58: scoped_ptr<base::DictionaryValue> error_data) { These error dictionaries seem to never be populated or used. Can you get rid of them in the plumbing? https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc (left): https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc:12: // Based on chromeos_public//.../network_DestinationVerification.py Nit: Why did you get rid of this comment?
Hi, thanks for your comments! I've addressed some of them. Remaining TODO: - Add event router observer interface, - Move CryptoVerifyMock to apitest, - Perform CryptoVerify on background thread, - Move network type filtering into Handler or WiFiService. thanks, -m https://codereview.chromium.org/22295002/diff/357001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/357001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:155: profile_->RemoveUserData(kNetworkingPrivateProcessClient); On 2013/10/17 19:30:21, tbarzic wrote: > shouldn't this be in ShutdownIfDone? Done. https://codereview.chromium.org/22295002/diff/357001/chrome/browser/profiles/... File chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc (right): https://codereview.chromium.org/22295002/diff/357001/chrome/browser/profiles/... chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc:262: #if defined(ENABLE_EXTENSIONS) && defined(OS_CHROMEOS) || defined(OS_WIN) On 2013/10/17 19:30:21, tbarzic wrote: > you don't need ENABLE_EXTENSIONS here (the block already is under if > defined(ENABLE_EXTENSIONS; see line 210) Done. https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:41: params->network_guid, // service path On 2013/10/17 21:10:11, cbentzel wrote: > Nit: why are these mentioning // service path? > > Can you just get rid of those comments? Done. https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:58: scoped_ptr<base::DictionaryValue> error_data) { On 2013/10/17 21:10:11, cbentzel wrote: > These error dictionaries seem to never be populated or used. Can you get rid of > them in the plumbing? Well, the NetworkingPrivateXXXFunction declarations are shared between _chromeos and _nonchromeos, so this would break. They are not currently used by the extension though. https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:243: for (base::ListValue::const_iterator it = network_list.begin(); On 2013/10/17 19:30:22, tbarzic wrote: > can the filtering be done in the wifi service (sorry if you've already answered > this question)? Yes it can. Will do. https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc (left): https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc:12: // Based on chromeos_public//.../network_DestinationVerification.py On 2013/10/17 21:10:11, cbentzel wrote: > Nit: Why did you get rid of this comment? Based on sleevi@'s suggestion in original CL review: https://codereview.chromium.org/23710003/diff/39001/chrome/browser/extensions... https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:72: return crypto.VerifyCredentials(properties.certificate, On 2013/10/17 19:30:22, tbarzic wrote: > how expensive is this? Is it ok to call it from the UI thread? It is verifying one RSA signature from hard-coded certificate. It is not that expensive, but it is not free either. I don't have a problem with doing it on a background thread if it is safer. https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:391: api::networking_private::OnNetworkListChanged::Create(network_guids)); On 2013/10/17 19:30:22, tbarzic wrote: > I still think this belongs to NetworkingPrivateEventRouter (instead of setting > have_event_listener, the event router should set itself to listen to the > ProcessClient (which should have an observe interface)) Sounds good. Will do. https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.h (right): https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.h:101: GetProcessClientForProfile(Profile* profile); On 2013/10/17 19:30:22, tbarzic wrote: > I'd rename this to GetForProfile (or even Get) > it's clear what's you getting when you call > NetworkingPrivateProcessClient::GetForProfile Done.
Are you planning to add the TODO items to this CL? If so, given the size and history of this CL, is there any way to split it apart into two or more pieces, so that we can a) look at smaller sets of changes b) look at this with fresh eyes (and with less history - codreview isn't the fastest thing ever even under the best circumstances) Specifically, maybe pull out the changes to networking_privateapi.h and networking_private_event_router.h, and the WS/ordering changes into a separate CL, since these aren't directly related to the Windows changes? Thanks, -Steven On Thu, Oct 17, 2013 at 3:03 PM, <mef@chromium.org> wrote: > Hi, thanks for your comments! I've addressed some of them. > > Remaining TODO: > - Add event router observer interface, > - Move CryptoVerifyMock to apitest, > - Perform CryptoVerify on background thread, > - Move network type filtering into Handler or WiFiService. > > thanks, > -m > > > > https://codereview.chromium.**org/22295002/diff/357001/** > chrome/browser/extensions/api/**networking_private/networking_** > private_process_client.cc<https://codereview.chromium.org/22295002/diff/357001/chrome/browser/extensions/api/networking_private/networking_private_process_client.cc> > File > chrome/browser/extensions/api/**networking_private/networking_** > private_process_client.cc > (right): > > https://codereview.chromium.**org/22295002/diff/357001/** > chrome/browser/extensions/api/**networking_private/networking_** > private_process_client.cc#**newcode155<https://codereview.chromium.org/22295002/diff/357001/chrome/browser/extensions/api/networking_private/networking_private_process_client.cc#newcode155> > chrome/browser/extensions/api/**networking_private/networking_** > private_process_client.cc:155: > profile_->RemoveUserData(**kNetworkingPrivateProcessClien**t); > On 2013/10/17 19:30:21, tbarzic wrote: > >> shouldn't this be in ShutdownIfDone? >> > > Done. > > > https://codereview.chromium.**org/22295002/diff/357001/** > chrome/browser/profiles/**chrome_browser_main_extra_**parts_profiles.cc<https://codereview.chromium.org/22295002/diff/357001/chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc> > File chrome/browser/profiles/**chrome_browser_main_extra_** > parts_profiles.cc > (right): > > https://codereview.chromium.**org/22295002/diff/357001/** > chrome/browser/profiles/**chrome_browser_main_extra_** > parts_profiles.cc#newcode262<https://codereview.chromium.org/22295002/diff/357001/chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc#newcode262> > chrome/browser/profiles/**chrome_browser_main_extra_** > parts_profiles.cc:262: > #if defined(ENABLE_EXTENSIONS) && defined(OS_CHROMEOS) || > defined(OS_WIN) > On 2013/10/17 19:30:21, tbarzic wrote: > >> you don't need ENABLE_EXTENSIONS here (the block already is under if >> defined(ENABLE_EXTENSIONS; see line 210) >> > > Done. > > > https://codereview.chromium.**org/22295002/diff/381001/** > chrome/browser/extensions/api/**networking_private/networking_** > private_api_nonchromeos.cc<https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc> > File > chrome/browser/extensions/api/**networking_private/networking_** > private_api_nonchromeos.cc > (right): > > https://codereview.chromium.**org/22295002/diff/381001/** > chrome/browser/extensions/api/**networking_private/networking_** > private_api_nonchromeos.cc#**newcode41<https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc#newcode41> > chrome/browser/extensions/api/**networking_private/networking_** > private_api_nonchromeos.cc:41: > params->network_guid, // service path > On 2013/10/17 21:10:11, cbentzel wrote: > >> Nit: why are these mentioning // service path? >> > > Can you just get rid of those comments? >> > > Done. > > > https://codereview.chromium.**org/22295002/diff/381001/** > chrome/browser/extensions/api/**networking_private/networking_** > private_api_nonchromeos.cc#**newcode58<https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc#newcode58> > chrome/browser/extensions/api/**networking_private/networking_** > private_api_nonchromeos.cc:58: > scoped_ptr<base::**DictionaryValue> error_data) { > On 2013/10/17 21:10:11, cbentzel wrote: > >> These error dictionaries seem to never be populated or used. Can you >> > get rid of > >> them in the plumbing? >> > Well, the NetworkingPrivateXXXFunction declarations are shared between > _chromeos and _nonchromeos, > so this would break. They are not currently used by the extension > though. > > > https://codereview.chromium.**org/22295002/diff/381001/** > chrome/browser/extensions/api/**networking_private/networking_** > private_api_nonchromeos.cc#**newcode243<https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc#newcode243> > chrome/browser/extensions/api/**networking_private/networking_** > private_api_nonchromeos.cc:**243: > for (base::ListValue::const_**iterator it = network_list.begin(); > On 2013/10/17 19:30:22, tbarzic wrote: > >> can the filtering be done in the wifi service (sorry if you've already >> > answered > >> this question)? >> > Yes it can. Will do. > > > https://codereview.chromium.**org/22295002/diff/381001/** > chrome/browser/extensions/api/**networking_private/networking_** > private_crypto_unittest.cc<https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc> > File > chrome/browser/extensions/api/**networking_private/networking_** > private_crypto_unittest.cc > (left): > > https://codereview.chromium.**org/22295002/diff/381001/** > chrome/browser/extensions/api/**networking_private/networking_** > private_crypto_unittest.cc#**oldcode12<https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc#oldcode12> > chrome/browser/extensions/api/**networking_private/networking_** > private_crypto_unittest.cc:12: > // Based on chromeos_public//.../network_**DestinationVerification.py > On 2013/10/17 21:10:11, cbentzel wrote: > >> Nit: Why did you get rid of this comment? >> > Based on sleevi@'s suggestion in original CL review: > https://codereview.chromium.**org/23710003/diff/39001/** > chrome/browser/extensions/api/**networking_private/networking_** > private_crypto_unittest.cc#**newcode12<https://codereview.chromium.org/23710003/diff/39001/chrome/browser/extensions/api/networking_private/networking_private_crypto_unittest.cc#newcode12> > > > https://codereview.chromium.**org/22295002/diff/381001/** > chrome/browser/extensions/api/**networking_private/networking_** > private_process_client.cc<https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extensions/api/networking_private/networking_private_process_client.cc> > File > chrome/browser/extensions/api/**networking_private/networking_** > private_process_client.cc > (right): > > https://codereview.chromium.**org/22295002/diff/381001/** > chrome/browser/extensions/api/**networking_private/networking_** > private_process_client.cc#**newcode72<https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extensions/api/networking_private/networking_private_process_client.cc#newcode72> > chrome/browser/extensions/api/**networking_private/networking_** > private_process_client.cc:72: > return crypto.VerifyCredentials(**properties.certificate, > On 2013/10/17 19:30:22, tbarzic wrote: > >> how expensive is this? Is it ok to call it from the UI thread? >> > It is verifying one RSA signature from hard-coded certificate. > It is not that expensive, but it is not free either. > I don't have a problem with doing it on a background thread if it is > safer. > > > https://codereview.chromium.**org/22295002/diff/381001/** > chrome/browser/extensions/api/**networking_private/networking_** > private_process_client.cc#**newcode391<https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extensions/api/networking_private/networking_private_process_client.cc#newcode391> > chrome/browser/extensions/api/**networking_private/networking_** > private_process_client.cc:391: > api::networking_private::**OnNetworkListChanged::Create(**network_guids)); > On 2013/10/17 19:30:22, tbarzic wrote: > >> I still think this belongs to NetworkingPrivateEventRouter (instead of >> > setting > >> have_event_listener, the event router should set itself to listen to >> > the > >> ProcessClient (which should have an observe interface)) >> > Sounds good. Will do. > > > https://codereview.chromium.**org/22295002/diff/381001/** > chrome/browser/extensions/api/**networking_private/networking_** > private_process_client.h<https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extensions/api/networking_private/networking_private_process_client.h> > File > chrome/browser/extensions/api/**networking_private/networking_** > private_process_client.h > (right): > > https://codereview.chromium.**org/22295002/diff/381001/** > chrome/browser/extensions/api/**networking_private/networking_** > private_process_client.h#**newcode101<https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extensions/api/networking_private/networking_private_process_client.h#newcode101> > chrome/browser/extensions/api/**networking_private/networking_** > private_process_client.h:101: > GetProcessClientForProfile(**Profile* profile); > On 2013/10/17 19:30:22, tbarzic wrote: > >> I'd rename this to GetForProfile (or even Get) >> it's clear what's you getting when you call >> NetworkingPrivateProcessClient**::GetForProfile >> > > Done. > > https://codereview.chromium.**org/22295002/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/10/17 22:14:20, stevenjb wrote: > Are you planning to add the TODO items to this CL? > > If so, given the size and history of this CL, is there any way to split it > apart into two or more pieces, so that we can > a) look at smaller sets of changes > b) look at this with fresh eyes (and with less history - codreview isn't > the fastest thing ever even under the best circumstances) > > Specifically, maybe pull out the changes to networking_privateapi.h > and networking_private_event_router.h, and the WS/ordering changes into a > separate CL, since these aren't directly related to the Windows changes? Yes, Windows implementation is now separated into http://crrev.com/27722003. The reset of WiFiService/NetworkingPrivateHandler/NetworkingPrivateMessages/NetworkingPrivateProcessClient are parts of the same plumbing, which gets tested via networking_private_apitest, so I'd argue that they should remain together. thanks, -m
Hi Toni, PTAL. I've addressed your comments except for moving network type filtering into NetworkingPrivateHandler (which I'm doing right now). thanks, -m https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:43: class CryptoVerifyMock : public NetworkingPrivateProcessClient::CryptoVerify { On 2013/10/17 19:30:22, tbarzic wrote: > I'd prefer having this class defined in the test than here. Done. https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:72: return crypto.VerifyCredentials(properties.certificate, On 2013/10/17 22:03:35, mef wrote: > On 2013/10/17 19:30:22, tbarzic wrote: > > how expensive is this? Is it ok to call it from the UI thread? > It is verifying one RSA signature from hard-coded certificate. > It is not that expensive, but it is not free either. > I don't have a problem with doing it on a background thread if it is safer. Done. https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:391: api::networking_private::OnNetworkListChanged::Create(network_guids)); On 2013/10/17 22:03:35, mef wrote: > On 2013/10/17 19:30:22, tbarzic wrote: > > I still think this belongs to NetworkingPrivateEventRouter (instead of setting > > have_event_listener, the event router should set itself to listen to the > > ProcessClient (which should have an observe interface)) > Sounds good. Will do. Done.
Personally I'd prefer moving the filtering to a separate cl. Will be easier to review since this one is so big. On Oct 18, 2013 4:18 PM, <mef@chromium.org> wrote: > Hi Toni, PTAL. > > I've addressed your comments except for moving network type filtering into > NetworkingPrivateHandler (which I'm doing right now). > > thanks, > -m > > > > > https://codereview.chromium.**org/22295002/diff/381001/** > chrome/browser/extensions/api/**networking_private/networking_** > private_process_client.cc<https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extensions/api/networking_private/networking_private_process_client.cc> > File > chrome/browser/extensions/api/**networking_private/networking_** > private_process_client.cc > (right): > > https://codereview.chromium.**org/22295002/diff/381001/** > chrome/browser/extensions/api/**networking_private/networking_** > private_process_client.cc#**newcode43<https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extensions/api/networking_private/networking_private_process_client.cc#newcode43> > chrome/browser/extensions/api/**networking_private/networking_** > private_process_client.cc:43: > class CryptoVerifyMock : public > NetworkingPrivateProcessClient**::CryptoVerify { > On 2013/10/17 19:30:22, tbarzic wrote: > >> I'd prefer having this class defined in the test than here. >> > > Done. > > https://codereview.chromium.**org/22295002/diff/381001/** > chrome/browser/extensions/api/**networking_private/networking_** > private_process_client.cc#**newcode72<https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extensions/api/networking_private/networking_private_process_client.cc#newcode72> > chrome/browser/extensions/api/**networking_private/networking_** > private_process_client.cc:72: > return crypto.VerifyCredentials(**properties.certificate, > On 2013/10/17 22:03:35, mef wrote: > >> On 2013/10/17 19:30:22, tbarzic wrote: >> > how expensive is this? Is it ok to call it from the UI thread? >> It is verifying one RSA signature from hard-coded certificate. >> It is not that expensive, but it is not free either. >> I don't have a problem with doing it on a background thread if it is >> > safer. > > Done. > > https://codereview.chromium.**org/22295002/diff/381001/** > chrome/browser/extensions/api/**networking_private/networking_** > private_process_client.cc#**newcode391<https://codereview.chromium.org/22295002/diff/381001/chrome/browser/extensions/api/networking_private/networking_private_process_client.cc#newcode391> > chrome/browser/extensions/api/**networking_private/networking_** > private_process_client.cc:391: > api::networking_private::**OnNetworkListChanged::Create(**network_guids)); > On 2013/10/17 22:03:35, mef wrote: > >> On 2013/10/17 19:30:22, tbarzic wrote: >> > I still think this belongs to NetworkingPrivateEventRouter (instead >> > of setting > >> > have_event_listener, the event router should set itself to listen to >> > the > >> > ProcessClient (which should have an observe interface)) >> Sounds good. Will do. >> > > Done. > > https://codereview.chromium.**org/22295002/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
btw. moving visible networks filtering to another cl (as suggested by cbentzel) sound really good.. https://codereview.chromium.org/22295002/diff/423001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_apitest.cc (right): https://codereview.chromium.org/22295002/diff/423001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_apitest.cc:21: #if defined(ENABLE_EXTENSIONS) && defined(OS_CHROMEOS) || defined(OS_WIN) you should exclude the file in gypi file instead of ifdefing the whole file https://codereview.chromium.org/22295002/diff/423001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_apitest.cc:36: #endif // OS_CHROMEOS add a blank line here https://codereview.chromium.org/22295002/diff/423001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_apitest.cc:42: namespace extensions { while you're here, put everything in an anonymous namespace (if possible) https://codereview.chromium.org/22295002/diff/423001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_apitest.cc:234: process_client_->UseMocks(mock_parameters, new CryptoVerifyMock()); I'd rename UseMocks to SetupForTest and add logic that keeps the client alive there instead of calling AddMessageCallback from here (or maybe add a test observer here). Also, you should ensure that the process client gets properly shutdown (which is currently not the case) in TearDowmOnMainThread https://codereview.chromium.org/22295002/diff/423001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_apitest.cc:242: class CryptoVerifyMock : public NetworkingPrivateProcessClient::CryptoVerify { move this to anonymous namespace https://codereview.chromium.org/22295002/diff/423001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/423001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:82: listening_ = false; stop observing NetworkingPrivateProcessClient (if listening) https://codereview.chromium.org/22295002/diff/423001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/423001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:246: message_callbacks->id, service_path, *properties_ptr.Pass())); s/*properties_ptr.Pass()/*properties_ptr/ (in case you missed my previous comment about this) https://codereview.chromium.org/22295002/diff/423001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.h (right): https://codereview.chromium.org/22295002/diff/423001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.h:242: class NetworkingPrivateProcessClient::NetworkEventsObserver { I'd rename this to Observer (and inline it inside NetworkingPrivateProcessClient)
https://codereview.chromium.org/22295002/diff/423001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/423001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:71: NetworkingPrivateProcessClient::GetForProfile(Profile* profile) { one thing I'm concerned about is the behaviour on shutdown. specifically, the case where there are pending API calls when the profile gets destroyed. In that case, Shutdown will never be called (as callbacks_map_ will not be emptied before the UI thread is stopped) Two things worry me: 1. EndBatchMode() will not be called on the process utility 2. ExtensionFunctions referenced inside pending callback will probably be leaked. So, we'll have to force Shutdown when the profile is (being) destroyed. One place for that could be from NetworkingPrivateEventRouter::Shutdown (as the NetworkingPrivateEventRouter is BrowserContextKeyedService, this is called on profile destruction), but this somehow feels a bit dirty. maybe a cleaner solution would be to make this a BrowserContextKeyedService too. With this approach, we'd have to change the way the process client is shutdown, as I'm not sure that we'd be able to change the ProcessClient instance once it's been created (e.g. we could just clear callbacks and stop the utility process; and later, if needed, restart the process in NetworkingPrivateProcessClient::Get()). Though, I'd leave this for a separate patch (if you choose to go this way).
Toni, thanks for your comments! I'll think them through and will try to address over the weekend. thanks, -m On 2013/10/18 22:11:53, tbarzic wrote: > https://codereview.chromium.org/22295002/diff/423001/chrome/browser/extension... > File > chrome/browser/extensions/api/networking_private/networking_private_process_client.cc > (right): > > https://codereview.chromium.org/22295002/diff/423001/chrome/browser/extension... > chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:71: > NetworkingPrivateProcessClient::GetForProfile(Profile* profile) { > one thing I'm concerned about is the behaviour on shutdown. > specifically, the case where there are pending API calls when the profile gets > destroyed. In that case, Shutdown will never be called (as callbacks_map_ will > not be emptied before the UI thread is stopped) > Two things worry me: > 1. EndBatchMode() will not be called on the process utility > 2. ExtensionFunctions referenced inside pending callback will probably be > leaked. > > So, we'll have to force Shutdown when the profile is (being) destroyed. One > place for that could be from NetworkingPrivateEventRouter::Shutdown (as the > NetworkingPrivateEventRouter is BrowserContextKeyedService, this is called on > profile destruction), but this somehow feels a bit dirty. > > maybe a cleaner solution would be to make this a BrowserContextKeyedService too. > With this approach, we'd have to change the way the process client is shutdown, > as I'm not sure that we'd be able to change the ProcessClient instance once it's > been created (e.g. we could just clear callbacks and stop the utility process; > and later, if needed, restart the process in > NetworkingPrivateProcessClient::Get()). Though, I'd leave this for a separate > patch (if you choose to go this way).
Hi, PTAL. Also the network filtering move is in http://crrev.com/30753002. thanks, -m https://codereview.chromium.org/22295002/diff/423001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_apitest.cc (right): https://codereview.chromium.org/22295002/diff/423001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_apitest.cc:36: #endif // OS_CHROMEOS On 2013/10/18 21:28:03, tbarzic wrote: > add a blank line here Done. https://codereview.chromium.org/22295002/diff/423001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_apitest.cc:42: namespace extensions { On 2013/10/18 21:28:03, tbarzic wrote: > while you're here, put everything in an anonymous namespace (if possible) Done. https://codereview.chromium.org/22295002/diff/423001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_apitest.cc:234: process_client_->UseMocks(mock_parameters, new CryptoVerifyMock()); On 2013/10/18 21:28:03, tbarzic wrote: > I'd rename UseMocks to SetupForTest and add logic that keeps the client alive > there instead of calling AddMessageCallback from here (or maybe add a test > observer here). > > Also, you should ensure that the process client gets properly shutdown (which is > currently not the case) in TearDowmOnMainThread Done. https://codereview.chromium.org/22295002/diff/423001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_apitest.cc:242: class CryptoVerifyMock : public NetworkingPrivateProcessClient::CryptoVerify { On 2013/10/18 21:28:03, tbarzic wrote: > move this to anonymous namespace Done. https://codereview.chromium.org/22295002/diff/423001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/423001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:82: listening_ = false; On 2013/10/18 21:28:03, tbarzic wrote: > stop observing NetworkingPrivateProcessClient (if listening) Done. https://codereview.chromium.org/22295002/diff/423001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/423001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:246: message_callbacks->id, service_path, *properties_ptr.Pass())); On 2013/10/18 21:28:03, tbarzic wrote: > s/*properties_ptr.Pass()/*properties_ptr/ > > (in case you missed my previous comment about this) Done. https://codereview.chromium.org/22295002/diff/423001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.h (right): https://codereview.chromium.org/22295002/diff/423001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.h:242: class NetworkingPrivateProcessClient::NetworkEventsObserver { On 2013/10/18 21:28:03, tbarzic wrote: > I'd rename this to Observer (and inline it inside > NetworkingPrivateProcessClient) Done.
chromeos bits LGTM https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:193: } nit: order Result / Error callbacks consistency with GetProperties https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_apitest.cc (right): https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_apitest.cc:259: // Use Mocks WiFiService to test plumbing. nit: s/Mocks/Mock
This mostly looks good. I sent some possible cleanup comments but please address those in followup CLs - this is nearly there and I don't want to slog through a big patch again. The only two issues I'd like to see addressed are the potential lifetime issue in ShutdownOnIOThread and the memory leak if the process crashes. https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:326: params->network_guid, // service path Nit: remove // service path comment. https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:133: BrowserThread::PostTask( You should do the PostTask before RemoveUserData - wouldn't the RemoveUserData automatically drop refcount to 0 for this object (and delete it)? Alternately, you can use a stack allocated scoped_refptr<> to ensure that |this} is alive through the entire function. https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:180: Start(); BUG: You'll need to reset utility_process_host_ in this case - you may be able to get by with just doing it inside of StartOnIOThread, or you'll need to add a RestartOnIOThread method. https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:275: base::WorkerPool::PostTask( Could this use PostTaskAndReply rather than depend on the worker thread to PostTask to UI thread when done? https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:323: if (verified) Nit: I'd add braces here since these span multiple lines. https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.h (right): https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.h:121: void VerifyDestination( Note: It's also a little strange to have VerifyDestination and VerifyAndEncryptData be part of this class's interface, as they don't actually involve communication to the utility process. Don't change it for this CL, but consider moving these out to a separate class which the networking_private_api_nonchromeos uses. https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.h:163: scoped_refptr<NetworkingPrivateProcessClient> process_client; I don't think these need a refcounted pointer back to the NetworkingPrivateProcessClient, since ultimately this is released when the ShutdownIfDone is called. That happens when the map is empty, but shouldn't happen while there are outstanding calls being made. https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.h:168: void Start(); Nit: Move this down near StartProcessOnIOThread declaration. https://codereview.chromium.org/22295002/diff/613001/chrome/utility/networkin... File chrome/utility/networking_private_handler.h (right): https://codereview.chromium.org/22295002/diff/613001/chrome/utility/networkin... chrome/utility/networking_private_handler.h:55: void OnGetPropertiesSucceeded( Nit: Some of the naming is a little confusing - you use the On prefix both for IPC message handlers as well as for callbacks from the WiFiService Perhaps use a different prefix for the WiFiService case. I am also fine with waiting for this in a followup CL.
https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:301: void NetworkingPrivateStartConnectFunction::ConnectionStartSuccess() { nit: the naming for callbacks is a bit inconsistent in the file: some are named ErrorCallback/ResultCallback, and some <FunctionName>Success/<FunctionName>Failed (you don't have to fix this now, though) https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_event_router_nonchromeos.cc:20: NetworkingPrivateProcessClient::Observer { public NetworkingPrivateProcessClient::Observer? https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:129: && callbacks_map_.IsEmpty()) { nit: && to the line before https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:180: Start(); On 2013/10/21 21:07:16, cbentzel wrote: > BUG: You'll need to reset utility_process_host_ in this case - you may be able > to get by with just doing it inside of StartOnIOThread, or you'll need to add a > RestartOnIOThread method. are you sure about this? shouldn't the utility process host delete itself on crash (and invalidate utility_process_host_ which is a weak ptr on IO thread)? https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:359: callbacks_map_.Clear(); ShutdownIfDone(); I would probably be nicer to have a separate method for cleanup in tests (maybe ResetForTest) than to do it by passing NULL to this method. (ok to do this later) https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.h (right): https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.h:122: scoped_ptr<base::ListValue> args, nit: can this be moved to the same line as VerifyDestination https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.h:127: scoped_ptr<base::ListValue> args, ditto
Hi, I'm a little bit confused about the workflow, but I've created another CL (https://codereview.chromium.org/34013002/), which addresses your comments. Another portion (move of network type filtering) is in https://codereview.chromium.org/30753002. Once approved should those be landed sequentially or merged before landing? thanks, -m https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:193: } On 2013/10/21 17:58:54, stevenjb wrote: > nit: order Result / Error callbacks consistency with GetProperties Done. https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:301: void NetworkingPrivateStartConnectFunction::ConnectionStartSuccess() { On 2013/10/21 21:54:07, tbarzic wrote: > nit: the naming for callbacks is a bit inconsistent in the file: some are named > ErrorCallback/ResultCallback, and some > <FunctionName>Success/<FunctionName>Failed > (you don't have to fix this now, though) I agree. I didn't name them, just took what was there in .h for Chromeos, but will rename them later for consistency. https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:326: params->network_guid, // service path On 2013/10/21 21:07:16, cbentzel wrote: > Nit: remove // service path comment. Done. https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:129: && callbacks_map_.IsEmpty()) { On 2013/10/21 21:54:07, tbarzic wrote: > nit: && to the line before Done. https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:133: BrowserThread::PostTask( On 2013/10/21 21:07:16, cbentzel wrote: > You should do the PostTask before RemoveUserData - wouldn't the RemoveUserData > automatically drop refcount to 0 for this object (and delete it)? > > Alternately, you can use a stack allocated scoped_refptr<> to ensure that |this} > is alive through the entire function. Done. https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:180: Start(); On 2013/10/21 21:54:07, tbarzic wrote: > On 2013/10/21 21:07:16, cbentzel wrote: > > BUG: You'll need to reset utility_process_host_ in this case - you may be able > > to get by with just doing it inside of StartOnIOThread, or you'll need to add > a > > RestartOnIOThread method. > > are you sure about this? shouldn't the utility process host delete itself on > crash (and invalidate utility_process_host_ which is a weak ptr on IO thread)? Done. AFAIU extra reset() here doesn't hurt anything. https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:275: base::WorkerPool::PostTask( On 2013/10/21 21:07:16, cbentzel wrote: > Could this use PostTaskAndReply rather than depend on the worker thread to > PostTask to UI thread when done? It could, but wouldn't it be inconsistent with VerifyAndEncryptData as that one has to choose between callback and error_callback? I could also add another method to handle reply and call either callback or error_callback, but this seems a bit overcomplicated. Is this to avoid hard-coding UI thread for callbacks? https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:323: if (verified) On 2013/10/21 21:07:16, cbentzel wrote: > Nit: I'd add braces here since these span multiple lines. Done. https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:359: callbacks_map_.Clear(); On 2013/10/21 21:54:07, tbarzic wrote: > ShutdownIfDone(); > > I would probably be nicer to have a separate method for cleanup in tests (maybe > ResetForTest) than to do it by passing NULL to this method. > (ok to do this later) Done. https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.h (right): https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.h:121: void VerifyDestination( On 2013/10/21 21:07:16, cbentzel wrote: > Note: It's also a little strange to have VerifyDestination and > VerifyAndEncryptData be part of this class's interface, as they don't actually > involve communication to the utility process. Don't change it for this CL, but > consider moving these out to a separate class which the > networking_private_api_nonchromeos uses. Will do, especially considering that they could be used for chromeos as well. https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.h:122: scoped_ptr<base::ListValue> args, On 2013/10/21 21:54:07, tbarzic wrote: > nit: can this be moved to the same line as VerifyDestination Done. https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.h:127: scoped_ptr<base::ListValue> args, On 2013/10/21 21:54:07, tbarzic wrote: > ditto Done. https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.h:163: scoped_refptr<NetworkingPrivateProcessClient> process_client; On 2013/10/21 21:07:16, cbentzel wrote: > I don't think these need a refcounted pointer back to the > NetworkingPrivateProcessClient, since ultimately this is released when the > ShutdownIfDone is called. That happens when the map is empty, but shouldn't > happen while there are outstanding calls being made. Done. It was my attempt to use refcounting instead of explicit ShutdownWhenDone. https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.h:168: void Start(); On 2013/10/21 21:07:16, cbentzel wrote: > Nit: Move this down near StartProcessOnIOThread declaration. Done. https://codereview.chromium.org/22295002/diff/613001/chrome/utility/networkin... File chrome/utility/networking_private_handler.h (right): https://codereview.chromium.org/22295002/diff/613001/chrome/utility/networkin... chrome/utility/networking_private_handler.h:55: void OnGetPropertiesSucceeded( On 2013/10/21 21:07:16, cbentzel wrote: > Nit: Some of the naming is a little confusing - you use the On prefix both for > IPC message handlers as well as for callbacks from the WiFiService Perhaps use a > different prefix for the WiFiService case. I am also fine with waiting for this > in a followup CL. Done.
The combination of this and https://codereview.chromium.org/34013002/ looks good. I would just merge those changes in here - sorry, I thought it would lead to a bigger rewrite which was why I was hesitant to do the changes here. Thanks for being patient with the reviews! https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:180: Start(); On 2013/10/21 21:54:07, tbarzic wrote: > On 2013/10/21 21:07:16, cbentzel wrote: > > BUG: You'll need to reset utility_process_host_ in this case - you may be able > > to get by with just doing it inside of StartOnIOThread, or you'll need to add > a > > RestartOnIOThread method. > > are you sure about this? shouldn't the utility process host delete itself on > crash (and invalidate utility_process_host_ which is a weak ptr on IO thread)? Not totally sure, but I took a look through the UtilityProcessHostImpl code and couldn't see any place where it deleted itself when the child process crashed. https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ut...
https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:180: Start(); On 2013/10/22 00:57:40, cbentzel wrote: > On 2013/10/21 21:54:07, tbarzic wrote: > > On 2013/10/21 21:07:16, cbentzel wrote: > > > BUG: You'll need to reset utility_process_host_ in this case - you may be > able > > > to get by with just doing it inside of StartOnIOThread, or you'll need to > add > > a > > > RestartOnIOThread method. > > > > are you sure about this? shouldn't the utility process host delete itself on > > crash (and invalidate utility_process_host_ which is a weak ptr on IO thread)? > > Not totally sure, but I took a look through the UtilityProcessHostImpl code and > couldn't see any place where it deleted itself when the child process crashed. > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/ut... > I think the host is deleted synchronously after UtilityProcessHostImpl::OnProcessCrashed is called (which posts this task to the UI thread): https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br... (BrowserChildProcessHost is created on utility process start and the UtilityProcessHostImpl is its delegate) hm, I guess saying "deletes itself" is not totally precise :)
On 2013/10/22 00:57:39, cbentzel wrote: > The combination of this and https://codereview.chromium.org/34013002/ looks > good. Excellent, thanks! > I would just merge those changes in here - sorry, I thought it would lead to a > bigger rewrite which was why I was hesitant to do the changes here. Thanks! I'll merge them tomorrow, my windows desktop shows 'offline' in chromoting again. What about network type filtering move (https://codereview.chromium.org/30753002/)? > Thanks for being patient with the reviews! My pleasure, I'm learning some good patterns and subtleties.
On 2013/10/22 02:21:16, mef wrote: > On 2013/10/22 00:57:39, cbentzel wrote: > > The combination of this and https://codereview.chromium.org/34013002/ looks > > good. > Excellent, thanks! > > > I would just merge those changes in here - sorry, I thought it would lead to a > > bigger rewrite which was why I was hesitant to do the changes here. > Thanks! I'll merge them tomorrow, my windows desktop shows 'offline' in > chromoting again. > What about network type filtering move > (https://codereview.chromium.org/30753002/%29? Leave that separate. > > > Thanks for being patient with the reviews! > My pleasure, I'm learning some good patterns and subtleties.
Sounds good. I've merged https://codereview.chromium.org/34013002 back in. Anything else? thanks, -m https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... File chrome/browser/extensions/api/networking_private/networking_private_apitest.cc (right): https://codereview.chromium.org/22295002/diff/613001/chrome/browser/extension... chrome/browser/extensions/api/networking_private/networking_private_apitest.cc:259: // Use Mocks WiFiService to test plumbing. On 2013/10/21 17:58:54, stevenjb wrote: > nit: s/Mocks/Mock Done.
LGTM Thanks!
Mostly looks ok, but I'm still a bit uneasy with pending message callback not getting cleared when the profile goes away (and extension functions bound to the callbacks getting destructed on IO thread after the UI thread stopped running (at least as far as I understand; process utility host keeps a reference to the NetworkingPrivateProcessClient, which keeps a callback with a reference to the ExtensionFunction, and the utility process host will be deleted during IO thread cleanup, at least if EndBatchMode is not called before https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/br...), especially since the extension function posts delete task to UI thread (https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...), so it will probably get leaked). can you at least add TODO for addressing this in a different cl, or proving me wrong (I'm not completely sure I'm not) :) https://codereview.chromium.org/22295002/diff/1013001/chrome/browser/extensio... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/1013001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:131: scoped_refptr<NetworkingPrivateProcessClient> ref_this_client(this); I prefer the other option (moving profile_->RemoveUserData after post task) https://codereview.chromium.org/22295002/diff/1013001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:180: utility_process_host_.reset(); no need for this, it's just confusing (and should not be done on UI thread) https://codereview.chromium.org/22295002/diff/1013001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:354: if (crypto_verify_mock) { I'd DCHECK for crypto_verify_mock instead. https://codereview.chromium.org/22295002/diff/1013001/chrome/browser/extensio... File chrome/browser/extensions/api/networking_private/networking_private_process_client.h (right): https://codereview.chromium.org/22295002/diff/1013001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_process_client.h:67: void(const std::string& error_name, why do you have both error_name and error? Shouldn't just one be enough?
Thanks, Toni! I've addressed simple comments, and added TODO to revisit profile deletion scenario. Do you know of a pattern/suggestion that I can follow? https://codereview.chromium.org/22295002/diff/1013001/chrome/browser/extensio... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/1013001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:131: scoped_refptr<NetworkingPrivateProcessClient> ref_this_client(this); On 2013/10/23 00:29:08, tbarzic wrote: > I prefer the other option (moving profile_->RemoveUserData after post task) Done. https://codereview.chromium.org/22295002/diff/1013001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:180: utility_process_host_.reset(); On 2013/10/23 00:29:08, tbarzic wrote: > no need for this, it's just confusing (and should not be done on UI thread) Done. https://codereview.chromium.org/22295002/diff/1013001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:354: if (crypto_verify_mock) { On 2013/10/23 00:29:08, tbarzic wrote: > I'd DCHECK for crypto_verify_mock instead. Done. https://codereview.chromium.org/22295002/diff/1013001/chrome/browser/extensio... File chrome/browser/extensions/api/networking_private/networking_private_process_client.h (right): https://codereview.chromium.org/22295002/diff/1013001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_process_client.h:67: void(const std::string& error_name, On 2013/10/23 00:29:08, tbarzic wrote: > why do you have both error_name and error? Shouldn't just one be enough? For some reason networking_private_api.h has the following signature of NetworkingPrivateVerifyXXXFunction::ErrorCallback void ErrorCallback(const std::string& error_name, const std::string& error); I didn't want to change it as it is shared with chromeos implementation. I'm planning some cleanup to make consistent NetworkingPrivateXXXFunction naming.
Hi guys, I would like to humbly ask for OWNER review of these changes: jhawkins@ - chrome/chrome.gyp (added files to utility library), chrome/utility/* (as it doesn't have local OWNERS). Please let me know if chrome/utility/wifi should local OWNERS. erg@ - chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc (extensions::NetworkingPrivateEventRouterFactory is now available on OS_WIN) palmer@ - chrome/common/networking_private_messages.* (IPC to utility process), ipc/ipc_message_start.h finnur@ - chrome/common/extensions/api/networking_private.json (make it available on "win"). thanks, -m
profiles lgtm
lgtm c/b/e/api/networking_private https://codereview.chromium.org/22295002/diff/1353001/chrome/browser/extensio... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/1353001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:191: // restart the utility process. if no observers, shutdown also, capital R in Restart https://codereview.chromium.org/22295002/diff/1353001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:368: *parameters_ptr.Pass())); nit: you missed one Pass ;) https://codereview.chromium.org/22295002/diff/1353001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:372: callbacks_map_.Clear(); nit: comment that a callback was added in SetupForTest, so it should be removed here.
Thanks! https://codereview.chromium.org/22295002/diff/1353001/chrome/browser/extensio... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/1353001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:191: // restart the utility process. On 2013/10/23 17:38:23, tbarzic wrote: > if no observers, shutdown > > also, capital R in Restart Done. https://codereview.chromium.org/22295002/diff/1353001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:368: *parameters_ptr.Pass())); On 2013/10/23 17:38:23, tbarzic wrote: > nit: you missed one Pass ;) Done. https://codereview.chromium.org/22295002/diff/1353001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:372: callbacks_map_.Clear(); On 2013/10/23 17:38:23, tbarzic wrote: > nit: comment that a callback was added in SetupForTest, so it should be removed > here. Done.
https://codereview.chromium.org/22295002/diff/1423001/chrome/browser/extensio... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/1423001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:191: if (network_events_observers_.might_have_observers()) nit: {} since it's multiline
https://codereview.chromium.org/22295002/diff/1423001/chrome/browser/extensio... File chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/1423001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:57: const std::string& error_name, As mentioned in the messages.h file, it's much better to make this an enum (and then range-checked in the IPC pickler, using IPC_ENUM_TRAITS_MAX_VALUE and friends), if possible. https://codereview.chromium.org/22295002/diff/1423001/chrome/browser/extensio... File chrome/browser/extensions/api/networking_private/networking_private_crypto.h (right): https://codereview.chromium.org/22295002/diff/1423001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:31: // RSAPublicKey. |data| is some string of bytes at smaller than the The preposition "at" is confusing me here; what does this paragraph mean? How does a potential caller of this function know what the maximum permissible size is? https://codereview.chromium.org/22295002/diff/1423001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:32: // maximum length permissable for PKCS#1 v1.5 with a key of |public_key| size. Typo: "permissible" https://codereview.chromium.org/22295002/diff/1423001/chrome/browser/extensio... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/1423001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:69: const char kNetworkingPrivateProcessClient[] = "NetworkingPrivateProcessClient"; Nit: This could also be static (or in the anonymous namespace). https://codereview.chromium.org/22295002/diff/1423001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:93: /* TODO(mef): Investigate the concern that pending message callback is not This comment is super confusing. Unbalanced parens. :) https://codereview.chromium.org/22295002/diff/1423001/chrome/browser/extensio... File chrome/browser/extensions/api/networking_private/networking_private_process_client.h (right): https://codereview.chromium.org/22295002/diff/1423001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_process_client.h:43: virtual bool VerifyAndEncryptData( Should this be named "SignAndEncrypt"? Normally I think of signature verification as something that happens before decryption. https://codereview.chromium.org/22295002/diff/1423001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_process_client.h:45: const std::string& plain_data, Following convention, I would call these |plaintext| and |ciphertext|. "encoded" doesn't mean "encrypted", to me (e.g. it could just mean base 64-encoded). https://codereview.chromium.org/22295002/diff/1423001/chrome/common/networkin... File chrome/common/networking_private_messages.h (right): https://codereview.chromium.org/22295002/diff/1423001/chrome/common/networkin... chrome/common/networking_private_messages.h:60: // |error_code| is a error code string. Typo: "an error" https://codereview.chromium.org/22295002/diff/1423001/chrome/common/networkin... chrome/common/networking_private_messages.h:63: std::string, /* error_code, if any */ Much better to pass ints, and stringify the errors in the browser. https://codereview.chromium.org/22295002/diff/1423001/chrome/common/networkin... chrome/common/networking_private_messages.h:67: // |properties| are properties convertible to NetworkProperties API object. Is it possible to send a NetworkProperties instead of a DictionaryValue? https://codereview.chromium.org/22295002/diff/1423001/chrome/common/networkin... chrome/common/networking_private_messages.h:91: base::ListValue /* Network list */) Is this also a sequence of GUID strings? What is the reason for using ListValue in some places and vector<string> in others?
erg@, tbarzic@ - thanks for your approval! palmer@ - thanks for your comments, PTAL (I have some answers)! I would really appreciate OWNERS review: jhawkins@ - chrome/chrome.gyp (added files to utility library), chrome/utility/* (as it doesn't have local OWNERS). Please let me know if chrome/utility/wifi should local OWNERS. finnur@ - chrome/common/extensions/api/networking_private.json (make it available on "win"). thanks, -m https://codereview.chromium.org/22295002/diff/1423001/chrome/browser/extensio... File chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc (right): https://codereview.chromium.org/22295002/diff/1423001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_api_nonchromeos.cc:57: const std::string& error_name, On 2013/10/23 22:16:12, Chromium Palmer wrote: > As mentioned in the messages.h file, it's much better to make this an enum (and > then range-checked in the IPC pickler, using IPC_ENUM_TRAITS_MAX_VALUE and > friends), if possible. The NetworkingPrivateXXXFunction API signatures has been previously defined and implemented for ChromeOS. I've tried to avoid changing it in Windows implementation. Having that said, it could make sense to use ENUMS for IPCs to utility process. Should I change those messages? https://codereview.chromium.org/22295002/diff/1423001/chrome/browser/extensio... File chrome/browser/extensions/api/networking_private/networking_private_crypto.h (right): https://codereview.chromium.org/22295002/diff/1423001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:31: // RSAPublicKey. |data| is some string of bytes at smaller than the On 2013/10/23 22:16:12, Chromium Palmer wrote: > The preposition "at" is confusing me here; what does this paragraph mean? How > does a potential caller of this function know what the maximum permissible size > is? Done. Bad cut-n-paste. https://codereview.chromium.org/22295002/diff/1423001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_crypto.h:32: // maximum length permissable for PKCS#1 v1.5 with a key of |public_key| size. On 2013/10/23 22:16:12, Chromium Palmer wrote: > Typo: "permissible" Done. https://codereview.chromium.org/22295002/diff/1423001/chrome/browser/extensio... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/1423001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:69: const char kNetworkingPrivateProcessClient[] = "NetworkingPrivateProcessClient"; On 2013/10/23 22:16:12, Chromium Palmer wrote: > Nit: This could also be static (or in the anonymous namespace). Done. https://codereview.chromium.org/22295002/diff/1423001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:93: /* TODO(mef): Investigate the concern that pending message callback is not On 2013/10/23 22:16:12, Chromium Palmer wrote: > This comment is super confusing. Unbalanced parens. :) Done. https://codereview.chromium.org/22295002/diff/1423001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:191: if (network_events_observers_.might_have_observers()) On 2013/10/23 18:27:23, tbarzic wrote: > nit: {} since it's multiline Done. https://codereview.chromium.org/22295002/diff/1423001/chrome/browser/extensio... File chrome/browser/extensions/api/networking_private/networking_private_process_client.h (right): https://codereview.chromium.org/22295002/diff/1423001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_process_client.h:43: virtual bool VerifyAndEncryptData( On 2013/10/23 22:16:12, Chromium Palmer wrote: > Should this be named "SignAndEncrypt"? Normally I think of signature > verification as something that happens before decryption. That's how it has been previously defined in the networking private api. https://codereview.chromium.org/22295002/diff/1423001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_process_client.h:45: const std::string& plain_data, On 2013/10/23 22:16:12, Chromium Palmer wrote: > Following convention, I would call these |plaintext| and |ciphertext|. "encoded" > doesn't mean "encrypted", to me (e.g. it could just mean base 64-encoded). Done. Yes, |plaintext| isn't base64-encoded, but |ciphertext| is. I don't know why. :-( https://codereview.chromium.org/22295002/diff/1423001/chrome/common/networkin... File chrome/common/networking_private_messages.h (right): https://codereview.chromium.org/22295002/diff/1423001/chrome/common/networkin... chrome/common/networking_private_messages.h:60: // |error_code| is a error code string. On 2013/10/23 22:16:12, Chromium Palmer wrote: > Typo: "an error" Done. https://codereview.chromium.org/22295002/diff/1423001/chrome/common/networkin... chrome/common/networking_private_messages.h:63: std::string, /* error_code, if any */ On 2013/10/23 22:16:12, Chromium Palmer wrote: > Much better to pass ints, and stringify the errors in the browser. I think Networking Private API already defines error as string, but I could pass int to NetworkingPrivateProcessClient here and stringify it there. Should I? https://codereview.chromium.org/22295002/diff/1423001/chrome/common/networkin... chrome/common/networking_private_messages.h:67: // |properties| are properties convertible to NetworkProperties API object. On 2013/10/23 22:16:12, Chromium Palmer wrote: > Is it possible to send a NetworkProperties instead of a DictionaryValue? Probably, but why? That DictionaryValue is passed directly to extension, so what would be the benefit of strong typing there? Besides, stevenjb@ has convinced me to replace all enums in NetworkProperties with strings from onc_constants.h. https://codereview.chromium.org/22295002/diff/1423001/chrome/common/networkin... chrome/common/networking_private_messages.h:91: base::ListValue /* Network list */) On 2013/10/23 22:16:12, Chromium Palmer wrote: > Is this also a sequence of GUID strings? What is the reason for using ListValue > in some places and vector<string> in others? This is list of NetworkingProperties-like DictionaryValues.
https://codereview.chromium.org/22295002/diff/1513001/chrome/common/extension... File chrome/common/extensions/api/networking_private.json (right): https://codereview.chromium.org/22295002/diff/1513001/chrome/common/extension... chrome/common/extensions/api/networking_private.json:12: "platforms": ["chromeos", "win"], I don't think this is correct. All platform names are spelled out in full (e.g. "windows"). https://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/fea...
Hi Finnur, thanks for your comment! Unfortunately there seems to be discrepancy here (see my comment). Any suggestions? thanks, -m https://codereview.chromium.org/22295002/diff/1513001/chrome/common/extension... File chrome/common/extensions/api/networking_private.json (right): https://codereview.chromium.org/22295002/diff/1513001/chrome/common/extension... chrome/common/extensions/api/networking_private.json:12: "platforms": ["chromeos", "win"], On 2013/10/24 08:51:55, Finnur wrote: > I don't think this is correct. All platform names are spelled out in full (e.g. > "windows"). > > https://src.chromium.org/viewvc/chrome/trunk/src/chrome/common/extensions/fea... I think this is different. This file is processed by cpp_bundle_generator (https://codereview.chromium.org/22295002/diff/1513001/tools/json_schema_compi...), which uses Platforms defined in model.py (https://code.google.com/p/chromium/codesearch#chromium/src/tools/json_schema_...) as follows: CHROMEOS = _PlatformInfo("chromeos") CHROMEOS_TOUCH = _PlatformInfo("chromeos_touch") LINUX = _PlatformInfo("linux") MAC = _PlatformInfo("mac") WIN = _PlatformInfo("win")
Oh, I see. LGTM then.
On 2013/10/24 13:16:09, Finnur wrote: > Oh, I see. > > LGTM then. Thanks, Finnur! Do you know, who would be a right person to OWNER-review https://codereview.chromium.org/22295002/diff/1513001/tools/json_schema_compi... as tools/ have '*' in OWNERS?
Well, that means anyone can do it. If it were me, I'd simply find someone who has worked on that file/function.
cpu FYI: is there any way on Windows we could restrict FS access without blocking the utility process from configuring WiFi interfaces? https://codereview.chromium.org/22295002/diff/1513001/chrome/browser/extensio... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/1513001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:118: utility_process_host_->DisableSandbox(); Was our conclusion that there was no way to block file system access on Windows without removing the ability to configure WiFi interfaces?
Hi, I would really appreciate OWNERS review: jam@ - chrome/chrome.gyp (added files to utility library), chrome/utility/* (as it doesn't have local OWNERS). Please let me know if chrome/utility/wifi should local OWNERS. palmer@ - could you take another look @ chrome/common/networking_private_messages.* (IPC to utility process), ipc/ipc_message_start.h (I've addressed and replied to your comments)? thanks, -m
On 2013/10/24 20:43:25, Jorge Lucangeli Obes wrote: > cpu FYI: is there any way on Windows we could restrict FS access without > blocking the utility process from configuring WiFi interfaces? > > https://codereview.chromium.org/22295002/diff/1513001/chrome/browser/extensio... > File > chrome/browser/extensions/api/networking_private/networking_private_process_client.cc > (right): > > https://codereview.chromium.org/22295002/diff/1513001/chrome/browser/extensio... > chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:118: > utility_process_host_->DisableSandbox(); > Was our conclusion that there was no way to block file system access on Windows > without removing the ability to configure WiFi interfaces? I haven't really looked into it and I didn't get any suggestions to research.
On 2013/10/24 20:48:14, mef wrote: > On 2013/10/24 20:43:25, Jorge Lucangeli Obes wrote: > > cpu FYI: is there any way on Windows we could restrict FS access without > > blocking the utility process from configuring WiFi interfaces? > > > > > https://codereview.chromium.org/22295002/diff/1513001/chrome/browser/extensio... > > File > > > chrome/browser/extensions/api/networking_private/networking_private_process_client.cc > > (right): > > > > > https://codereview.chromium.org/22295002/diff/1513001/chrome/browser/extensio... > > > chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:118: > > utility_process_host_->DisableSandbox(); > > Was our conclusion that there was no way to block file system access on > Windows > > without removing the ability to configure WiFi interfaces? > > I haven't really looked into it and I didn't get any suggestions to research. The gypi file lgtm Can you please explain what you mean by "isolate" in the cl description. i.e. why are you using the utility process, is this code crashy? Or does it leave behind extra dlls loaded in the process? something else?
> The gypi file lgtm > > Can you please explain what you mean by "isolate" in the cl description. i.e. > why are you using the utility process, is this code crashy? Or does it leave > behind extra dlls loaded in the process? something else? Thanks jam, I think jorgelo can better explain the motivation of using the utility process, but here is my understanding: - We need to use additional system api (WlanApi on WIndows), that is not needed for browser process. - We could tweak utility process sandbox to precisely limit needed permissions (e.g. don't allow file access). - WlanApi implementation is dependent on OEM Wlan drivers which can be buggy/crashy/hangy. - Clean IPC interface between processes limits potential for abuse if browser process is compromised. thanks, -m
https://codereview.chromium.org/22295002/diff/1513001/chrome/browser/extensio... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/1513001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:120: } We need to take a look at this more carefully. There are several sandbox levels each with adjustable knobs. I dare to say that there should be a sandbox level that works for this case.
On 2013/10/25 11:57:47, mef wrote: > > The gypi file lgtm > > > > Can you please explain what you mean by "isolate" in the cl description. i.e. > > why are you using the utility process, is this code crashy? Or does it leave > > behind extra dlls loaded in the process? something else? > > Thanks jam, I think jorgelo can better explain the motivation of using the > utility process, but here is my understanding: > > - We need to use additional system api (WlanApi on WIndows), that is not needed > for browser process. > - We could tweak utility process sandbox to precisely limit needed permissions > (e.g. don't allow file access). > - WlanApi implementation is dependent on OEM Wlan drivers which can be > buggy/crashy/hangy. > - Clean IPC interface between processes limits potential for abuse if browser > process is compromised. > > thanks, > -m It wasn't clear to us how much the Windows sandbox could be tweaked to restrict this code to only access what it needs in the WlanApi, so a utility process implementation would give us the ability to tighten that in the future.
Hi, thank you for your comments, I would really appreciate OWNERS review: jam@ - thanks for *.gyp* approval. Please confirm whether chrome/utility/* looks good too (as it doesn't have local OWNERS). Please let me know if chrome/utility/wifi should get local OWNERS. cevans@ - chrome/common/networking_private_messages.* (IPC to utility process), ipc/ipc_message_start.h (palmer@ has started to review, but is busy at the moment)? thanks, -m https://codereview.chromium.org/22295002/diff/1513001/chrome/browser/extensio... File chrome/browser/extensions/api/networking_private/networking_private_process_client.cc (right): https://codereview.chromium.org/22295002/diff/1513001/chrome/browser/extensio... chrome/browser/extensions/api/networking_private/networking_private_process_client.cc:120: } On 2013/10/25 16:53:48, cpu wrote: > We need to take a look at this more carefully. There are several sandbox levels > each with adjustable knobs. I dare to say that there should be a sandbox level > that works for this case. Sounds good.
IPC security LGTM.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mef@chromium.org/22295002/1513001
On 2013/10/25 16:58:52, Jorge Lucangeli Obes wrote: > On 2013/10/25 11:57:47, mef wrote: > > > The gypi file lgtm > > > > > > Can you please explain what you mean by "isolate" in the cl description. > i.e. > > > why are you using the utility process, is this code crashy? Or does it leave > > > behind extra dlls loaded in the process? something else? > > > > Thanks jam, I think jorgelo can better explain the motivation of using the > > utility process, but here is my understanding: > > > > - We need to use additional system api (WlanApi on WIndows), that is not > needed > > for browser process. > > - We could tweak utility process sandbox to precisely limit needed permissions > > (e.g. don't allow file access). > > - WlanApi implementation is dependent on OEM Wlan drivers which can be > > buggy/crashy/hangy. > > - Clean IPC interface between processes limits potential for abuse if browser > > process is compromised. > > > > thanks, > > -m > > It wasn't clear to us how much the Windows sandbox could be tweaked to restrict > this code to only access what it needs in the WlanApi, so a utility process > implementation would give us the ability to tighten that in the future. hey guys, I'd like a bit more background before we land this cl. So far, I don't understand the reason to to this in the utility process. Normally, we have two reasons to use the utility process. The first is that we're doing an untrusted operation, say extension unpacking. We want to run this in the sandbox because we don't trust that the extension file could have an exploit. The second is that we know a piece of code is crashy. We want to do that in another process so we don't impact the stability of the browser process. In this case, we're calling Windows APIs to get a list of network adapters. From my naive perspective, I don't see how this is insecure? And do we have any data that this is crashy? It seems that getting this information is something that happens all the time on users' machines. Using the utility process is not free. It takes time to start it, but more importantly, we end up having to write this extra code to do IPC.
Originally, the API was expected to slurp WPA credentials from the OS, and that would have needed a UAC prompt. That's why the code couldn't live in the browser process. The credentials requirement was subsequently dropped. Keeping this code in the utility process continued to make sense to me mostly because the data being processed is influenced by the environment. We had an issue on CrOS a while ago where malformed SSIDs would make wpa_supplicant crash. However, in this case malformed data is more likely to crash Windows than Chrome. If we don't expect the credentials requirement to come back, don't believe instability is gonna be an issue, and don't consider spawning a new process worth it, then we can reconsider the utility process approach. FWIW, we're not just getting the list of networks, we're reconfiguring the adapters to connect to a specific one (so that Chrome can setup a Chromecast). On Fri, Oct 25, 2013 at 12:43 PM, <jam@chromium.org> wrote: > On 2013/10/25 16:58:52, Jorge Lucangeli Obes wrote: > >> On 2013/10/25 11:57:47, mef wrote: >> > > The gypi file lgtm >> > > >> > > Can you please explain what you mean by "isolate" in the cl >> description. >> i.e. >> > > why are you using the utility process, is this code crashy? Or does it >> > leave > >> > > behind extra dlls loaded in the process? something else? >> > >> > Thanks jam, I think jorgelo can better explain the motivation of using >> the >> > utility process, but here is my understanding: >> > >> > - We need to use additional system api (WlanApi on WIndows), that is not >> needed >> > for browser process. >> > - We could tweak utility process sandbox to precisely limit needed >> > permissions > >> > (e.g. don't allow file access). >> > - WlanApi implementation is dependent on OEM Wlan drivers which can be >> > buggy/crashy/hangy. >> > - Clean IPC interface between processes limits potential for abuse if >> > browser > >> > process is compromised. >> > >> > thanks, >> > -m >> > > It wasn't clear to us how much the Windows sandbox could be tweaked to >> > restrict > >> this code to only access what it needs in the WlanApi, so a utility >> process >> implementation would give us the ability to tighten that in the future. >> > > hey guys, I'd like a bit more background before we land this cl. So far, I > don't > understand the reason to to this in the utility process. > > Normally, we have two reasons to use the utility process. The first is that > we're doing an untrusted operation, say extension unpacking. We want to > run this > in the sandbox because we don't trust that the extension file could have an > exploit. The second is that we know a piece of code is crashy. We want to > do > that in another process so we don't impact the stability of the browser > process. > > In this case, we're calling Windows APIs to get a list of network > adapters. From > my naive perspective, I don't see how this is insecure? And do we have any > data > that this is crashy? It seems that getting this information is something > that > happens all the time on users' machines. > > Using the utility process is not free. It takes time to start it, but more > importantly, we end up having to write this extra code to do IPC. > > https://codereview.chromium.**org/22295002/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Note: the WPA credential slurping may come back soon. On Oct 25, 2013 7:54 PM, "Jorge Lucangeli Obes" <jorgelo@chromium.org> wrote: > Originally, the API was expected to slurp WPA credentials from the OS, and > that would have needed a UAC prompt. That's why the code couldn't live in > the browser process. The credentials requirement was subsequently dropped. > > Keeping this code in the utility process continued to make sense to me > mostly because the data being processed is influenced by the environment. > We had an issue on CrOS a while ago where malformed SSIDs would make > wpa_supplicant crash. However, in this case malformed data is more likely > to crash Windows than Chrome. If we don't expect the credentials > requirement to come back, don't believe instability is gonna be an issue, > and don't consider spawning a new process worth it, then we can reconsider > the utility process approach. > > FWIW, we're not just getting the list of networks, we're reconfiguring the > adapters to connect to a specific one (so that Chrome can setup a > Chromecast). > > On Fri, Oct 25, 2013 at 12:43 PM, <jam@chromium.org> wrote: > >> On 2013/10/25 16:58:52, Jorge Lucangeli Obes wrote: >> >>> On 2013/10/25 11:57:47, mef wrote: >>> > > The gypi file lgtm >>> > > >>> > > Can you please explain what you mean by "isolate" in the cl >>> description. >>> i.e. >>> > > why are you using the utility process, is this code crashy? Or does >>> it >>> >> leave >> >>> > > behind extra dlls loaded in the process? something else? >>> > >>> > Thanks jam, I think jorgelo can better explain the motivation of using >>> the >>> > utility process, but here is my understanding: >>> > >>> > - We need to use additional system api (WlanApi on WIndows), that is >>> not >>> needed >>> > for browser process. >>> > - We could tweak utility process sandbox to precisely limit needed >>> >> permissions >> >>> > (e.g. don't allow file access). >>> > - WlanApi implementation is dependent on OEM Wlan drivers which can be >>> > buggy/crashy/hangy. >>> > - Clean IPC interface between processes limits potential for abuse if >>> >> browser >> >>> > process is compromised. >>> > >>> > thanks, >>> > -m >>> >> >> It wasn't clear to us how much the Windows sandbox could be tweaked to >>> >> restrict >> >>> this code to only access what it needs in the WlanApi, so a utility >>> process >>> implementation would give us the ability to tighten that in the future. >>> >> >> hey guys, I'd like a bit more background before we land this cl. So far, >> I don't >> understand the reason to to this in the utility process. >> >> Normally, we have two reasons to use the utility process. The first is >> that >> we're doing an untrusted operation, say extension unpacking. We want to >> run this >> in the sandbox because we don't trust that the extension file could have >> an >> exploit. The second is that we know a piece of code is crashy. We want to >> do >> that in another process so we don't impact the stability of the browser >> process. >> >> In this case, we're calling Windows APIs to get a list of network >> adapters. From >> my naive perspective, I don't see how this is insecure? And do we have >> any data >> that this is crashy? It seems that getting this information is something >> that >> happens all the time on users' machines. >> >> Using the utility process is not free. It takes time to start it, but more >> importantly, we end up having to write this extra code to do IPC. >> >> https://codereview.chromium.**org/22295002/<https://codereview.chromium.org/2... >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/10/25 23:54:19, Jorge Lucangeli Obes wrote: > Originally, the API was expected to slurp WPA credentials from the OS, and > that would have needed a UAC prompt. That's why the code couldn't live in > the browser process. I'm not familiar with this, can you explain why we can't UAC from the browser process? > The credentials requirement was subsequently dropped. > > Keeping this code in the utility process continued to make sense to me > mostly because the data being processed is influenced by the environment. I don't follow this logic? We process lots of stuff in the browser process that's influenced by the environment. > We had an issue on CrOS a while ago where malformed SSIDs would make > wpa_supplicant crash. However, in this case malformed data is more likely > to crash Windows than Chrome. So I still don't understand why a utility process helps? The CrOS case seems like something that was fixed, and is in our control anyways. If on Windows the OS crashes, then it doesn't matter which process calls this. > If we don't expect the credentials > requirement to come back, don't believe instability is gonna be an issue, > and don't consider spawning a new process worth it, then we can reconsider > the utility process approach. > > FWIW, we're not just getting the list of networks, we're reconfiguring the > adapters to connect to a specific one (so that Chrome can setup a > Chromecast). > > On Fri, Oct 25, 2013 at 12:43 PM, <mailto:jam@chromium.org> wrote: > > > On 2013/10/25 16:58:52, Jorge Lucangeli Obes wrote: > > > >> On 2013/10/25 11:57:47, mef wrote: > >> > > The gypi file lgtm > >> > > > >> > > Can you please explain what you mean by "isolate" in the cl > >> description. > >> i.e. > >> > > why are you using the utility process, is this code crashy? Or does it > >> > > leave > > > >> > > behind extra dlls loaded in the process? something else? > >> > > >> > Thanks jam, I think jorgelo can better explain the motivation of using > >> the > >> > utility process, but here is my understanding: > >> > > >> > - We need to use additional system api (WlanApi on WIndows), that is not > >> needed > >> > for browser process. > >> > - We could tweak utility process sandbox to precisely limit needed > >> > > permissions > > > >> > (e.g. don't allow file access). > >> > - WlanApi implementation is dependent on OEM Wlan drivers which can be > >> > buggy/crashy/hangy. > >> > - Clean IPC interface between processes limits potential for abuse if > >> > > browser > > > >> > process is compromised. > >> > > >> > thanks, > >> > -m > >> > > > > It wasn't clear to us how much the Windows sandbox could be tweaked to > >> > > restrict > > > >> this code to only access what it needs in the WlanApi, so a utility > >> process > >> implementation would give us the ability to tighten that in the future. > >> > > > > hey guys, I'd like a bit more background before we land this cl. So far, I > > don't > > understand the reason to to this in the utility process. > > > > Normally, we have two reasons to use the utility process. The first is that > > we're doing an untrusted operation, say extension unpacking. We want to > > run this > > in the sandbox because we don't trust that the extension file could have an > > exploit. The second is that we know a piece of code is crashy. We want to > > do > > that in another process so we don't impact the stability of the browser > > process. > > > > In this case, we're calling Windows APIs to get a list of network > > adapters. From > > my naive perspective, I don't see how this is insecure? And do we have any > > data > > that this is crashy? It seems that getting this information is something > > that > > happens all the time on users' machines. > > > > Using the utility process is not free. It takes time to start it, but more > > importantly, we end up having to write this extra code to do IPC. > > > > > https://codereview.chromium.**org/22295002/%3Chttps://codereview.chromium.org...> > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Chris and Jorge will correct me if I'm wrong, but I just want to express my side of the story. - My (possibly incorrect) understanding is that we want to minimize amount of code that is running outside of sandbox. - Process isolation is useful tool to limit exposure surface. - The utility process is used for this purpose to isolate import of bookmarks, Picasa albums, iTunes Library, etc. - So, it would appear logical to use utility process to isolate usage of possible harmful apis (e.g. transparent switch of wifi networks) from browser process. The biggest negative side here is code complexity as process startup performance and cross-process IPCs are not very important in this use case (once-in-a-while setup of ChromeCast device). > > Originally, the API was expected to slurp WPA credentials from the OS, and > > that would have needed a UAC prompt. That's why the code couldn't live in > > the browser process. > > I'm not familiar with this, can you explain why we can't UAC from the browser > process? We CAN UAC from browser process, but I don't think that the process should be launched raised UAC privilieges. > > The credentials requirement was subsequently dropped. > > Keeping this code in the utility process continued to make sense to me > > mostly because the data being processed is influenced by the environment. > I don't follow this logic? We process lots of stuff in the browser process > that's influenced by the environment. Do we have some formal or informal guidelines in regards to safe and unsafe things to do in browser process? > > We had an issue on CrOS a while ago where malformed SSIDs would make > > wpa_supplicant crash. However, in this case malformed data is more likely > > to crash Windows than Chrome. > > So I still don't understand why a utility process helps? The CrOS case seems > like something that was fixed, and is in our control anyways. If on Windows the > OS crashes, then it doesn't matter which process calls this. It doesn't, but if utility process crashes, then the rest of the browser keeps on going. > > If we don't expect the credentials > > requirement to come back, don't believe instability is gonna be an issue, > > and don't consider spawning a new process worth it, then we can reconsider > > the utility process approach. > > > > FWIW, we're not just getting the list of networks, we're reconfiguring the > > adapters to connect to a specific one (so that Chrome can setup a > > Chromecast). > > On Fri, Oct 25, 2013 at 12:43 PM, <mailto:jam@chromium.org> wrote: > > > > > On 2013/10/25 16:58:52, Jorge Lucangeli Obes wrote: > > > > > >> On 2013/10/25 11:57:47, mef wrote: > > >> > > The gypi file lgtm > > >> > > > > >> > > Can you please explain what you mean by "isolate" in the cl > > >> description. > > >> i.e. > > >> > > why are you using the utility process, is this code crashy? Or does it > > >> > > > leave > > > > > >> > > behind extra dlls loaded in the process? something else? > > >> > > > >> > Thanks jam, I think jorgelo can better explain the motivation of using > > >> the > > >> > utility process, but here is my understanding: > > >> > > > >> > - We need to use additional system api (WlanApi on WIndows), that is not > > >> needed > > >> > for browser process. > > >> > - We could tweak utility process sandbox to precisely limit needed > > >> > > > permissions > > > > > >> > (e.g. don't allow file access). > > >> > - WlanApi implementation is dependent on OEM Wlan drivers which can be > > >> > buggy/crashy/hangy. > > >> > - Clean IPC interface between processes limits potential for abuse if > > >> > > > browser > > > > > >> > process is compromised. > > >> > > > >> > thanks, > > >> > -m > > >> > > > > > > It wasn't clear to us how much the Windows sandbox could be tweaked to > > >> > > > restrict > > > > > >> this code to only access what it needs in the WlanApi, so a utility > > >> process > > >> implementation would give us the ability to tighten that in the future. > > >> > > > > > > hey guys, I'd like a bit more background before we land this cl. So far, I > > > don't > > > understand the reason to to this in the utility process. > > > > > > Normally, we have two reasons to use the utility process. The first is that > > > we're doing an untrusted operation, say extension unpacking. We want to > > > run this > > > in the sandbox because we don't trust that the extension file could have an > > > exploit. The second is that we know a piece of code is crashy. We want to > > > do > > > that in another process so we don't impact the stability of the browser > > > process. > > > > > > In this case, we're calling Windows APIs to get a list of network > > > adapters. From > > > my naive perspective, I don't see how this is insecure? And do we have any > > > data > > > that this is crashy? It seems that getting this information is something > > > that > > > happens all the time on users' machines. > > > > > > Using the utility process is not free. It takes time to start it, but more > > > importantly, we end up having to write this extra code to do IPC. > > > > > > > > > https://codereview.chromium.**org/22295002/%253Chttps://codereview.chromium.o...> > > > > > > > To unsubscribe from this group and stop receiving emails from it, send an > email > > to mailto:chromium-reviews+unsubscribe@chromium.org.
On Mon, Oct 28, 2013 at 9:54 AM, <jam@chromium.org> wrote: > On 2013/10/25 23:54:19, Jorge Lucangeli Obes wrote: > >> Originally, the API was expected to slurp WPA credentials from the OS, and >> that would have needed a UAC prompt. That's why the code couldn't live in >> the browser process. >> > > I'm not familiar with this, can you explain why we can't UAC from the > browser > process? > > Not a Windows expert here, but when I brought this up in the security team, we agreed that a process with elevated privileges (i.e. after UAC prompting) should be short-lived. That way we don't have to have code in the browser process to drop the token, not we have to deal with races on that. It makes reasoning about the privileges of the different processes easier. It's the same reason we ship a separate chrome-sandbox setuid binary on Linux. > > The credentials requirement was subsequently dropped. >> > > Keeping this code in the utility process continued to make sense to me >> mostly because the data being processed is influenced by the environment. >> > > I don't follow this logic? We process lots of stuff in the browser process > that's influenced by the environment. > > > We had an issue on CrOS a while ago where malformed SSIDs would make >> wpa_supplicant crash. However, in this case malformed data is more likely >> to crash Windows than Chrome. >> > > So I still don't understand why a utility process helps? The CrOS case > seems > like something that was fixed, and is in our control anyways. If on > Windows the > OS crashes, then it doesn't matter which process calls this. > > > If we don't expect the credentials >> requirement to come back, don't believe instability is gonna be an issue, >> and don't consider spawning a new process worth it, then we can reconsider >> the utility process approach. >> > > FWIW, we're not just getting the list of networks, we're reconfiguring the >> adapters to connect to a specific one (so that Chrome can setup a >> Chromecast). >> > > On Fri, Oct 25, 2013 at 12:43 PM, <mailto:jam@chromium.org> wrote: >> > > > On 2013/10/25 16:58:52, Jorge Lucangeli Obes wrote: >> > >> >> On 2013/10/25 11:57:47, mef wrote: >> >> > > The gypi file lgtm >> >> > > >> >> > > Can you please explain what you mean by "isolate" in the cl >> >> description. >> >> i.e. >> >> > > why are you using the utility process, is this code crashy? Or >> does it >> >> >> > leave >> > >> >> > > behind extra dlls loaded in the process? something else? >> >> > >> >> > Thanks jam, I think jorgelo can better explain the motivation of >> using >> >> the >> >> > utility process, but here is my understanding: >> >> > >> >> > - We need to use additional system api (WlanApi on WIndows), that is >> not >> >> needed >> >> > for browser process. >> >> > - We could tweak utility process sandbox to precisely limit needed >> >> >> > permissions >> > >> >> > (e.g. don't allow file access). >> >> > - WlanApi implementation is dependent on OEM Wlan drivers which can >> be >> >> > buggy/crashy/hangy. >> >> > - Clean IPC interface between processes limits potential for abuse if >> >> >> > browser >> > >> >> > process is compromised. >> >> > >> >> > thanks, >> >> > -m >> >> >> > >> > It wasn't clear to us how much the Windows sandbox could be tweaked to >> >> >> > restrict >> > >> >> this code to only access what it needs in the WlanApi, so a utility >> >> process >> >> implementation would give us the ability to tighten that in the future. >> >> >> > >> > hey guys, I'd like a bit more background before we land this cl. So >> far, I >> > don't >> > understand the reason to to this in the utility process. >> > >> > Normally, we have two reasons to use the utility process. The first is >> that >> > we're doing an untrusted operation, say extension unpacking. We want to >> > run this >> > in the sandbox because we don't trust that the extension file could >> have an >> > exploit. The second is that we know a piece of code is crashy. We want >> to >> > do >> > that in another process so we don't impact the stability of the browser >> > process. >> > >> > In this case, we're calling Windows APIs to get a list of network >> > adapters. From >> > my naive perspective, I don't see how this is insecure? And do we have >> any >> > data >> > that this is crashy? It seems that getting this information is something >> > that >> > happens all the time on users' machines. >> > >> > Using the utility process is not free. It takes time to start it, but >> more >> > importantly, we end up having to write this extra code to do IPC. >> > >> > >> > > https://codereview.chromium.****org/22295002/%3Chttps://codere** > view.chromium.org/22295002/ <http://codereview.chromium.org/22295002/>> > >> > >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to mailto:chromium-reviews+**unsubscribe@chromium.org<chromium-reviews%2Bunsubscribe@chromium.org> >> . >> > > > > https://codereview.chromium.**org/22295002/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Mon, Oct 28, 2013 at 10:57 AM, <mef@chromium.org> wrote: > Chris and Jorge will correct me if I'm wrong, but I just want to express > my side > of the story. > > - My (possibly incorrect) understanding is that we want to minimize amount > of > code that is running outside of sandbox. > - Process isolation is useful tool to limit exposure surface. > - The utility process is used for this purpose to isolate import of > bookmarks, > Picasa albums, iTunes Library, etc. > - So, it would appear logical to use utility process to isolate usage of > possible harmful apis (e.g. transparent switch of wifi networks) from > browser > process. > > This is true, but what we want to sandbox is mostly processing of untrusted input. There's no processing of untrusted input in this case. What we do have is a (possible, future) operation that needs elevated privileges, which we don't want to give (and then have to take away) from a long-lived process like the browser process. > The biggest negative side here is code complexity as > process startup performance and cross-process IPCs are not very important > in > this use case (once-in-a-while setup of ChromeCast device). > > > > Originally, the API was expected to slurp WPA credentials from the OS, >> and >> > that would have needed a UAC prompt. That's why the code couldn't live >> in >> > the browser process. >> > > I'm not familiar with this, can you explain why we can't UAC from the >> browser >> process? >> > We CAN UAC from browser process, but I don't think that the process should > be > launched raised UAC privilieges. > > > > The credentials requirement was subsequently dropped. >> > Keeping this code in the utility process continued to make sense to me >> > mostly because the data being processed is influenced by the >> environment. >> I don't follow this logic? We process lots of stuff in the browser process >> that's influenced by the environment. >> > Do we have some formal or informal guidelines in regards to safe and unsafe > things to do in browser process? > > Untrusted input should not be processed in the browser process. > > > > We had an issue on CrOS a while ago where malformed SSIDs would make >> > wpa_supplicant crash. However, in this case malformed data is more >> likely >> > to crash Windows than Chrome. >> > > So I still don't understand why a utility process helps? The CrOS case >> seems >> like something that was fixed, and is in our control anyways. If on >> Windows >> > the > >> OS crashes, then it doesn't matter which process calls this. >> > It doesn't, but if utility process crashes, then the rest of the browser > keeps > on going. > > > > If we don't expect the credentials >> > requirement to come back, don't believe instability is gonna be an >> issue, >> > and don't consider spawning a new process worth it, then we can >> reconsider >> > the utility process approach. >> > >> > FWIW, we're not just getting the list of networks, we're reconfiguring >> the >> > adapters to connect to a specific one (so that Chrome can setup a >> > Chromecast). >> > > > > > On Fri, Oct 25, 2013 at 12:43 PM, <mailto:jam@chromium.org> wrote: >> > >> > > On 2013/10/25 16:58:52, Jorge Lucangeli Obes wrote: >> > > >> > >> On 2013/10/25 11:57:47, mef wrote: >> > >> > > The gypi file lgtm >> > >> > > >> > >> > > Can you please explain what you mean by "isolate" in the cl >> > >> description. >> > >> i.e. >> > >> > > why are you using the utility process, is this code crashy? Or >> does >> > it > >> > >> >> > > leave >> > > >> > >> > > behind extra dlls loaded in the process? something else? >> > >> > >> > >> > Thanks jam, I think jorgelo can better explain the motivation of >> using >> > >> the >> > >> > utility process, but here is my understanding: >> > >> > >> > >> > - We need to use additional system api (WlanApi on WIndows), that >> is >> > not > >> > >> needed >> > >> > for browser process. >> > >> > - We could tweak utility process sandbox to precisely limit needed >> > >> >> > > permissions >> > > >> > >> > (e.g. don't allow file access). >> > >> > - WlanApi implementation is dependent on OEM Wlan drivers which >> can be >> > >> > buggy/crashy/hangy. >> > >> > - Clean IPC interface between processes limits potential for abuse >> if >> > >> >> > > browser >> > > >> > >> > process is compromised. >> > >> > >> > >> > thanks, >> > >> > -m >> > >> >> > > >> > > It wasn't clear to us how much the Windows sandbox could be tweaked >> to >> > >> >> > > restrict >> > > >> > >> this code to only access what it needs in the WlanApi, so a utility >> > >> process >> > >> implementation would give us the ability to tighten that in the >> future. >> > >> >> > > >> > > hey guys, I'd like a bit more background before we land this cl. So >> far, I >> > > don't >> > > understand the reason to to this in the utility process. >> > > >> > > Normally, we have two reasons to use the utility process. The first is >> > that > >> > > we're doing an untrusted operation, say extension unpacking. We want >> to >> > > run this >> > > in the sandbox because we don't trust that the extension file could >> have >> > an > >> > > exploit. The second is that we know a piece of code is crashy. We >> want to >> > > do >> > > that in another process so we don't impact the stability of the >> browser >> > > process. >> > > >> > > In this case, we're calling Windows APIs to get a list of network >> > > adapters. From >> > > my naive perspective, I don't see how this is insecure? And do we >> have any >> > > data >> > > that this is crashy? It seems that getting this information is >> something >> > > that >> > > happens all the time on users' machines. >> > > >> > > Using the utility process is not free. It takes time to start it, but >> more >> > > importantly, we end up having to write this extra code to do IPC. >> > > >> > > >> > >> > > https://codereview.chromium.****org/22295002/%253Chttps://code** > review.chromium.org/22295002/ <http://codereview.chromium.org/22295002/>> > > > > >> > >> > To unsubscribe from this group and stop receiving emails from it, send >> an >> email >> > to mailto:chromium-reviews+**unsubscribe@chromium.org<chromium-reviews%2Bunsubscribe@chromium.org> >> . >> > > > https://codereview.chromium.**org/22295002/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/10/28 21:43:43, Jorge Lucangeli Obes wrote: > On Mon, Oct 28, 2013 at 10:57 AM, <mailto:mef@chromium.org> wrote: > > > Chris and Jorge will correct me if I'm wrong, but I just want to express > > my side > > of the story. > > > > - My (possibly incorrect) understanding is that we want to minimize amount > > of > > code that is running outside of sandbox. > > - Process isolation is useful tool to limit exposure surface. > > - The utility process is used for this purpose to isolate import of > > bookmarks, > > Picasa albums, iTunes Library, etc. > > - So, it would appear logical to use utility process to isolate usage of > > possible harmful apis (e.g. transparent switch of wifi networks) from > > browser > > process. > > > > > This is true, but what we want to sandbox is mostly processing of untrusted > input. There's no processing of untrusted input in this case. What we do > have is a (possible, future) operation that needs elevated privileges, > which we don't want to give (and then have to take away) from a long-lived > process like the browser process. I see, thanks for the background. In that case, let's not use the utility process until the future comes when we need UAC prompting.
The need for this may be very soon. I'll try to make sure over the next day. On Oct 28, 2013 6:26 PM, <jam@chromium.org> wrote: > On 2013/10/28 21:43:43, Jorge Lucangeli Obes wrote: > >> On Mon, Oct 28, 2013 at 10:57 AM, <mailto:mef@chromium.org> wrote: >> > > > Chris and Jorge will correct me if I'm wrong, but I just want to express >> > my side >> > of the story. >> > >> > - My (possibly incorrect) understanding is that we want to minimize >> amount >> > of >> > code that is running outside of sandbox. >> > - Process isolation is useful tool to limit exposure surface. >> > - The utility process is used for this purpose to isolate import of >> > bookmarks, >> > Picasa albums, iTunes Library, etc. >> > - So, it would appear logical to use utility process to isolate usage of >> > possible harmful apis (e.g. transparent switch of wifi networks) from >> > browser >> > process. >> > >> > >> This is true, but what we want to sandbox is mostly processing of >> untrusted >> input. There's no processing of untrusted input in this case. What we do >> have is a (possible, future) operation that needs elevated privileges, >> which we don't want to give (and then have to take away) from a long-lived >> process like the browser process. >> > > I see, thanks for the background. > > In that case, let's not use the utility process until the future comes > when we > need UAC prompting. > > https://codereview.chromium.**org/22295002/<https://codereview.chromium.org/2... > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/10/28 22:26:32, jam wrote: > On 2013/10/28 21:43:43, Jorge Lucangeli Obes wrote: > > On Mon, Oct 28, 2013 at 10:57 AM, <mailto:mef@chromium.org> wrote: > > > > > Chris and Jorge will correct me if I'm wrong, but I just want to express > > > my side > > > of the story. > > > > > > - My (possibly incorrect) understanding is that we want to minimize amount > > > of > > > code that is running outside of sandbox. > > > - Process isolation is useful tool to limit exposure surface. > > > - The utility process is used for this purpose to isolate import of > > > bookmarks, > > > Picasa albums, iTunes Library, etc. > > > - So, it would appear logical to use utility process to isolate usage of > > > possible harmful apis (e.g. transparent switch of wifi networks) from > > > browser > > > process. > > > > > > > > This is true, but what we want to sandbox is mostly processing of untrusted > > input. There's no processing of untrusted input in this case. What we do > > have is a (possible, future) operation that needs elevated privileges, > > which we don't want to give (and then have to take away) from a long-lived > > process like the browser process. > > I see, thanks for the background. > > In that case, let's not use the utility process until the future comes when we > need UAC prompting. I've run some experiments and wlanapi calls seem to work fine from browser process. Moreover, wlanapi.dll is always loaded into it anyway (probably by geolocation module). Also, I was able to ShellExecute netsh.exe under UAC to obtain clear-text WiFi password. It sounds like we are willing to trade little extra safety for little lower complexity. Just to confirm that I understand proposed change, here is what I'm going to do: - Remove networking_private_handler.*, networking_private_process_client.*, networking_private_messages.* - Add networking_private_service_client.* to replace networking_private_process_client.* to encapsulate inproc lifetime and thread/task management interface between NetworkingPrivateApi*Functions and WiFiService. - Move wifi_service.* from chrome/utility/wifi/ to component/wifi/ or chrome/browser/extensions/api/networking_private/ (which one is better?). I'll update the design doc (https://docs.google.com/a/google.com/document/d/1-QmMyvuJgjyyriT0mjrpr56Pg0CK...) to reflect proposed hierarchy. thanks, -m
I'm still worried about shell executing netsh.exe from a product standpoint - if the UAC prompt comes up and blames Chrome once the user clicks "grab password" it may not surprise them, but if it blames netsh they may be concerned. On Wed, Oct 30, 2013 at 12:04 PM, <mef@chromium.org> wrote: > On 2013/10/28 22:26:32, jam wrote: >> >> On 2013/10/28 21:43:43, Jorge Lucangeli Obes wrote: >> > On Mon, Oct 28, 2013 at 10:57 AM, <mailto:mef@chromium.org> wrote: >> > >> > > Chris and Jorge will correct me if I'm wrong, but I just want to >> > > express >> > > my side >> > > of the story. >> > > >> > > - My (possibly incorrect) understanding is that we want to minimize >> > > amount >> > > of >> > > code that is running outside of sandbox. >> > > - Process isolation is useful tool to limit exposure surface. >> > > - The utility process is used for this purpose to isolate import of >> > > bookmarks, >> > > Picasa albums, iTunes Library, etc. >> > > - So, it would appear logical to use utility process to isolate usage >> > > of >> > > possible harmful apis (e.g. transparent switch of wifi networks) from >> > > browser >> > > process. >> > > >> > > >> > This is true, but what we want to sandbox is mostly processing of >> > untrusted >> > input. There's no processing of untrusted input in this case. What we do >> > have is a (possible, future) operation that needs elevated privileges, >> > which we don't want to give (and then have to take away) from a >> > long-lived >> > process like the browser process. > > >> I see, thanks for the background. > > >> In that case, let's not use the utility process until the future comes >> when we >> need UAC prompting. > > > I've run some experiments and wlanapi calls seem to work fine from browser > process. > Moreover, wlanapi.dll is always loaded into it anyway (probably by > geolocation > module). > Also, I was able to ShellExecute netsh.exe under UAC to obtain clear-text > WiFi > password. > > It sounds like we are willing to trade little extra safety for little lower > complexity. > > Just to confirm that I understand proposed change, here is what I'm going to > do: > > - Remove networking_private_handler.*, networking_private_process_client.*, > networking_private_messages.* > - Add networking_private_service_client.* to replace > networking_private_process_client.* to encapsulate > inproc lifetime and thread/task management interface between > NetworkingPrivateApi*Functions and WiFiService. > - Move wifi_service.* from chrome/utility/wifi/ to component/wifi/ or > chrome/browser/extensions/api/networking_private/ (which one is better?). > > I'll update the design doc > (https://docs.google.com/a/google.com/document/d/1-QmMyvuJgjyyriT0mjrpr56Pg0CK...) > to reflect proposed hierarchy. > > thanks, > -m > > > > > https://codereview.chromium.org/22295002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/10/30 16:21:32, cbentzel wrote: > I'm still worried about shell executing netsh.exe from a product > standpoint - if the UAC prompt comes up and blames Chrome once the > user clicks "grab password" it may not surprise them, but if it blames > netsh they may be concerned. > > On Wed, Oct 30, 2013 at 12:04 PM, <mailto:mef@chromium.org> wrote: > > On 2013/10/28 22:26:32, jam wrote: > >> > >> On 2013/10/28 21:43:43, Jorge Lucangeli Obes wrote: > >> > On Mon, Oct 28, 2013 at 10:57 AM, <mailto:mef@chromium.org> wrote: > >> > > >> > > Chris and Jorge will correct me if I'm wrong, but I just want to > >> > > express > >> > > my side > >> > > of the story. > >> > > > >> > > - My (possibly incorrect) understanding is that we want to minimize > >> > > amount > >> > > of > >> > > code that is running outside of sandbox. > >> > > - Process isolation is useful tool to limit exposure surface. > >> > > - The utility process is used for this purpose to isolate import of > >> > > bookmarks, > >> > > Picasa albums, iTunes Library, etc. > >> > > - So, it would appear logical to use utility process to isolate usage > >> > > of > >> > > possible harmful apis (e.g. transparent switch of wifi networks) from > >> > > browser > >> > > process. > >> > > > >> > > > >> > This is true, but what we want to sandbox is mostly processing of > >> > untrusted > >> > input. There's no processing of untrusted input in this case. What we do > >> > have is a (possible, future) operation that needs elevated privileges, > >> > which we don't want to give (and then have to take away) from a > >> > long-lived > >> > process like the browser process. > > > > > >> I see, thanks for the background. > > > > > >> In that case, let's not use the utility process until the future comes > >> when we > >> need UAC prompting. > > > > > > I've run some experiments and wlanapi calls seem to work fine from browser > > process. > > Moreover, wlanapi.dll is always loaded into it anyway (probably by > > geolocation > > module). > > Also, I was able to ShellExecute netsh.exe under UAC to obtain clear-text > > WiFi > > password. > > > > It sounds like we are willing to trade little extra safety for little lower > > complexity. > > > > Just to confirm that I understand proposed change, here is what I'm going to > > do: > > > > - Remove networking_private_handler.*, networking_private_process_client.*, > > networking_private_messages.* > > - Add networking_private_service_client.* to replace > > networking_private_process_client.* to encapsulate > > inproc lifetime and thread/task management interface between > > NetworkingPrivateApi*Functions and WiFiService. > > - Move wifi_service.* from chrome/utility/wifi/ to component/wifi/ or > > chrome/browser/extensions/api/networking_private/ (which one is better?). > > > > I'll update the design doc > > > (https://docs.google.com/a/google.com/document/d/1-QmMyvuJgjyyriT0mjrpr56Pg0CK...) > > to reflect proposed hierarchy. > > > > thanks, > > -m > > > > > > > > > > https://codereview.chromium.org/22295002/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. I agree. I think that decision should be made with input from UX and Security teams, I just wanted point out that it is possible to launch UAC-elevated process from browser process. I believe previously jorgelo@ has expressed concerns about launching utility process with elevated priviliges. If wifi password slurping is required, then we (implementors, security, UX) need to agree on few key items: - Process binary (chrome.exe, netsh.exe or some other), - IPC mechanism, - Networking Private API functions to be used for password slurping. Regardless of those cecisions I would prefer to add password slurping as separate CL. thanks, -m
SGTM On Wed, Oct 30, 2013 at 1:15 PM, <mef@chromium.org> wrote: > On 2013/10/30 16:21:32, cbentzel wrote: >> >> I'm still worried about shell executing netsh.exe from a product >> standpoint - if the UAC prompt comes up and blames Chrome once the >> user clicks "grab password" it may not surprise them, but if it blames >> netsh they may be concerned. > > >> On Wed, Oct 30, 2013 at 12:04 PM, <mailto:mef@chromium.org> wrote: >> > On 2013/10/28 22:26:32, jam wrote: >> >> >> >> On 2013/10/28 21:43:43, Jorge Lucangeli Obes wrote: >> >> > On Mon, Oct 28, 2013 at 10:57 AM, <mailto:mef@chromium.org> wrote: >> >> > >> >> > > Chris and Jorge will correct me if I'm wrong, but I just want to >> >> > > express >> >> > > my side >> >> > > of the story. >> >> > > >> >> > > - My (possibly incorrect) understanding is that we want to minimize >> >> > > amount >> >> > > of >> >> > > code that is running outside of sandbox. >> >> > > - Process isolation is useful tool to limit exposure surface. >> >> > > - The utility process is used for this purpose to isolate import of >> >> > > bookmarks, >> >> > > Picasa albums, iTunes Library, etc. >> >> > > - So, it would appear logical to use utility process to isolate >> >> > > usage >> >> > > of >> >> > > possible harmful apis (e.g. transparent switch of wifi networks) >> >> > > from >> >> > > browser >> >> > > process. >> >> > > >> >> > > >> >> > This is true, but what we want to sandbox is mostly processing of >> >> > untrusted >> >> > input. There's no processing of untrusted input in this case. What we >> >> > do >> >> > have is a (possible, future) operation that needs elevated >> >> > privileges, >> >> > which we don't want to give (and then have to take away) from a >> >> > long-lived >> >> > process like the browser process. >> > >> > >> >> I see, thanks for the background. >> > >> > >> >> In that case, let's not use the utility process until the future comes >> >> when we >> >> need UAC prompting. >> > >> > >> > I've run some experiments and wlanapi calls seem to work fine from >> > browser >> > process. >> > Moreover, wlanapi.dll is always loaded into it anyway (probably by >> > geolocation >> > module). >> > Also, I was able to ShellExecute netsh.exe under UAC to obtain >> > clear-text >> > WiFi >> > password. >> > >> > It sounds like we are willing to trade little extra safety for little >> > lower >> > complexity. >> > >> > Just to confirm that I understand proposed change, here is what I'm >> > going to >> > do: >> > >> > - Remove networking_private_handler.*, >> > networking_private_process_client.*, >> > networking_private_messages.* >> > - Add networking_private_service_client.* to replace >> > networking_private_process_client.* to encapsulate >> > inproc lifetime and thread/task management interface between >> > NetworkingPrivateApi*Functions and WiFiService. >> > - Move wifi_service.* from chrome/utility/wifi/ to component/wifi/ or >> > chrome/browser/extensions/api/networking_private/ (which one is >> > better?). >> > >> > I'll update the design doc >> > > > > (https://docs.google.com/a/google.com/document/d/1-QmMyvuJgjyyriT0mjrpr56Pg0CK...) >> >> > to reflect proposed hierarchy. >> > >> > thanks, >> > -m >> > >> > >> > >> > >> > https://codereview.chromium.org/22295002/ > > >> To unsubscribe from this group and stop receiving emails from it, send an > > email >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > > I agree. I think that decision should be made with input from UX and > Security > teams, > I just wanted point out that it is possible to launch UAC-elevated process > from > browser process. > > I believe previously jorgelo@ has expressed concerns about launching utility > process with elevated priviliges. > If wifi password slurping is required, then we (implementors, security, UX) > need > to agree on few key items: > > - Process binary (chrome.exe, netsh.exe or some other), > - IPC mechanism, > - Networking Private API functions to be used for password slurping. > > Regardless of those cecisions I would prefer to add password slurping as > separate CL. > > thanks, > -m > > > > https://codereview.chromium.org/22295002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/10/30 18:09:45, cbentzel wrote: > SGTM > > On Wed, Oct 30, 2013 at 1:15 PM, <mailto:mef@chromium.org> wrote: > > On 2013/10/30 16:21:32, cbentzel wrote: > >> > >> I'm still worried about shell executing netsh.exe from a product > >> standpoint - if the UAC prompt comes up and blames Chrome once the > >> user clicks "grab password" it may not surprise them, but if it blames > >> netsh they may be concerned. > > Chris, where are we standing wrt credentials? It seems silly for Misha to land the browser process version and immediately follow that with his previous implementation. > > > >> On Wed, Oct 30, 2013 at 12:04 PM, <mailto:mef@chromium.org> wrote: > >> > On 2013/10/28 22:26:32, jam wrote: > >> >> > >> >> On 2013/10/28 21:43:43, Jorge Lucangeli Obes wrote: > >> >> > On Mon, Oct 28, 2013 at 10:57 AM, <mailto:mef@chromium.org> wrote: > >> >> > > >> >> > > Chris and Jorge will correct me if I'm wrong, but I just want to > >> >> > > express > >> >> > > my side > >> >> > > of the story. > >> >> > > > >> >> > > - My (possibly incorrect) understanding is that we want to minimize > >> >> > > amount > >> >> > > of > >> >> > > code that is running outside of sandbox. > >> >> > > - Process isolation is useful tool to limit exposure surface. > >> >> > > - The utility process is used for this purpose to isolate import of > >> >> > > bookmarks, > >> >> > > Picasa albums, iTunes Library, etc. > >> >> > > - So, it would appear logical to use utility process to isolate > >> >> > > usage > >> >> > > of > >> >> > > possible harmful apis (e.g. transparent switch of wifi networks) > >> >> > > from > >> >> > > browser > >> >> > > process. > >> >> > > > >> >> > > > >> >> > This is true, but what we want to sandbox is mostly processing of > >> >> > untrusted > >> >> > input. There's no processing of untrusted input in this case. What we > >> >> > do > >> >> > have is a (possible, future) operation that needs elevated > >> >> > privileges, > >> >> > which we don't want to give (and then have to take away) from a > >> >> > long-lived > >> >> > process like the browser process. > >> > > >> > > >> >> I see, thanks for the background. > >> > > >> > > >> >> In that case, let's not use the utility process until the future comes > >> >> when we > >> >> need UAC prompting. > >> > > >> > > >> > I've run some experiments and wlanapi calls seem to work fine from > >> > browser > >> > process. > >> > Moreover, wlanapi.dll is always loaded into it anyway (probably by > >> > geolocation > >> > module). > >> > Also, I was able to ShellExecute netsh.exe under UAC to obtain > >> > clear-text > >> > WiFi > >> > password. > >> > > >> > It sounds like we are willing to trade little extra safety for little > >> > lower > >> > complexity. > >> > > >> > Just to confirm that I understand proposed change, here is what I'm > >> > going to > >> > do: > >> > > >> > - Remove networking_private_handler.*, > >> > networking_private_process_client.*, > >> > networking_private_messages.* > >> > - Add networking_private_service_client.* to replace > >> > networking_private_process_client.* to encapsulate > >> > inproc lifetime and thread/task management interface between > >> > NetworkingPrivateApi*Functions and WiFiService. > >> > - Move wifi_service.* from chrome/utility/wifi/ to component/wifi/ or > >> > chrome/browser/extensions/api/networking_private/ (which one is > >> > better?). > >> > > >> > I'll update the design doc > >> > > > > > > > > (https://docs.google.com/a/google.com/document/d/1-QmMyvuJgjyyriT0mjrpr56Pg0CK...) > >> > >> > to reflect proposed hierarchy. > >> > > >> > thanks, > >> > -m > >> > > >> > > >> > > >> > > >> > https://codereview.chromium.org/22295002/ > > > > > >> To unsubscribe from this group and stop receiving emails from it, send an > > > > email > >> > >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > > I agree. I think that decision should be made with input from UX and > > Security > > teams, > > I just wanted point out that it is possible to launch UAC-elevated process > > from > > browser process. > > > > I believe previously jorgelo@ has expressed concerns about launching utility > > process with elevated priviliges. > > If wifi password slurping is required, then we (implementors, security, UX) > > need > > to agree on few key items: > > > > - Process binary (chrome.exe, netsh.exe or some other), > > - IPC mechanism, > > - Networking Private API functions to be used for password slurping. > > > > Regardless of those cecisions I would prefer to add password slurping as > > separate CL. > > > > thanks, > > -m > > > > > > > > https://codereview.chromium.org/22295002/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
I think we will need credentials to support the installer app. However, I think we could restrict the use of the utility process to only that particular method in the API. Although this would still require a fair amount of the infrastructure that exists in this patch, it would reduce complexity a bit (for example, we wouldn't have to deal with keeping the process alive when event handlers are registered). The primary advantage of a separate process at this point would be possibly for stability issues - but we don't know if there are any issues here yet. On Wed, Oct 30, 2013 at 5:05 PM, <jorgelo@chromium.org> wrote: > On 2013/10/30 18:09:45, cbentzel wrote: >> >> SGTM > > >> On Wed, Oct 30, 2013 at 1:15 PM, <mailto:mef@chromium.org> wrote: >> > On 2013/10/30 16:21:32, cbentzel wrote: >> >> >> >> I'm still worried about shell executing netsh.exe from a product >> >> standpoint - if the UAC prompt comes up and blames Chrome once the >> >> user clicks "grab password" it may not surprise them, but if it blames >> >> netsh they may be concerned. >> > > > > Chris, where are we standing wrt credentials? It seems silly for Misha to > land > the browser process version and immediately follow that with his previous > implementation. > > >> > >> >> On Wed, Oct 30, 2013 at 12:04 PM, <mailto:mef@chromium.org> wrote: >> >> > On 2013/10/28 22:26:32, jam wrote: >> >> >> >> >> >> On 2013/10/28 21:43:43, Jorge Lucangeli Obes wrote: >> >> >> > On Mon, Oct 28, 2013 at 10:57 AM, <mailto:mef@chromium.org> wrote: >> >> >> > >> >> >> > > Chris and Jorge will correct me if I'm wrong, but I just want to >> >> >> > > express >> >> >> > > my side >> >> >> > > of the story. >> >> >> > > >> >> >> > > - My (possibly incorrect) understanding is that we want to >> >> >> > > minimize >> >> >> > > amount >> >> >> > > of >> >> >> > > code that is running outside of sandbox. >> >> >> > > - Process isolation is useful tool to limit exposure surface. >> >> >> > > - The utility process is used for this purpose to isolate import >> >> >> > > of >> >> >> > > bookmarks, >> >> >> > > Picasa albums, iTunes Library, etc. >> >> >> > > - So, it would appear logical to use utility process to isolate >> >> >> > > usage >> >> >> > > of >> >> >> > > possible harmful apis (e.g. transparent switch of wifi networks) >> >> >> > > from >> >> >> > > browser >> >> >> > > process. >> >> >> > > >> >> >> > > >> >> >> > This is true, but what we want to sandbox is mostly processing of >> >> >> > untrusted >> >> >> > input. There's no processing of untrusted input in this case. What >> >> >> > we >> >> >> > do >> >> >> > have is a (possible, future) operation that needs elevated >> >> >> > privileges, >> >> >> > which we don't want to give (and then have to take away) from a >> >> >> > long-lived >> >> >> > process like the browser process. >> >> > >> >> > >> >> >> I see, thanks for the background. >> >> > >> >> > >> >> >> In that case, let's not use the utility process until the future >> >> >> comes >> >> >> when we >> >> >> need UAC prompting. >> >> > >> >> > >> >> > I've run some experiments and wlanapi calls seem to work fine from >> >> > browser >> >> > process. >> >> > Moreover, wlanapi.dll is always loaded into it anyway (probably by >> >> > geolocation >> >> > module). >> >> > Also, I was able to ShellExecute netsh.exe under UAC to obtain >> >> > clear-text >> >> > WiFi >> >> > password. >> >> > >> >> > It sounds like we are willing to trade little extra safety for little >> >> > lower >> >> > complexity. >> >> > >> >> > Just to confirm that I understand proposed change, here is what I'm >> >> > going to >> >> > do: >> >> > >> >> > - Remove networking_private_handler.*, >> >> > networking_private_process_client.*, >> >> > networking_private_messages.* >> >> > - Add networking_private_service_client.* to replace >> >> > networking_private_process_client.* to encapsulate >> >> > inproc lifetime and thread/task management interface between >> >> > NetworkingPrivateApi*Functions and WiFiService. >> >> > - Move wifi_service.* from chrome/utility/wifi/ to component/wifi/ or >> >> > chrome/browser/extensions/api/networking_private/ (which one is >> >> > better?). >> >> > >> >> > I'll update the design doc >> >> > >> > >> > >> > > > > (https://docs.google.com/a/google.com/document/d/1-QmMyvuJgjyyriT0mjrpr56Pg0CK...) >> >> >> >> >> > to reflect proposed hierarchy. >> >> > >> >> > thanks, >> >> > -m >> >> > >> >> > >> >> > >> >> > >> >> > https://codereview.chromium.org/22295002/ >> > >> > >> >> To unsubscribe from this group and stop receiving emails from it, send >> >> an >> > >> > email >> >> >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > >> > >> > I agree. I think that decision should be made with input from UX and >> > Security >> > teams, >> > I just wanted point out that it is possible to launch UAC-elevated >> > process >> > from >> > browser process. >> > >> > I believe previously jorgelo@ has expressed concerns about launching >> > utility >> > process with elevated priviliges. >> > If wifi password slurping is required, then we (implementors, security, >> > UX) >> > need >> > to agree on few key items: >> > >> > - Process binary (chrome.exe, netsh.exe or some other), >> > - IPC mechanism, >> > - Networking Private API functions to be used for password slurping. >> > >> > Regardless of those cecisions I would prefer to add password slurping as >> > separate CL. >> > >> > thanks, >> > -m >> > >> > >> > >> > https://codereview.chromium.org/22295002/ > > >> To unsubscribe from this group and stop receiving emails from it, send an > > email >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > https://codereview.chromium.org/22295002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/10/30 21:16:49, cbentzel wrote: > I think we will need credentials to support the installer app. > > However, I think we could restrict the use of the utility process to > only that particular method in the API. Although this would still > require a fair amount of the infrastructure that exists in this patch, > it would reduce complexity a bit (for example, we wouldn't have to > deal with keeping the process alive when event handlers are > registered). > > The primary advantage of a separate process at this point would be > possibly for stability issues - but we don't know if there are any > issues here yet. > Well, the objections to UAC'ing in the browser process still stand, so if we need credentials we will need to use the utility process. Using the utility process *just* for that could be a reasonable compromise. > On Wed, Oct 30, 2013 at 5:05 PM, <mailto:jorgelo@chromium.org> wrote: > > On 2013/10/30 18:09:45, cbentzel wrote: > >> > >> SGTM > > > > > >> On Wed, Oct 30, 2013 at 1:15 PM, <mailto:mef@chromium.org> wrote: > >> > On 2013/10/30 16:21:32, cbentzel wrote: > >> >> > >> >> I'm still worried about shell executing netsh.exe from a product > >> >> standpoint - if the UAC prompt comes up and blames Chrome once the > >> >> user clicks "grab password" it may not surprise them, but if it blames > >> >> netsh they may be concerned. > >> > > > > > > > Chris, where are we standing wrt credentials? It seems silly for Misha to > > land > > the browser process version and immediately follow that with his previous > > implementation. > > > > > >> > > >> >> On Wed, Oct 30, 2013 at 12:04 PM, <mailto:mef@chromium.org> wrote: > >> >> > On 2013/10/28 22:26:32, jam wrote: > >> >> >> > >> >> >> On 2013/10/28 21:43:43, Jorge Lucangeli Obes wrote: > >> >> >> > On Mon, Oct 28, 2013 at 10:57 AM, <mailto:mef@chromium.org> wrote: > >> >> >> > > >> >> >> > > Chris and Jorge will correct me if I'm wrong, but I just want to > >> >> >> > > express > >> >> >> > > my side > >> >> >> > > of the story. > >> >> >> > > > >> >> >> > > - My (possibly incorrect) understanding is that we want to > >> >> >> > > minimize > >> >> >> > > amount > >> >> >> > > of > >> >> >> > > code that is running outside of sandbox. > >> >> >> > > - Process isolation is useful tool to limit exposure surface. > >> >> >> > > - The utility process is used for this purpose to isolate import > >> >> >> > > of > >> >> >> > > bookmarks, > >> >> >> > > Picasa albums, iTunes Library, etc. > >> >> >> > > - So, it would appear logical to use utility process to isolate > >> >> >> > > usage > >> >> >> > > of > >> >> >> > > possible harmful apis (e.g. transparent switch of wifi networks) > >> >> >> > > from > >> >> >> > > browser > >> >> >> > > process. > >> >> >> > > > >> >> >> > > > >> >> >> > This is true, but what we want to sandbox is mostly processing of > >> >> >> > untrusted > >> >> >> > input. There's no processing of untrusted input in this case. What > >> >> >> > we > >> >> >> > do > >> >> >> > have is a (possible, future) operation that needs elevated > >> >> >> > privileges, > >> >> >> > which we don't want to give (and then have to take away) from a > >> >> >> > long-lived > >> >> >> > process like the browser process. > >> >> > > >> >> > > >> >> >> I see, thanks for the background. > >> >> > > >> >> > > >> >> >> In that case, let's not use the utility process until the future > >> >> >> comes > >> >> >> when we > >> >> >> need UAC prompting. > >> >> > > >> >> > > >> >> > I've run some experiments and wlanapi calls seem to work fine from > >> >> > browser > >> >> > process. > >> >> > Moreover, wlanapi.dll is always loaded into it anyway (probably by > >> >> > geolocation > >> >> > module). > >> >> > Also, I was able to ShellExecute netsh.exe under UAC to obtain > >> >> > clear-text > >> >> > WiFi > >> >> > password. > >> >> > > >> >> > It sounds like we are willing to trade little extra safety for little > >> >> > lower > >> >> > complexity. > >> >> > > >> >> > Just to confirm that I understand proposed change, here is what I'm > >> >> > going to > >> >> > do: > >> >> > > >> >> > - Remove networking_private_handler.*, > >> >> > networking_private_process_client.*, > >> >> > networking_private_messages.* > >> >> > - Add networking_private_service_client.* to replace > >> >> > networking_private_process_client.* to encapsulate > >> >> > inproc lifetime and thread/task management interface between > >> >> > NetworkingPrivateApi*Functions and WiFiService. > >> >> > - Move wifi_service.* from chrome/utility/wifi/ to component/wifi/ or > >> >> > chrome/browser/extensions/api/networking_private/ (which one is > >> >> > better?). > >> >> > > >> >> > I'll update the design doc > >> >> > > >> > > >> > > >> > > > > > > > > (https://docs.google.com/a/google.com/document/d/1-QmMyvuJgjyyriT0mjrpr56Pg0CK...) > >> > >> >> > >> >> > to reflect proposed hierarchy. > >> >> > > >> >> > thanks, > >> >> > -m > >> >> > > >> >> > > >> >> > > >> >> > > >> >> > https://codereview.chromium.org/22295002/ > >> > > >> > > >> >> To unsubscribe from this group and stop receiving emails from it, send > >> >> an > >> > > >> > email > >> >> > >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. > >> > > >> > > >> > I agree. I think that decision should be made with input from UX and > >> > Security > >> > teams, > >> > I just wanted point out that it is possible to launch UAC-elevated > >> > process > >> > from > >> > browser process. > >> > > >> > I believe previously jorgelo@ has expressed concerns about launching > >> > utility > >> > process with elevated priviliges. > >> > If wifi password slurping is required, then we (implementors, security, > >> > UX) > >> > need > >> > to agree on few key items: > >> > > >> > - Process binary (chrome.exe, netsh.exe or some other), > >> > - IPC mechanism, > >> > - Networking Private API functions to be used for password slurping. > >> > > >> > Regardless of those cecisions I would prefer to add password slurping as > >> > separate CL. > >> > > >> > thanks, > >> > -m > >> > > >> > > >> > > >> > https://codereview.chromium.org/22295002/ > > > > > >> To unsubscribe from this group and stop receiving emails from it, send an > > > > email > >> > >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > > > > > > https://codereview.chromium.org/22295002/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Misha was worried that I was coupling utility process with password slurping. To be more clear: - We shouldn't do password slurping via the browser process due to priivilege escalation. - It could be done in utility process, or other processes. On Wed, Oct 30, 2013 at 5:16 PM, Chris Bentzel <cbentzel@chromium.org> wrote: > I think we will need credentials to support the installer app. > > However, I think we could restrict the use of the utility process to > only that particular method in the API. Although this would still > require a fair amount of the infrastructure that exists in this patch, > it would reduce complexity a bit (for example, we wouldn't have to > deal with keeping the process alive when event handlers are > registered). > > The primary advantage of a separate process at this point would be > possibly for stability issues - but we don't know if there are any > issues here yet. > > On Wed, Oct 30, 2013 at 5:05 PM, <jorgelo@chromium.org> wrote: >> On 2013/10/30 18:09:45, cbentzel wrote: >>> >>> SGTM >> >> >>> On Wed, Oct 30, 2013 at 1:15 PM, <mailto:mef@chromium.org> wrote: >>> > On 2013/10/30 16:21:32, cbentzel wrote: >>> >> >>> >> I'm still worried about shell executing netsh.exe from a product >>> >> standpoint - if the UAC prompt comes up and blames Chrome once the >>> >> user clicks "grab password" it may not surprise them, but if it blames >>> >> netsh they may be concerned. >>> > >> >> >> Chris, where are we standing wrt credentials? It seems silly for Misha to >> land >> the browser process version and immediately follow that with his previous >> implementation. >> >> >>> > >>> >> On Wed, Oct 30, 2013 at 12:04 PM, <mailto:mef@chromium.org> wrote: >>> >> > On 2013/10/28 22:26:32, jam wrote: >>> >> >> >>> >> >> On 2013/10/28 21:43:43, Jorge Lucangeli Obes wrote: >>> >> >> > On Mon, Oct 28, 2013 at 10:57 AM, <mailto:mef@chromium.org> wrote: >>> >> >> > >>> >> >> > > Chris and Jorge will correct me if I'm wrong, but I just want to >>> >> >> > > express >>> >> >> > > my side >>> >> >> > > of the story. >>> >> >> > > >>> >> >> > > - My (possibly incorrect) understanding is that we want to >>> >> >> > > minimize >>> >> >> > > amount >>> >> >> > > of >>> >> >> > > code that is running outside of sandbox. >>> >> >> > > - Process isolation is useful tool to limit exposure surface. >>> >> >> > > - The utility process is used for this purpose to isolate import >>> >> >> > > of >>> >> >> > > bookmarks, >>> >> >> > > Picasa albums, iTunes Library, etc. >>> >> >> > > - So, it would appear logical to use utility process to isolate >>> >> >> > > usage >>> >> >> > > of >>> >> >> > > possible harmful apis (e.g. transparent switch of wifi networks) >>> >> >> > > from >>> >> >> > > browser >>> >> >> > > process. >>> >> >> > > >>> >> >> > > >>> >> >> > This is true, but what we want to sandbox is mostly processing of >>> >> >> > untrusted >>> >> >> > input. There's no processing of untrusted input in this case. What >>> >> >> > we >>> >> >> > do >>> >> >> > have is a (possible, future) operation that needs elevated >>> >> >> > privileges, >>> >> >> > which we don't want to give (and then have to take away) from a >>> >> >> > long-lived >>> >> >> > process like the browser process. >>> >> > >>> >> > >>> >> >> I see, thanks for the background. >>> >> > >>> >> > >>> >> >> In that case, let's not use the utility process until the future >>> >> >> comes >>> >> >> when we >>> >> >> need UAC prompting. >>> >> > >>> >> > >>> >> > I've run some experiments and wlanapi calls seem to work fine from >>> >> > browser >>> >> > process. >>> >> > Moreover, wlanapi.dll is always loaded into it anyway (probably by >>> >> > geolocation >>> >> > module). >>> >> > Also, I was able to ShellExecute netsh.exe under UAC to obtain >>> >> > clear-text >>> >> > WiFi >>> >> > password. >>> >> > >>> >> > It sounds like we are willing to trade little extra safety for little >>> >> > lower >>> >> > complexity. >>> >> > >>> >> > Just to confirm that I understand proposed change, here is what I'm >>> >> > going to >>> >> > do: >>> >> > >>> >> > - Remove networking_private_handler.*, >>> >> > networking_private_process_client.*, >>> >> > networking_private_messages.* >>> >> > - Add networking_private_service_client.* to replace >>> >> > networking_private_process_client.* to encapsulate >>> >> > inproc lifetime and thread/task management interface between >>> >> > NetworkingPrivateApi*Functions and WiFiService. >>> >> > - Move wifi_service.* from chrome/utility/wifi/ to component/wifi/ or >>> >> > chrome/browser/extensions/api/networking_private/ (which one is >>> >> > better?). >>> >> > >>> >> > I'll update the design doc >>> >> > >>> > >>> > >>> > >> >> >> (https://docs.google.com/a/google.com/document/d/1-QmMyvuJgjyyriT0mjrpr56Pg0CK...) >>> >>> >> >>> >> > to reflect proposed hierarchy. >>> >> > >>> >> > thanks, >>> >> > -m >>> >> > >>> >> > >>> >> > >>> >> > >>> >> > https://codereview.chromium.org/22295002/ >>> > >>> > >>> >> To unsubscribe from this group and stop receiving emails from it, send >>> >> an >>> > >>> > email >>> >> >>> >> to mailto:chromium-reviews+unsubscribe@chromium.org. >>> > >>> > >>> > I agree. I think that decision should be made with input from UX and >>> > Security >>> > teams, >>> > I just wanted point out that it is possible to launch UAC-elevated >>> > process >>> > from >>> > browser process. >>> > >>> > I believe previously jorgelo@ has expressed concerns about launching >>> > utility >>> > process with elevated priviliges. >>> > If wifi password slurping is required, then we (implementors, security, >>> > UX) >>> > need >>> > to agree on few key items: >>> > >>> > - Process binary (chrome.exe, netsh.exe or some other), >>> > - IPC mechanism, >>> > - Networking Private API functions to be used for password slurping. >>> > >>> > Regardless of those cecisions I would prefer to add password slurping as >>> > separate CL. >>> > >>> > thanks, >>> > -m >>> > >>> > >>> > >>> > https://codereview.chromium.org/22295002/ >> >> >>> To unsubscribe from this group and stop receiving emails from it, send an >> >> email >>> >>> to mailto:chromium-reviews+unsubscribe@chromium.org. >> >> >> >> >> https://codereview.chromium.org/22295002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Utility process or other short lived child process. Complete agreement there. On Wed, Oct 30, 2013 at 5:20 PM, <jorgelo@chromium.org> wrote: > On 2013/10/30 21:16:49, cbentzel wrote: >> >> I think we will need credentials to support the installer app. > > >> However, I think we could restrict the use of the utility process to >> only that particular method in the API. Although this would still >> require a fair amount of the infrastructure that exists in this patch, >> it would reduce complexity a bit (for example, we wouldn't have to >> deal with keeping the process alive when event handlers are >> registered). > > >> The primary advantage of a separate process at this point would be >> possibly for stability issues - but we don't know if there are any >> issues here yet. > > > > Well, the objections to UAC'ing in the browser process still stand, so if we > need credentials we will need to use the utility process. > > Using the utility process *just* for that could be a reasonable compromise. > >> On Wed, Oct 30, 2013 at 5:05 PM, <mailto:jorgelo@chromium.org> wrote: >> > On 2013/10/30 18:09:45, cbentzel wrote: >> >> >> >> SGTM >> > >> > >> >> On Wed, Oct 30, 2013 at 1:15 PM, <mailto:mef@chromium.org> wrote: >> >> > On 2013/10/30 16:21:32, cbentzel wrote: >> >> >> >> >> >> I'm still worried about shell executing netsh.exe from a product >> >> >> standpoint - if the UAC prompt comes up and blames Chrome once the >> >> >> user clicks "grab password" it may not surprise them, but if it >> >> >> blames >> >> >> netsh they may be concerned. >> >> > >> > >> > >> > Chris, where are we standing wrt credentials? It seems silly for Misha >> > to >> > land >> > the browser process version and immediately follow that with his >> > previous >> > implementation. >> > >> > >> >> > >> >> >> On Wed, Oct 30, 2013 at 12:04 PM, <mailto:mef@chromium.org> wrote: >> >> >> > On 2013/10/28 22:26:32, jam wrote: >> >> >> >> >> >> >> >> On 2013/10/28 21:43:43, Jorge Lucangeli Obes wrote: >> >> >> >> > On Mon, Oct 28, 2013 at 10:57 AM, <mailto:mef@chromium.org> >> >> >> >> > wrote: >> >> >> >> > >> >> >> >> > > Chris and Jorge will correct me if I'm wrong, but I just want >> >> >> >> > > to >> >> >> >> > > express >> >> >> >> > > my side >> >> >> >> > > of the story. >> >> >> >> > > >> >> >> >> > > - My (possibly incorrect) understanding is that we want to >> >> >> >> > > minimize >> >> >> >> > > amount >> >> >> >> > > of >> >> >> >> > > code that is running outside of sandbox. >> >> >> >> > > - Process isolation is useful tool to limit exposure surface. >> >> >> >> > > - The utility process is used for this purpose to isolate >> >> >> >> > > import >> >> >> >> > > of >> >> >> >> > > bookmarks, >> >> >> >> > > Picasa albums, iTunes Library, etc. >> >> >> >> > > - So, it would appear logical to use utility process to >> >> >> >> > > isolate >> >> >> >> > > usage >> >> >> >> > > of >> >> >> >> > > possible harmful apis (e.g. transparent switch of wifi >> >> >> >> > > networks) >> >> >> >> > > from >> >> >> >> > > browser >> >> >> >> > > process. >> >> >> >> > > >> >> >> >> > > >> >> >> >> > This is true, but what we want to sandbox is mostly processing >> >> >> >> > of >> >> >> >> > untrusted >> >> >> >> > input. There's no processing of untrusted input in this case. >> >> >> >> > What >> >> >> >> > we >> >> >> >> > do >> >> >> >> > have is a (possible, future) operation that needs elevated >> >> >> >> > privileges, >> >> >> >> > which we don't want to give (and then have to take away) from a >> >> >> >> > long-lived >> >> >> >> > process like the browser process. >> >> >> > >> >> >> > >> >> >> >> I see, thanks for the background. >> >> >> > >> >> >> > >> >> >> >> In that case, let's not use the utility process until the future >> >> >> >> comes >> >> >> >> when we >> >> >> >> need UAC prompting. >> >> >> > >> >> >> > >> >> >> > I've run some experiments and wlanapi calls seem to work fine from >> >> >> > browser >> >> >> > process. >> >> >> > Moreover, wlanapi.dll is always loaded into it anyway (probably by >> >> >> > geolocation >> >> >> > module). >> >> >> > Also, I was able to ShellExecute netsh.exe under UAC to obtain >> >> >> > clear-text >> >> >> > WiFi >> >> >> > password. >> >> >> > >> >> >> > It sounds like we are willing to trade little extra safety for >> >> >> > little >> >> >> > lower >> >> >> > complexity. >> >> >> > >> >> >> > Just to confirm that I understand proposed change, here is what >> >> >> > I'm >> >> >> > going to >> >> >> > do: >> >> >> > >> >> >> > - Remove networking_private_handler.*, >> >> >> > networking_private_process_client.*, >> >> >> > networking_private_messages.* >> >> >> > - Add networking_private_service_client.* to replace >> >> >> > networking_private_process_client.* to encapsulate >> >> >> > inproc lifetime and thread/task management interface between >> >> >> > NetworkingPrivateApi*Functions and WiFiService. >> >> >> > - Move wifi_service.* from chrome/utility/wifi/ to component/wifi/ >> >> >> > or >> >> >> > chrome/browser/extensions/api/networking_private/ (which one is >> >> >> > better?). >> >> >> > >> >> >> > I'll update the design doc >> >> >> > >> >> > >> >> > >> >> > >> > >> > >> > > > > (https://docs.google.com/a/google.com/document/d/1-QmMyvuJgjyyriT0mjrpr56Pg0CK...) >> >> >> >> >> >> >> >> >> > to reflect proposed hierarchy. >> >> >> > >> >> >> > thanks, >> >> >> > -m >> >> >> > >> >> >> > >> >> >> > >> >> >> > >> >> >> > https://codereview.chromium.org/22295002/ >> >> > >> >> > >> >> >> To unsubscribe from this group and stop receiving emails from it, >> >> >> send >> >> >> an >> >> > >> >> > email >> >> >> >> >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> >> > >> >> > >> >> > I agree. I think that decision should be made with input from UX and >> >> > Security >> >> > teams, >> >> > I just wanted point out that it is possible to launch UAC-elevated >> >> > process >> >> > from >> >> > browser process. >> >> > >> >> > I believe previously jorgelo@ has expressed concerns about launching >> >> > utility >> >> > process with elevated priviliges. >> >> > If wifi password slurping is required, then we (implementors, >> >> > security, >> >> > UX) >> >> > need >> >> > to agree on few key items: >> >> > >> >> > - Process binary (chrome.exe, netsh.exe or some other), >> >> > - IPC mechanism, >> >> > - Networking Private API functions to be used for password slurping. >> >> > >> >> > Regardless of those cecisions I would prefer to add password slurping >> >> > as >> >> > separate CL. >> >> > >> >> > thanks, >> >> > -m >> >> > >> >> > >> >> > >> >> > https://codereview.chromium.org/22295002/ >> > >> > >> >> To unsubscribe from this group and stop receiving emails from it, send >> >> an >> > >> > email >> >> >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. >> > >> > >> > >> > >> > https://codereview.chromium.org/22295002/ > > >> To unsubscribe from this group and stop receiving emails from it, send an > > email >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > https://codereview.chromium.org/22295002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2013/10/31 13:59:44, cbentzel wrote: > Utility process or other short lived child process. Complete agreement there. > > On Wed, Oct 30, 2013 at 5:20 PM, <mailto:jorgelo@chromium.org> wrote: > > On 2013/10/30 21:16:49, cbentzel wrote: > >> > >> I think we will need credentials to support the installer app. > > > > > >> However, I think we could restrict the use of the utility process to > >> only that particular method in the API. Although this would still > >> require a fair amount of the infrastructure that exists in this patch, > >> it would reduce complexity a bit (for example, we wouldn't have to > >> deal with keeping the process alive when event handlers are > >> registered). > > > > > >> The primary advantage of a separate process at this point would be > >> possibly for stability issues - but we don't know if there are any > >> issues here yet. > > > > > > > > Well, the objections to UAC'ing in the browser process still stand, so if we > > need credentials we will need to use the utility process. > > > > Using the utility process *just* for that could be a reasonable compromise. > > > >> On Wed, Oct 30, 2013 at 5:05 PM, <mailto:jorgelo@chromium.org> wrote: > >> > On 2013/10/30 18:09:45, cbentzel wrote: > >> >> > >> >> SGTM > >> > > >> > > >> >> On Wed, Oct 30, 2013 at 1:15 PM, <mailto:mef@chromium.org> wrote: > >> >> > On 2013/10/30 16:21:32, cbentzel wrote: > >> >> >> > >> >> >> I'm still worried about shell executing netsh.exe from a product > >> >> >> standpoint - if the UAC prompt comes up and blames Chrome once the > >> >> >> user clicks "grab password" it may not surprise them, but if it > >> >> >> blames > >> >> >> netsh they may be concerned. > >> >> > > >> > > >> > > >> > Chris, where are we standing wrt credentials? It seems silly for Misha > >> > to > >> > land > >> > the browser process version and immediately follow that with his > >> > previous > >> > implementation. > >> > > >> > > >> >> > > >> >> >> On Wed, Oct 30, 2013 at 12:04 PM, <mailto:mef@chromium.org> wrote: > >> >> >> > On 2013/10/28 22:26:32, jam wrote: > >> >> >> >> > >> >> >> >> On 2013/10/28 21:43:43, Jorge Lucangeli Obes wrote: > >> >> >> >> > On Mon, Oct 28, 2013 at 10:57 AM, <mailto:mef@chromium.org> > >> >> >> >> > wrote: > >> >> >> >> > > >> >> >> >> > > Chris and Jorge will correct me if I'm wrong, but I just want > >> >> >> >> > > to > >> >> >> >> > > express > >> >> >> >> > > my side > >> >> >> >> > > of the story. > >> >> >> >> > > > >> >> >> >> > > - My (possibly incorrect) understanding is that we want to > >> >> >> >> > > minimize > >> >> >> >> > > amount > >> >> >> >> > > of > >> >> >> >> > > code that is running outside of sandbox. > >> >> >> >> > > - Process isolation is useful tool to limit exposure surface. > >> >> >> >> > > - The utility process is used for this purpose to isolate > >> >> >> >> > > import > >> >> >> >> > > of > >> >> >> >> > > bookmarks, > >> >> >> >> > > Picasa albums, iTunes Library, etc. > >> >> >> >> > > - So, it would appear logical to use utility process to > >> >> >> >> > > isolate > >> >> >> >> > > usage > >> >> >> >> > > of > >> >> >> >> > > possible harmful apis (e.g. transparent switch of wifi > >> >> >> >> > > networks) > >> >> >> >> > > from > >> >> >> >> > > browser > >> >> >> >> > > process. > >> >> >> >> > > > >> >> >> >> > > > >> >> >> >> > This is true, but what we want to sandbox is mostly processing > >> >> >> >> > of > >> >> >> >> > untrusted > >> >> >> >> > input. There's no processing of untrusted input in this case. > >> >> >> >> > What > >> >> >> >> > we > >> >> >> >> > do > >> >> >> >> > have is a (possible, future) operation that needs elevated > >> >> >> >> > privileges, > >> >> >> >> > which we don't want to give (and then have to take away) from a > >> >> >> >> > long-lived > >> >> >> >> > process like the browser process. > >> >> >> > > >> >> >> > > >> >> >> >> I see, thanks for the background. > >> >> >> > > >> >> >> > > >> >> >> >> In that case, let's not use the utility process until the future > >> >> >> >> comes > >> >> >> >> when we > >> >> >> >> need UAC prompting. > >> >> >> > > >> >> >> > > >> >> >> > I've run some experiments and wlanapi calls seem to work fine from > >> >> >> > browser > >> >> >> > process. > >> >> >> > Moreover, wlanapi.dll is always loaded into it anyway (probably by > >> >> >> > geolocation > >> >> >> > module). > >> >> >> > Also, I was able to ShellExecute netsh.exe under UAC to obtain > >> >> >> > clear-text > >> >> >> > WiFi > >> >> >> > password. > >> >> >> > > >> >> >> > It sounds like we are willing to trade little extra safety for > >> >> >> > little > >> >> >> > lower > >> >> >> > complexity. > >> >> >> > > >> >> >> > Just to confirm that I understand proposed change, here is what > >> >> >> > I'm > >> >> >> > going to > >> >> >> > do: > >> >> >> > > >> >> >> > - Remove networking_private_handler.*, > >> >> >> > networking_private_process_client.*, > >> >> >> > networking_private_messages.* > >> >> >> > - Add networking_private_service_client.* to replace > >> >> >> > networking_private_process_client.* to encapsulate > >> >> >> > inproc lifetime and thread/task management interface between > >> >> >> > NetworkingPrivateApi*Functions and WiFiService. > >> >> >> > - Move wifi_service.* from chrome/utility/wifi/ to component/wifi/ > >> >> >> > or > >> >> >> > chrome/browser/extensions/api/networking_private/ (which one is > >> >> >> > better?). > >> >> >> > > >> >> >> > I'll update the design doc > >> >> >> > > >> >> > > >> >> > > >> >> > > >> > > >> > > >> > > > > > > > > (https://docs.google.com/a/google.com/document/d/1-QmMyvuJgjyyriT0mjrpr56Pg0CK...) > >> > >> >> > >> >> >> > >> >> >> > to reflect proposed hierarchy. > >> >> >> > > >> >> >> > thanks, > >> >> >> > -m > >> >> >> > > >> >> >> > > >> >> >> > > >> >> >> > > >> >> >> > https://codereview.chromium.org/22295002/ > >> >> > > >> >> > > >> >> >> To unsubscribe from this group and stop receiving emails from it, > >> >> >> send > >> >> >> an > >> >> > > >> >> > email > >> >> >> > >> >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. > >> >> > > >> >> > > >> >> > I agree. I think that decision should be made with input from UX and > >> >> > Security > >> >> > teams, > >> >> > I just wanted point out that it is possible to launch UAC-elevated > >> >> > process > >> >> > from > >> >> > browser process. > >> >> > > >> >> > I believe previously jorgelo@ has expressed concerns about launching > >> >> > utility > >> >> > process with elevated priviliges. > >> >> > If wifi password slurping is required, then we (implementors, > >> >> > security, > >> >> > UX) > >> >> > need > >> >> > to agree on few key items: > >> >> > > >> >> > - Process binary (chrome.exe, netsh.exe or some other), > >> >> > - IPC mechanism, > >> >> > - Networking Private API functions to be used for password slurping. > >> >> > > >> >> > Regardless of those cecisions I would prefer to add password slurping > >> >> > as > >> >> > separate CL. > >> >> > > >> >> > thanks, > >> >> > -m > >> >> > > >> >> > > >> >> > > >> >> > https://codereview.chromium.org/22295002/ > >> > > >> > > >> >> To unsubscribe from this group and stop receiving emails from it, send > >> >> an > >> > > >> > email > >> >> > >> >> to mailto:chromium-reviews+unsubscribe@chromium.org. > >> > > >> > > >> > > >> > > >> > https://codereview.chromium.org/22295002/ > > > > > >> To unsubscribe from this group and stop receiving emails from it, send an > > > > email > >> > >> to mailto:chromium-reviews+unsubscribe@chromium.org. > > > > > > > > > > https://codereview.chromium.org/22295002/ > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Hi, please take a look @ https://codereview.chromium.org/54323003. I've changed WiFiService to run in browser process instead of utility process. I've also moved it from chrome/utility/wifi to components/wifi. I've created a separate CL (https://codereview.chromium.org/54323003/) for easier review, but will be happy to merge it back into this one if that makes sense. I haven't yet updated windows-specific implementation to match this. thanks, -m |
