Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12517011:
Added activity logging for ext APIs with custom bindings (Closed)
|
Created:
7 years, 9 months ago by felt Modified:
7 years, 9 months ago CC:
chromium-reviews, Aaron Boodman, darin-cc_chromium.org, chromium-apps-reviews_chromium.org, Eric Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdded activity logging for ext APIs with custom bindings
The Activity Log wasn't catching chrome.* API calls if they went through custom bindings. This CL fixes that and adds a test case.
BUG=39802
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190571
Patch Set 1 #Patch Set 2 : lint fix #
Total comments: 7
Patch Set 3 : Modified it to work with another CL that happened while I was working on it #
Total comments: 2
Patch Set 4 : Modified setHandleRequest to avoid double logging #
Total comments: 12
Patch Set 5 : Merged with conflicting change (RouteStaticFunction to RouteFunction) #Patch Set 6 : Added a flag to sendRequest #Patch Set 7 : Missed the removal of two booleans #
Total comments: 6
Patch Set 8 : Renamed some variables #
Total comments: 1
Patch Set 9 : typo #
Total comments: 6
Patch Set 10 : Added an earlier check for the IPC communication #
Total comments: 5
Patch Set 11 : removed constant left over from testing #Patch Set 12 : Somehow a file got removed from Patchset 11 -- adding the file back in #Patch Set 13 : Yet another file disappeared from the patchset #Patch Set 14 : Rebased #Messages
Total messages: 33 (0 generated)
Hi Matt, Thanks for the tips on what was causing us to miss some logging actions. This should fix it. Adrienne
https://codereview.chromium.org/12517011/diff/1014/chrome/renderer/extensions... File chrome/renderer/extensions/api_activity_logger.cc (right): https://codereview.chromium.org/12517011/diff/1014/chrome/renderer/extensions... chrome/renderer/extensions/api_activity_logger.cc:40: scoped_ptr<V8ValueConverter> converter(V8ValueConverter::create()); converter->FromV8Value(args[2]) should work and return a ListValue. https://codereview.chromium.org/12517011/diff/1014/chrome/renderer/extensions... chrome/renderer/extensions/api_activity_logger.cc:50: // Initiate the IPC process. nit: superfluous comment https://codereview.chromium.org/12517011/diff/1014/chrome/renderer/resources/... File chrome/renderer/resources/extensions/schema_generated_bindings.js (right): https://codereview.chromium.org/12517011/diff/1014/chrome/renderer/resources/... chrome/renderer/resources/extensions/schema_generated_bindings.js:59: logActivity(ext_id, apiName, Unfortunately, some APIs that use setHandleRequest still use the ExtensionFunction machinery too. This will double-count those. See for example https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re... Anything that calls sendRequest is using an ExtensionFunction. I can think of a few ideas. From the hacky: - Have sendRequest set a global variable, and only log if that var is not set. to the slightly better but still ugly: - Add a setHandleRequestLocally (or something) for APIs that don't call sendRequest, and only log from there. There's no good way to enforce they don't call sendRequest, though. Do you have any ideas? https://codereview.chromium.org/12517011/diff/20001/chrome/renderer/resources... File chrome/renderer/resources/extensions/binding.js (right): https://codereview.chromium.org/12517011/diff/20001/chrome/renderer/resources... chrome/renderer/resources/extensions/binding.js:53: var ext_id = extensionId; is this temporary needed?
https://codereview.chromium.org/12517011/diff/1014/chrome/renderer/extensions... File chrome/renderer/extensions/api_activity_logger.cc (right): https://codereview.chromium.org/12517011/diff/1014/chrome/renderer/extensions... chrome/renderer/extensions/api_activity_logger.cc:40: scoped_ptr<V8ValueConverter> converter(V8ValueConverter::create()); On 2013/03/14 20:32:31, Matt Perry wrote: > converter->FromV8Value(args[2]) should work and return a ListValue. converter->FromV8Value(args[2], v8::Context::GetCurrent()) returns a base::Value*, not a ListValue; so I'm not sure that works. https://codereview.chromium.org/12517011/diff/1014/chrome/renderer/extensions... chrome/renderer/extensions/api_activity_logger.cc:50: // Initiate the IPC process. On 2013/03/14 20:32:31, Matt Perry wrote: > nit: superfluous comment Done. https://codereview.chromium.org/12517011/diff/1014/chrome/renderer/resources/... File chrome/renderer/resources/extensions/schema_generated_bindings.js (right): https://codereview.chromium.org/12517011/diff/1014/chrome/renderer/resources/... chrome/renderer/resources/extensions/schema_generated_bindings.js:59: logActivity(ext_id, apiName, Good catch. OK, I did a version of the second one. I added a boolean argument to setHandleRequest. If the extra arg is false or missing, it logs; if it's true, it doesn't log. This has two properties: (1) Someone who is copying code to use setHandleRequest will see the boolean. If we made a new method like setHandleRequestDontLog, people might not see it. (2) If I missed an invocation to setHandleRequest, or someone in the future forgets the boolean, we don't start missing log entries; instead, the worst case is that an event gets logged twice. On 2013/03/14 20:32:31, Matt Perry wrote: > Unfortunately, some APIs that use setHandleRequest still use the > ExtensionFunction machinery too. This will double-count those. See for example > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re... > > Anything that calls sendRequest is using an ExtensionFunction. > > I can think of a few ideas. From the hacky: > - Have sendRequest set a global variable, and only log if that var is not set. > > to the slightly better but still ugly: > - Add a setHandleRequestLocally (or something) for APIs that don't call > sendRequest, and only log from there. There's no good way to enforce they don't > call sendRequest, though. > > Do you have any ideas? https://codereview.chromium.org/12517011/diff/20001/chrome/renderer/resources... File chrome/renderer/resources/extensions/binding.js (right): https://codereview.chromium.org/12517011/diff/20001/chrome/renderer/resources... chrome/renderer/resources/extensions/binding.js:53: var ext_id = extensionId; On 2013/03/14 20:32:31, Matt Perry wrote: > is this temporary needed? Done.
FYI I also added a testcase for the double logging in options.js. On 2013/03/15 00:04:47, felt wrote: > https://codereview.chromium.org/12517011/diff/1014/chrome/renderer/extensions... > File chrome/renderer/extensions/api_activity_logger.cc (right): > > https://codereview.chromium.org/12517011/diff/1014/chrome/renderer/extensions... > chrome/renderer/extensions/api_activity_logger.cc:40: > scoped_ptr<V8ValueConverter> converter(V8ValueConverter::create()); > On 2013/03/14 20:32:31, Matt Perry wrote: > > converter->FromV8Value(args[2]) should work and return a ListValue. > > converter->FromV8Value(args[2], v8::Context::GetCurrent()) returns a > base::Value*, not a ListValue; so I'm not sure that works. > > https://codereview.chromium.org/12517011/diff/1014/chrome/renderer/extensions... > chrome/renderer/extensions/api_activity_logger.cc:50: // Initiate the IPC > process. > On 2013/03/14 20:32:31, Matt Perry wrote: > > nit: superfluous comment > > Done. > > https://codereview.chromium.org/12517011/diff/1014/chrome/renderer/resources/... > File chrome/renderer/resources/extensions/schema_generated_bindings.js (right): > > https://codereview.chromium.org/12517011/diff/1014/chrome/renderer/resources/... > chrome/renderer/resources/extensions/schema_generated_bindings.js:59: > logActivity(ext_id, apiName, > Good catch. OK, I did a version of the second one. I added a boolean argument > to setHandleRequest. If the extra arg is false or missing, it logs; if it's > true, it doesn't log. This has two properties: > > (1) Someone who is copying code to use setHandleRequest will see the boolean. If > we made a new method like setHandleRequestDontLog, people might not see it. > (2) If I missed an invocation to setHandleRequest, or someone in the future > forgets the boolean, we don't start missing log entries; instead, the worst case > is that an event gets logged twice. > > > On 2013/03/14 20:32:31, Matt Perry wrote: > > Unfortunately, some APIs that use setHandleRequest still use the > > ExtensionFunction machinery too. This will double-count those. See for example > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re... > > > > Anything that calls sendRequest is using an ExtensionFunction. > > > > I can think of a few ideas. From the hacky: > > - Have sendRequest set a global variable, and only log if that var is not set. > > > > to the slightly better but still ugly: > > - Add a setHandleRequestLocally (or something) for APIs that don't call > > sendRequest, and only log from there. There's no good way to enforce they > don't > > call sendRequest, though. > > > > Do you have any ideas? > > https://codereview.chromium.org/12517011/diff/20001/chrome/renderer/resources... > File chrome/renderer/resources/extensions/binding.js (right): > > https://codereview.chromium.org/12517011/diff/20001/chrome/renderer/resources... > chrome/renderer/resources/extensions/binding.js:53: var ext_id = extensionId; > On 2013/03/14 20:32:31, Matt Perry wrote: > > is this temporary needed? > > Done.
After seeing how easy it is to get it wrong about whether sendRequest is called, I'm thinking the global variable option is the best route. I pointed out in the code where the entry and exit points are, where you would clear the global var. Put the var on chromeHidden so pages don't have access to it. It's hacky, but it's more robust in this case. https://codereview.chromium.org/12517011/diff/1014/chrome/renderer/extensions... File chrome/renderer/extensions/api_activity_logger.cc (right): https://codereview.chromium.org/12517011/diff/1014/chrome/renderer/extensions... chrome/renderer/extensions/api_activity_logger.cc:40: scoped_ptr<V8ValueConverter> converter(V8ValueConverter::create()); On 2013/03/15 00:04:47, felt wrote: > On 2013/03/14 20:32:31, Matt Perry wrote: > > converter->FromV8Value(args[2]) should work and return a ListValue. > > converter->FromV8Value(args[2], v8::Context::GetCurrent()) returns a > base::Value*, not a ListValue; so I'm not sure that works. If args[2] is a v8::Array, the Value* will be of type ListValue. (You can use GetAsList() to check). https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... File chrome/renderer/resources/extensions/app_window_custom_bindings.js (right): https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... chrome/renderer/resources/extensions/app_window_custom_bindings.js:130: }, true); I don't see where this one calls sendRequest. https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... File chrome/renderer/resources/extensions/binding.js (right): https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... chrome/renderer/resources/extensions/binding.js:303: (clear global here) https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... chrome/renderer/resources/extensions/binding.js:315: (clear global here) https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... File chrome/renderer/resources/extensions/browser_action_custom_bindings.js (right): https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... chrome/renderer/resources/extensions/browser_action_custom_bindings.js:17: }, false); setIcon calls sendRequest behind the scenes... Confused yet? :) https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... File chrome/renderer/resources/extensions/extension_custom_bindings.js (right): https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... chrome/renderer/resources/extensions/extension_custom_bindings.js:101: }, false); This one is the messaging API - do you plan on logging that separately? https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... File chrome/renderer/resources/extensions/file_browser_handler_custom_bindings.js (right): https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... chrome/renderer/resources/extensions/file_browser_handler_custom_bindings.js:55: }, false); selectFile uses sendRequest. (We have a handful of custom APIs that call through to internal APIs after some fiddling. The internal APIs are generally implemented with sendRequest.) https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... File chrome/renderer/resources/extensions/notifications_custom_bindings.js (right): https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... chrome/renderer/resources/extensions/notifications_custom_bindings.js:124: apiFunctions.setHandleRequest('update', handleCreate, false); Both of these use sendRequest... sometimes. See genHandle. https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... File chrome/renderer/resources/extensions/page_action_custom_bindings.js (right): https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... chrome/renderer/resources/extensions/page_action_custom_bindings.js:17: }, false); setIcon https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... File chrome/renderer/resources/extensions/system_indicator_custom_bindings.js (right): https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... chrome/renderer/resources/extensions/system_indicator_custom_bindings.js:20: }, false); setIcon
Yikes - I suppose I should have followed the call graphs for setIcon etc., but there were a lot of them so I'd just looked at the immediate method. Is this a mistake a Chrome extension developer (i.e., someone who would normally be editing the code) would make? If not, perhaps I should just fix the mistakes; if so, I'll implement the global variable option. On 2013/03/15 17:51:06, Matt Perry wrote: > After seeing how easy it is to get it wrong about whether sendRequest is called, > I'm thinking the global variable option is the best route. I pointed out in the > code where the entry and exit points are, where you would clear the global var. > Put the var on chromeHidden so pages don't have access to it. It's hacky, but > it's more robust in this case. > > https://codereview.chromium.org/12517011/diff/1014/chrome/renderer/extensions... > File chrome/renderer/extensions/api_activity_logger.cc (right): > > https://codereview.chromium.org/12517011/diff/1014/chrome/renderer/extensions... > chrome/renderer/extensions/api_activity_logger.cc:40: > scoped_ptr<V8ValueConverter> converter(V8ValueConverter::create()); > On 2013/03/15 00:04:47, felt wrote: > > On 2013/03/14 20:32:31, Matt Perry wrote: > > > converter->FromV8Value(args[2]) should work and return a ListValue. > > > > converter->FromV8Value(args[2], v8::Context::GetCurrent()) returns a > > base::Value*, not a ListValue; so I'm not sure that works. > > If args[2] is a v8::Array, the Value* will be of type ListValue. (You can use > GetAsList() to check). > > https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... > File chrome/renderer/resources/extensions/app_window_custom_bindings.js (right): > > https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... > chrome/renderer/resources/extensions/app_window_custom_bindings.js:130: }, > true); > I don't see where this one calls sendRequest. > > https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... > File chrome/renderer/resources/extensions/binding.js (right): > > https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... > chrome/renderer/resources/extensions/binding.js:303: > (clear global here) > > https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... > chrome/renderer/resources/extensions/binding.js:315: > (clear global here) > > https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... > File chrome/renderer/resources/extensions/browser_action_custom_bindings.js > (right): > > https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... > chrome/renderer/resources/extensions/browser_action_custom_bindings.js:17: }, > false); > setIcon calls sendRequest behind the scenes... Confused yet? :) > > https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... > File chrome/renderer/resources/extensions/extension_custom_bindings.js (right): > > https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... > chrome/renderer/resources/extensions/extension_custom_bindings.js:101: }, > false); > This one is the messaging API - do you plan on logging that separately? > > https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... > File > chrome/renderer/resources/extensions/file_browser_handler_custom_bindings.js > (right): > > https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... > chrome/renderer/resources/extensions/file_browser_handler_custom_bindings.js:55: > }, false); > selectFile uses sendRequest. (We have a handful of custom APIs that call through > to internal APIs after some fiddling. The internal APIs are generally > implemented with sendRequest.) > > https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... > File chrome/renderer/resources/extensions/notifications_custom_bindings.js > (right): > > https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... > chrome/renderer/resources/extensions/notifications_custom_bindings.js:124: > apiFunctions.setHandleRequest('update', handleCreate, false); > Both of these use sendRequest... sometimes. See genHandle. > > https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... > File chrome/renderer/resources/extensions/page_action_custom_bindings.js > (right): > > https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... > chrome/renderer/resources/extensions/page_action_custom_bindings.js:17: }, > false); > setIcon > > https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... > File chrome/renderer/resources/extensions/system_indicator_custom_bindings.js > (right): > > https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... > chrome/renderer/resources/extensions/system_indicator_custom_bindings.js:20: }, > false); > setIcon
It's definitely an easy mistake to make - I'm sure I've missed some of the callsites in the review. I recommend the global variable option. On 2013/03/15 20:03:51, felt wrote: > Yikes - I suppose I should have followed the call graphs for setIcon etc., but > there were a lot of them so I'd just looked at the immediate method. Is this a > mistake a Chrome extension developer (i.e., someone who would normally be > editing the code) would make? If not, perhaps I should just fix the mistakes; if > so, I'll implement the global variable option. > > On 2013/03/15 17:51:06, Matt Perry wrote: > > After seeing how easy it is to get it wrong about whether sendRequest is > called, > > I'm thinking the global variable option is the best route. I pointed out in > the > > code where the entry and exit points are, where you would clear the global > var. > > Put the var on chromeHidden so pages don't have access to it. It's hacky, but > > it's more robust in this case. > > > > > https://codereview.chromium.org/12517011/diff/1014/chrome/renderer/extensions... > > File chrome/renderer/extensions/api_activity_logger.cc (right): > > > > > https://codereview.chromium.org/12517011/diff/1014/chrome/renderer/extensions... > > chrome/renderer/extensions/api_activity_logger.cc:40: > > scoped_ptr<V8ValueConverter> converter(V8ValueConverter::create()); > > On 2013/03/15 00:04:47, felt wrote: > > > On 2013/03/14 20:32:31, Matt Perry wrote: > > > > converter->FromV8Value(args[2]) should work and return a ListValue. > > > > > > converter->FromV8Value(args[2], v8::Context::GetCurrent()) returns a > > > base::Value*, not a ListValue; so I'm not sure that works. > > > > If args[2] is a v8::Array, the Value* will be of type ListValue. (You can use > > GetAsList() to check). > > > > > https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... > > File chrome/renderer/resources/extensions/app_window_custom_bindings.js > (right): > > > > > https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... > > chrome/renderer/resources/extensions/app_window_custom_bindings.js:130: }, > > true); > > I don't see where this one calls sendRequest. > > > > > https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... > > File chrome/renderer/resources/extensions/binding.js (right): > > > > > https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... > > chrome/renderer/resources/extensions/binding.js:303: > > (clear global here) > > > > > https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... > > chrome/renderer/resources/extensions/binding.js:315: > > (clear global here) > > > > > https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... > > File chrome/renderer/resources/extensions/browser_action_custom_bindings.js > > (right): > > > > > https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... > > chrome/renderer/resources/extensions/browser_action_custom_bindings.js:17: }, > > false); > > setIcon calls sendRequest behind the scenes... Confused yet? :) > > > > > https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... > > File chrome/renderer/resources/extensions/extension_custom_bindings.js > (right): > > > > > https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... > > chrome/renderer/resources/extensions/extension_custom_bindings.js:101: }, > > false); > > This one is the messaging API - do you plan on logging that separately? > > > > > https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... > > File > > chrome/renderer/resources/extensions/file_browser_handler_custom_bindings.js > > (right): > > > > > https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... > > > chrome/renderer/resources/extensions/file_browser_handler_custom_bindings.js:55: > > }, false); > > selectFile uses sendRequest. (We have a handful of custom APIs that call > through > > to internal APIs after some fiddling. The internal APIs are generally > > implemented with sendRequest.) > > > > > https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... > > File chrome/renderer/resources/extensions/notifications_custom_bindings.js > > (right): > > > > > https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... > > chrome/renderer/resources/extensions/notifications_custom_bindings.js:124: > > apiFunctions.setHandleRequest('update', handleCreate, false); > > Both of these use sendRequest... sometimes. See genHandle. > > > > > https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... > > File chrome/renderer/resources/extensions/page_action_custom_bindings.js > > (right): > > > > > https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... > > chrome/renderer/resources/extensions/page_action_custom_bindings.js:17: }, > > false); > > setIcon > > > > > https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... > > File chrome/renderer/resources/extensions/system_indicator_custom_bindings.js > > (right): > > > > > https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... > > chrome/renderer/resources/extensions/system_indicator_custom_bindings.js:20: > }, > > false); > > setIcon
I added a variable to sendRequest. I assume that all of this code is single-threaded? I looked and didn't see any worker threads (and I couldn't trigger any bugs that looked like threading issues in testing), but double checking with you anyway. https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... File chrome/renderer/resources/extensions/binding.js (right): https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... chrome/renderer/resources/extensions/binding.js:303: On 2013/03/15 17:51:07, Matt Perry wrote: > (clear global here) Done. https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... chrome/renderer/resources/extensions/binding.js:315: On 2013/03/15 17:51:07, Matt Perry wrote: > (clear global here) Done. https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... File chrome/renderer/resources/extensions/extension_custom_bindings.js (right): https://codereview.chromium.org/12517011/diff/35001/chrome/renderer/resources... chrome/renderer/resources/extensions/extension_custom_bindings.js:101: }, false); FYI I have a separate CL pertaining to messaging that I haven't uploaded yet (with special-case handling for callbacks + a cross-extension test). On 2013/03/15 17:51:07, Matt Perry wrote: > This one is the messaging API - do you plan on logging that separately?
https://codereview.chromium.org/12517011/diff/61001/chrome/renderer/resources... File chrome/renderer/resources/extensions/binding.js (right): https://codereview.chromium.org/12517011/diff/61001/chrome/renderer/resources... chrome/renderer/resources/extensions/binding.js:315: sendRequestHandler.setRequestStatus(false); move this outside this block. https://codereview.chromium.org/12517011/diff/61001/chrome/renderer/resources... File chrome/renderer/resources/extensions/send_request.js (right): https://codereview.chromium.org/12517011/diff/61001/chrome/renderer/resources... chrome/renderer/resources/extensions/send_request.js:18: var requestStatus = false; nit: I wouldn't guess what this for based on the name. How about "didCallSendRequest"? https://codereview.chromium.org/12517011/diff/61001/chrome/renderer/resources... chrome/renderer/resources/extensions/send_request.js:153: exports.setRequestStatus = setRequestStatus; nit: better to expose a clearRequestStatus that always sets the status to false.
Done https://codereview.chromium.org/12517011/diff/61001/chrome/renderer/resources... File chrome/renderer/resources/extensions/binding.js (right): https://codereview.chromium.org/12517011/diff/61001/chrome/renderer/resources... chrome/renderer/resources/extensions/binding.js:315: sendRequestHandler.setRequestStatus(false); On 2013/03/15 23:33:21, Matt Perry wrote: > move this outside this block. Done. https://codereview.chromium.org/12517011/diff/61001/chrome/renderer/resources... File chrome/renderer/resources/extensions/send_request.js (right): https://codereview.chromium.org/12517011/diff/61001/chrome/renderer/resources... chrome/renderer/resources/extensions/send_request.js:18: var requestStatus = false; On 2013/03/15 23:33:21, Matt Perry wrote: > nit: I wouldn't guess what this for based on the name. How about > "didCallSendRequest"? Done. https://codereview.chromium.org/12517011/diff/61001/chrome/renderer/resources... chrome/renderer/resources/extensions/send_request.js:153: exports.setRequestStatus = setRequestStatus; On 2013/03/15 23:33:21, Matt Perry wrote: > nit: better to expose a clearRequestStatus that always sets the status to false. Done.
lgtm https://codereview.chromium.org/12517011/diff/3005/chrome/renderer/resources/... File chrome/renderer/resources/extensions/binding.js (right): https://codereview.chromium.org/12517011/diff/3005/chrome/renderer/resources/... chrome/renderer/resources/extensions/binding.js:316: sendRequestHandler.getCalledSendRequest(); clear
brettw@chromium.org - I need OWNERS for chrome/browser/renderer_host/chrome_render_message_filter.* thanks! On 2013/03/15 23:42:18, Matt Perry wrote: > lgtm > > https://codereview.chromium.org/12517011/diff/3005/chrome/renderer/resources/... > File chrome/renderer/resources/extensions/binding.js (right): > > https://codereview.chromium.org/12517011/diff/3005/chrome/renderer/resources/... > chrome/renderer/resources/extensions/binding.js:316: > sendRequestHandler.getCalledSendRequest(); > clear
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felt@chromium.org/12517011/72001
Presubmit check for 12517011-72001 failed and returned exit status 1.
INFO:root:Found 13 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/common/extensions/PRESUBMIT.py
** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
chrome/common/extensions/extension_messages.h
Presubmit checks took 2.3s to calculate.
Hi Chris, could you please review chrome/common/extensions/extension_messages.h? It is a very small addition. On 2013/03/18 20:08:26, I haz the power (commit-bot) wrote: > Presubmit check for 12517011-72001 failed and returned exit status 1. > > INFO:root:Found 13 file(s). > > Running presubmit commit checks ... > Running /b/commit-queue/workdir/chromium/PRESUBMIT.py > Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py > Running /b/commit-queue/workdir/chromium/chrome/common/extensions/PRESUBMIT.py > > ** Presubmit ERRORS ** > Missing LGTM from an OWNER for these files: > chrome/common/extensions/extension_messages.h > > Presubmit checks took 2.3s to calculate.
https://codereview.chromium.org/12517011/diff/72001/chrome/common/extensions/... File chrome/common/extensions/extension_messages.h (right): https://codereview.chromium.org/12517011/diff/72001/chrome/common/extensions/... chrome/common/extensions/extension_messages.h:34: IPC_STRUCT_BEGIN(ExtensionHostMsg_APIAction_Params) This is a rich API; taking a DeepCopy of the arguments and so on. Cold we put this behind a flag (potentially manipulable with the Finch experiments framework) at first? I don't feel super comfortable having this enabled on all Chrome installations right away. https://codereview.chromium.org/12517011/diff/72001/chrome/renderer/extension... File chrome/renderer/extensions/api_activity_logger.cc (right): https://codereview.chromium.org/12517011/diff/72001/chrome/renderer/extension... chrome/renderer/extensions/api_activity_logger.cc:45: for (int i = 0; i < static_cast<int>(arg_array->Length()); i++) { No, just make |i| a size_t and drop the static_cast. NIT: Also, Chromium style is pre-increment.
https://codereview.chromium.org/12517011/diff/72001/chrome/common/extensions/... File chrome/common/extensions/extension_messages.h (right): https://codereview.chromium.org/12517011/diff/72001/chrome/common/extensions/... chrome/common/extensions/extension_messages.h:34: IPC_STRUCT_BEGIN(ExtensionHostMsg_APIAction_Params) What are your concerns? Perhaps I can address them. (FYI: this is almost identical to the existing ExtensionHostMsg_AddDOMAPIActionToActivityLog, but with different parameters.) I can put it behind the ActivityLog flag so that IPCs are only set if the ActivityLog is enabled. On 2013/03/18 20:58:10, Chris P. wrote: > This is a rich API; taking a DeepCopy of the arguments and so on. Cold we put > this behind a flag (potentially manipulable with the Finch experiments > framework) at first? I don't feel super comfortable having this enabled on all > Chrome installations right away.
What do you think, Justin? https://codereview.chromium.org/12517011/diff/72001/chrome/common/extensions/... File chrome/common/extensions/extension_messages.h (right): https://codereview.chromium.org/12517011/diff/72001/chrome/common/extensions/... chrome/common/extensions/extension_messages.h:34: IPC_STRUCT_BEGIN(ExtensionHostMsg_APIAction_Params) > What are your concerns? Perhaps I can address them. (FYI: this is almost > identical to the existing ExtensionHostMsg_AddDOMAPIActionToActivityLog, but > with different parameters.) I don't see an obvious, immediate vulnerability, but we are skittish about powerful general-purpose IPC APIs because we've gotten bitten a few times. I added ExtensionHostMsg_AddDOMAPIActionToActivityLog to our spreadsheet of IPCs to audit. :) In this case, the information flows from renderer to browser, > I can put it behind the ActivityLog flag so that IPCs are only set if the > ActivityLog is enabled. Gating it on whether or not the ActivityLog is enabled sounds good, but the place to enforce it (for security purposes) is on the receiving side: the receiver should immediately ignore incoming IPC messages unless ActivityLog is enabled. (Of course, at that point it also makes sense for the IPC client to check the setting before sending the message.) I see in chrome/browser/extensions/activity_log.cc that it does return immediately if !IsLogEnabled(), but it might be more efficient to shut the process down sooner in the call tree. I'm not super certain that there is undue risk here. CCing jschuh who has more experience here. Thoughts, Justin?
On 2013/03/18 21:22:12, Chris P. wrote: > What do you think, Justin? > > https://codereview.chromium.org/12517011/diff/72001/chrome/common/extensions/... > File chrome/common/extensions/extension_messages.h (right): > > https://codereview.chromium.org/12517011/diff/72001/chrome/common/extensions/... > chrome/common/extensions/extension_messages.h:34: > IPC_STRUCT_BEGIN(ExtensionHostMsg_APIAction_Params) > > What are your concerns? Perhaps I can address them. (FYI: this is almost > > identical to the existing ExtensionHostMsg_AddDOMAPIActionToActivityLog, but > > with different parameters.) > > I don't see an obvious, immediate vulnerability, but we are skittish about > powerful general-purpose IPC APIs because we've gotten bitten a few times. I > added ExtensionHostMsg_AddDOMAPIActionToActivityLog to our spreadsheet of IPCs > to audit. :) > > In this case, the information flows from renderer to browser, > > > I can put it behind the ActivityLog flag so that IPCs are only set if the > > ActivityLog is enabled. > > Gating it on whether or not the ActivityLog is enabled sounds good, but the > place to enforce it (for security purposes) is on the receiving side: the > receiver should immediately ignore incoming IPC messages unless ActivityLog is > enabled. (Of course, at that point it also makes sense for the IPC client to > check the setting before sending the message.) I see in > chrome/browser/extensions/activity_log.cc that it does return immediately if > !IsLogEnabled(), but it might be more efficient to shut the process down sooner > in the call tree. > > I'm not super certain that there is undue risk here. CCing jschuh who has more > experience here. Thoughts, Justin? Yeah, I'd definitely gate this with a check on the browser side that's as close as reasonably possible to the IPC's entrypoint.
I added checks for the ActivityLog flag earlier in the IPC handling, for both this message and the related DOM API message. When you do the audit of the two IPC channels, please feel free to ping me with any further questions. https://codereview.chromium.org/12517011/diff/72001/chrome/common/extensions/... File chrome/common/extensions/extension_messages.h (right): https://codereview.chromium.org/12517011/diff/72001/chrome/common/extensions/... chrome/common/extensions/extension_messages.h:34: IPC_STRUCT_BEGIN(ExtensionHostMsg_APIAction_Params) OK, I added an additional check in chrome/browser/renderer_host/chrome_render_message_filter.cc that simply returns if the ActivityLog is not turned on. This is one method call closer to the originating IPC. On 2013/03/18 21:22:13, Chris P. wrote: > > What are your concerns? Perhaps I can address them. (FYI: this is almost > > identical to the existing ExtensionHostMsg_AddDOMAPIActionToActivityLog, but > > with different parameters.) > > I don't see an obvious, immediate vulnerability, but we are skittish about > powerful general-purpose IPC APIs because we've gotten bitten a few times. I > added ExtensionHostMsg_AddDOMAPIActionToActivityLog to our spreadsheet of IPCs > to audit. :) > > In this case, the information flows from renderer to browser, > > > I can put it behind the ActivityLog flag so that IPCs are only set if the > > ActivityLog is enabled. > > Gating it on whether or not the ActivityLog is enabled sounds good, but the > place to enforce it (for security purposes) is on the receiving side: the > receiver should immediately ignore incoming IPC messages unless ActivityLog is > enabled. (Of course, at that point it also makes sense for the IPC client to > check the setting before sending the message.) I see in > chrome/browser/extensions/activity_log.cc that it does return immediately if > !IsLogEnabled(), but it might be more efficient to shut the process down sooner > in the call tree. > > I'm not super certain that there is undue risk here. CCing jschuh who has more > experience here. Thoughts, Justin? https://codereview.chromium.org/12517011/diff/72001/chrome/renderer/extension... File chrome/renderer/extensions/api_activity_logger.cc (right): https://codereview.chromium.org/12517011/diff/72001/chrome/renderer/extension... chrome/renderer/extensions/api_activity_logger.cc:45: for (int i = 0; i < static_cast<int>(arg_array->Length()); i++) { On 2013/03/18 20:58:10, Chris P. wrote: > No, just make |i| a size_t and drop the static_cast. > > NIT: Also, Chromium style is pre-increment. Done.
Chris, Justin: are there any other changes you need at this point beyond the extra checks? On 2013/03/18 23:34:19, felt wrote: > I added checks for the ActivityLog flag earlier in the IPC handling, for both > this message and the related DOM API message. > > When you do the audit of the two IPC channels, please feel free to ping me with > any further questions. > > https://codereview.chromium.org/12517011/diff/72001/chrome/common/extensions/... > File chrome/common/extensions/extension_messages.h (right): > > https://codereview.chromium.org/12517011/diff/72001/chrome/common/extensions/... > chrome/common/extensions/extension_messages.h:34: > IPC_STRUCT_BEGIN(ExtensionHostMsg_APIAction_Params) > OK, I added an additional check in > chrome/browser/renderer_host/chrome_render_message_filter.cc that simply returns > if the ActivityLog is not turned on. This is one method call closer to the > originating IPC. > > On 2013/03/18 21:22:13, Chris P. wrote: > > > What are your concerns? Perhaps I can address them. (FYI: this is almost > > > identical to the existing ExtensionHostMsg_AddDOMAPIActionToActivityLog, but > > > with different parameters.) > > > > I don't see an obvious, immediate vulnerability, but we are skittish about > > powerful general-purpose IPC APIs because we've gotten bitten a few times. I > > added ExtensionHostMsg_AddDOMAPIActionToActivityLog to our spreadsheet of IPCs > > to audit. :) > > > > In this case, the information flows from renderer to browser, > > > > > I can put it behind the ActivityLog flag so that IPCs are only set if the > > > ActivityLog is enabled. > > > > Gating it on whether or not the ActivityLog is enabled sounds good, but the > > place to enforce it (for security purposes) is on the receiving side: the > > receiver should immediately ignore incoming IPC messages unless ActivityLog is > > enabled. (Of course, at that point it also makes sense for the IPC client to > > check the setting before sending the message.) I see in > > chrome/browser/extensions/activity_log.cc that it does return immediately if > > !IsLogEnabled(), but it might be more efficient to shut the process down > sooner > > in the call tree. > > > > I'm not super certain that there is undue risk here. CCing jschuh who has more > > experience here. Thoughts, Justin? > > https://codereview.chromium.org/12517011/diff/72001/chrome/renderer/extension... > File chrome/renderer/extensions/api_activity_logger.cc (right): > > https://codereview.chromium.org/12517011/diff/72001/chrome/renderer/extension... > chrome/renderer/extensions/api_activity_logger.cc:45: for (int i = 0; i < > static_cast<int>(arg_array->Length()); i++) { > On 2013/03/18 20:58:10, Chris P. wrote: > > No, just make |i| a size_t and drop the static_cast. > > > > NIT: Also, Chromium style is pre-increment. > > Done.
Chris, Justin: ping :) On 2013/03/20 05:22:47, felt wrote: > Chris, Justin: are there any other changes you need at this point beyond the > extra checks? > > On 2013/03/18 23:34:19, felt wrote: > > I added checks for the ActivityLog flag earlier in the IPC handling, for both > > this message and the related DOM API message. > > > > When you do the audit of the two IPC channels, please feel free to ping me > with > > any further questions. > > > > > https://codereview.chromium.org/12517011/diff/72001/chrome/common/extensions/... > > File chrome/common/extensions/extension_messages.h (right): > > > > > https://codereview.chromium.org/12517011/diff/72001/chrome/common/extensions/... > > chrome/common/extensions/extension_messages.h:34: > > IPC_STRUCT_BEGIN(ExtensionHostMsg_APIAction_Params) > > OK, I added an additional check in > > chrome/browser/renderer_host/chrome_render_message_filter.cc that simply > returns > > if the ActivityLog is not turned on. This is one method call closer to the > > originating IPC. > > > > On 2013/03/18 21:22:13, Chris P. wrote: > > > > What are your concerns? Perhaps I can address them. (FYI: this is almost > > > > identical to the existing ExtensionHostMsg_AddDOMAPIActionToActivityLog, > but > > > > with different parameters.) > > > > > > I don't see an obvious, immediate vulnerability, but we are skittish about > > > powerful general-purpose IPC APIs because we've gotten bitten a few times. I > > > added ExtensionHostMsg_AddDOMAPIActionToActivityLog to our spreadsheet of > IPCs > > > to audit. :) > > > > > > In this case, the information flows from renderer to browser, > > > > > > > I can put it behind the ActivityLog flag so that IPCs are only set if the > > > > ActivityLog is enabled. > > > > > > Gating it on whether or not the ActivityLog is enabled sounds good, but the > > > place to enforce it (for security purposes) is on the receiving side: the > > > receiver should immediately ignore incoming IPC messages unless ActivityLog > is > > > enabled. (Of course, at that point it also makes sense for the IPC client to > > > check the setting before sending the message.) I see in > > > chrome/browser/extensions/activity_log.cc that it does return immediately if > > > !IsLogEnabled(), but it might be more efficient to shut the process down > > sooner > > > in the call tree. > > > > > > I'm not super certain that there is undue risk here. CCing jschuh who has > more > > > experience here. Thoughts, Justin? > > > > > https://codereview.chromium.org/12517011/diff/72001/chrome/renderer/extension... > > File chrome/renderer/extensions/api_activity_logger.cc (right): > > > > > https://codereview.chromium.org/12517011/diff/72001/chrome/renderer/extension... > > chrome/renderer/extensions/api_activity_logger.cc:45: for (int i = 0; i < > > static_cast<int>(arg_array->Length()); i++) { > > On 2013/03/18 20:58:10, Chris P. wrote: > > > No, just make |i| a size_t and drop the static_cast. > > > > > > NIT: Also, Chromium style is pre-increment. > > > > Done.
https://codereview.chromium.org/12517011/diff/49002/chrome/browser/renderer_h... File chrome/browser/renderer_host/chrome_render_message_filter.cc (right): https://codereview.chromium.org/12517011/diff/49002/chrome/browser/renderer_h... chrome/browser/renderer_host/chrome_render_message_filter.cc:67: // running on the wrong thread, re-dispatch from the main thread. In theory, this shouldn't happen? Can you use a DCHECK to ensure we're on the right thread, and then remove the PostTask? If not, then go ahead and ignore me. :) https://codereview.chromium.org/12517011/diff/49002/chrome/renderer/resources... File chrome/renderer/resources/extensions/binding.js (right): https://codereview.chromium.org/12517011/diff/49002/chrome/renderer/resources... chrome/renderer/resources/extensions/binding.js:259: var doubleLogging = "abcdefg"; This is never used?
Fixed the nit, and I don't think the other change needs to be made? (Thanks for taking time to look at it.) https://codereview.chromium.org/12517011/diff/49002/chrome/browser/renderer_h... File chrome/browser/renderer_host/chrome_render_message_filter.cc (right): https://codereview.chromium.org/12517011/diff/49002/chrome/browser/renderer_h... chrome/browser/renderer_host/chrome_render_message_filter.cc:67: // running on the wrong thread, re-dispatch from the main thread. I found in testing (with a DCHECK) that this is almost always invoked from a thread other than the UI thread. I assumed this was expected behavior. Is there a reason to think otherwise? On 2013/03/21 18:39:38, Chris P. wrote: > In theory, this shouldn't happen? Can you use a DCHECK to ensure we're on the > right thread, and then remove the PostTask? > > If not, then go ahead and ignore me. :) https://codereview.chromium.org/12517011/diff/49002/chrome/renderer/resources... File chrome/renderer/resources/extensions/binding.js (right): https://codereview.chromium.org/12517011/diff/49002/chrome/renderer/resources... chrome/renderer/resources/extensions/binding.js:259: var doubleLogging = "abcdefg"; Whoops, left in from testing. On 2013/03/21 18:39:38, Chris P. wrote: > This is never used?
lgtm https://codereview.chromium.org/12517011/diff/49002/chrome/browser/renderer_h... File chrome/browser/renderer_host/chrome_render_message_filter.cc (right): https://codereview.chromium.org/12517011/diff/49002/chrome/browser/renderer_h... chrome/browser/renderer_host/chrome_render_message_filter.cc:67: // running on the wrong thread, re-dispatch from the main thread. > I found in testing (with a DCHECK) that this is almost always invoked from a > thread other than the UI thread. I assumed this was expected behavior. Is there > a reason to think otherwise? The only reason to think otherwise was that I didn't know what to think. :)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felt@chromium.org/12517011/103001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felt@chromium.org/12517011/120013
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felt@chromium.org/12517011/120013
Failed to apply patch for chrome/renderer/resources/extensions/binding.js:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file chrome/renderer/resources/extensions/binding.js
Hunk #1 FAILED at 15.
Hunk #2 succeeded at 49 (offset 1 line).
Hunk #3 succeeded at 145 with fuzz 2 (offset 14 lines).
Hunk #4 succeeded at 298 (offset -1 lines).
Hunk #5 succeeded at 311 (offset -1 lines).
1 out of 5 hunks FAILED -- saving rejects to file
chrome/renderer/resources/extensions/binding.js.rej
Patch: chrome/renderer/resources/extensions/binding.js
Index: chrome/renderer/resources/extensions/binding.js
diff --git a/chrome/renderer/resources/extensions/binding.js
b/chrome/renderer/resources/extensions/binding.js
index
59c9917984a99f13ae3339b5d1117cd551e62f11..248427355c7dc85200c70e3c7f19fc9185786d5c
100644
--- a/chrome/renderer/resources/extensions/binding.js
+++ b/chrome/renderer/resources/extensions/binding.js
@@ -15,15 +15,18 @@ var extensionId = process.GetExtensionId();
var manifestVersion = process.GetManifestVersion();
var schemaRegistry = requireNative('schema_registry');
var schemaUtils = require('schemaUtils');
-var sendRequest = require('sendRequest').sendRequest;
var utils = require('utils');
+var sendRequestHandler = require('sendRequest');
+var sendRequest = sendRequestHandler.sendRequest;
+var logActivity = requireNative('activityLogger').LogActivity;
// Stores the name and definition of each API function, with methods to
// modify their behaviour (such as a custom way to handle requests to the
// API, a custom callback, etc).
-function APIFunctions() {
+function APIFunctions(namespace) {
this.apiFunctions_ = {};
this.unavailableApiFunctions_ = {};
+ this.namespace = namespace;
}
APIFunctions.prototype.register = function(apiName, apiFunction) {
@@ -48,7 +51,17 @@ APIFunctions.prototype.setHook_ =
APIFunctions.prototype.setHandleRequest =
function(apiName, customizedFunction) {
- return this.setHook_(apiName, 'handleRequest', customizedFunction);
+ var prefix = this.namespace;
+ return this.setHook_(apiName, 'handleRequest',
+ function() {
+ var ret = customizedFunction.apply(this, arguments);
+ // Logs API calls to the Activity Log if it doesn't go through an
+ // ExtensionFunction.
+ if (!sendRequestHandler.getCalledSendRequest())
+ logActivity(extensionId, prefix + "." + apiName,
+ Array.prototype.slice.call(arguments));
+ return ret;
+ });
};
APIFunctions.prototype.setUpdateArgumentsPostValidate =
@@ -121,7 +134,7 @@ var platform = getPlatform();
function Binding(schema) {
this.schema_ = schema;
- this.apiFunctions_ = new APIFunctions();
+ this.apiFunctions_ = new APIFunctions(schema.namespace);
this.customEvent_ = null;
this.customTypes_ = {};
this.customHooks_ = [];
@@ -289,6 +302,8 @@ Binding.prototype = {
if (this.updateArgumentsPostValidate)
args = this.updateArgumentsPostValidate.apply(this, args);
+ sendRequestHandler.clearCalledSendRequest();
+
var retval;
if (this.handleRequest) {
retval = this.handleRequest.apply(this, args);
@@ -300,6 +315,7 @@ Binding.prototype = {
this.definition.parameters,
optArgs);
}
+ sendRequestHandler.clearCalledSendRequest();
// Validate return value if defined - only in debug.
if (chromeHidden.validateCallbacks &&
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felt@chromium.org/12517011/40002
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/felt@chromium.org/12517011/40002
Message was sent while issue was closed.
Change committed as 190571 |
