[Activity Log] fix logging of blocked API calls

[Activity Log] fix logging of blocked API calls

Recent refactoring of how extension permissions are checked (now using Features) broke the logging of blocked API calls. This CL tries to restore the logging.

BUG=259118
R=kalman@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=211595

[felt, 2013-07-11 19:13:51]

Hi Ben,

Some recent refactoring accidentally squashed my logging of blocked API calls.
It used to be that most blocked API calls went through
Dispatcher::CheckContextAccessToExtensionAPI, but now failure happens earlier &
I'm no longer logging in the right place. 

Could you please review this CL, and see if I'm now logging in the right places
again?

I also can't figure out how to get the full name of the api call in binding.js
(see comment in the file) -- do you know if it's possible?

Thanks,
Adrienne

https://codereview.chromium.org/18769009/diff/2001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/binding.js (right):

https://codereview.chromium.org/18769009/diff/2001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/binding.js:429:
logActivity.LogBlockedCall(extensionId, schema.namespace);
Here I can't figure out how to get the full name of the api call?

[not at google - send to devlin, 2013-07-11 19:22:33]

https://codereview.chromium.org/18769009/diff/2001/chrome/renderer/extensions...
File chrome/renderer/extensions/api_activity_logger.cc (right):

https://codereview.chromium.org/18769009/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/api_activity_logger.cc:45:
*v8::String::AsciiValue(args[1]->ToString()));
you actually don't need the ToString() here. AsciiValue will do that
automatically.

https://codereview.chromium.org/18769009/diff/2001/chrome/renderer/extensions...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/18769009/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/dispatcher.cc:1448: result !=
Feature::UNSUPPORTED_CHANNEL) {
these flags seem a bit arbitrary, is there any chance you can do the filtering
in LogBlockedCall instead?

https://codereview.chromium.org/18769009/diff/2001/chrome/renderer/extensions...
File chrome/renderer/extensions/runtime_custom_bindings.cc (right):

https://codereview.chromium.org/18769009/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/runtime_custom_bindings.cc:74: "nativeMessaging");
Ok, maybe we should add a utility to ChromeV8Context to do this automatically,
like IsFeatureAvailable which takes a Feature. Use that for the call from
dispatcher.cc too.

https://codereview.chromium.org/18769009/diff/2001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/binding.js (right):

https://codereview.chromium.org/18769009/diff/2001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/binding.js:429:
logActivity.LogBlockedCall(extensionId, schema.namespace);
On 2013/07/11 19:13:51, felt wrote:
> Here I can't figure out how to get the full name of the api call?

Yeah, it's because this code only deals with API-level availability. There is
basically no way to get the name of the function without some serious
refactoring (entirely redoing the way that we setup bindings - it'd be at least
3 weeks of work).

[felt, 2013-07-11 19:26:37]

One quick question before I make edits

https://codereview.chromium.org/18769009/diff/2001/chrome/renderer/extensions...
File chrome/renderer/extensions/runtime_custom_bindings.cc (right):

https://codereview.chromium.org/18769009/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/runtime_custom_bindings.cc:74: "nativeMessaging");
Sorry, not 100% sure what you mean here -- add a method to Chromev8context that,
in turn, then calls APIActivityLogger::LogBlockedCall?

https://codereview.chromium.org/18769009/diff/2001/chrome/renderer/resources/...
File chrome/renderer/resources/extensions/binding.js (right):

https://codereview.chromium.org/18769009/diff/2001/chrome/renderer/resources/...
chrome/renderer/resources/extensions/binding.js:429:
logActivity.LogBlockedCall(extensionId, schema.namespace);
Ah, OK. Definitely not worth 3 weeks of work, just the API-level name will be
fine.

On 2013/07/11 19:22:33, kalman wrote:
> On 2013/07/11 19:13:51, felt wrote:
> > Here I can't figure out how to get the full name of the api call?
> 
> Yeah, it's because this code only deals with API-level availability. There is
> basically no way to get the name of the function without some serious
> refactoring (entirely redoing the way that we setup bindings - it'd be at
least
> 3 weeks of work).

[not at google - send to devlin, 2013-07-11 19:28:02]

https://codereview.chromium.org/18769009/diff/2001/chrome/renderer/extensions...
File chrome/renderer/extensions/runtime_custom_bindings.cc (right):

https://codereview.chromium.org/18769009/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/runtime_custom_bindings.cc:74: "nativeMessaging");
On 2013/07/11 19:26:37, felt wrote:
> Sorry, not 100% sure what you mean here -- add a method to Chromev8context
that,
> in turn, then calls APIActivityLogger::LogBlockedCall?

Yep that's what I meant, do the logging in that single place if possible.

[felt, 2013-07-12 01:53:04]

https://codereview.chromium.org/18769009/diff/2001/chrome/renderer/extensions...
File chrome/renderer/extensions/api_activity_logger.cc (right):

https://codereview.chromium.org/18769009/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/api_activity_logger.cc:45:
*v8::String::AsciiValue(args[1]->ToString()));
On 2013/07/11 19:22:33, kalman wrote:
> you actually don't need the ToString() here. AsciiValue will do that
> automatically.

Done.

https://codereview.chromium.org/18769009/diff/2001/chrome/renderer/extensions...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/18769009/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/dispatcher.cc:1448: result !=
Feature::UNSUPPORTED_CHANNEL) {
On 2013/07/11 19:22:33, kalman wrote:
> these flags seem a bit arbitrary, is there any chance you can do the filtering
> in LogBlockedCall instead?

Done.

https://codereview.chromium.org/18769009/diff/2001/chrome/renderer/extensions...
File chrome/renderer/extensions/runtime_custom_bindings.cc (right):

https://codereview.chromium.org/18769009/diff/2001/chrome/renderer/extensions...
chrome/renderer/extensions/runtime_custom_bindings.cc:74: "nativeMessaging");
Instead of adding an extra method to be a wrapper around LogBlockedCall, I
changed LogBlockedCall to take in an AvailabilityResult. Does that work for you?

[not at google - send to devlin, 2013-07-12 15:53:06]

lgtm

https://codereview.chromium.org/18769009/diff/14001/chrome/renderer/extension...
File chrome/renderer/extensions/api_activity_logger.h (right):

https://codereview.chromium.org/18769009/diff/14001/chrome/renderer/extension...
chrome/renderer/extensions/api_activity_logger.h:42: const
v8::FunctionCallbackInfo<v8::Value>& args);
unrelated comment: do these v8-exposed functions need to be public?

https://codereview.chromium.org/18769009/diff/14001/chrome/renderer/extension...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/18769009/diff/14001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:1447: Feature::AvailabilityResult
result = availability.result();
nit: inline?

[felt, 2013-07-13 04:38:54]

thanks for the help!

https://codereview.chromium.org/18769009/diff/14001/chrome/renderer/extension...
File chrome/renderer/extensions/api_activity_logger.h (right):

https://codereview.chromium.org/18769009/diff/14001/chrome/renderer/extension...
chrome/renderer/extensions/api_activity_logger.h:42: const
v8::FunctionCallbackInfo<v8::Value>& args);
On 2013/07/12 15:53:07, kalman wrote:
> unrelated comment: do these v8-exposed functions need to be public?

I thought they had be to work, but it appears they don't -- so I made them
private.

https://codereview.chromium.org/18769009/diff/14001/chrome/renderer/extension...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/18769009/diff/14001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:1447: Feature::AvailabilityResult
result = availability.result();
On 2013/07/12 15:53:07, kalman wrote:
> nit: inline?

Done.

[commit-bot: I haz the power, 2013-07-14 01:19:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/felt@chromium.org/18769009/25001

[commit-bot: I haz the power, 2013-07-14 08:18:24]

Change committed as 211595