[Extensions] Short-circuit activity logging if not enabled

[Extensions] Short-circuit activity logging if not enabled

In order to log activity events in extensions, we need to copy all the
arguments that were passed to the method or event. In some cases (such
as messaging), this can be an extreme amount of work. Additionally, this
can require passing objects from the IO to UI thread. In an extreme
case, we would also create copies of arguments sent via the activityLog
API itself - meaning that for any given API call, we created two extra
copies of all arguments (one for the activity log itself, and another
for the record of the activityLog event dispatch). In some cases, this
activity logging resulted in more work than the API call itself.

Instead, store state around which profiles have an actively-recording
ActivityLog and which extensions are whitelisted in a thread-safe manner
so that we can short-circuit the logging in most cases, and avoid
creating excessive copies for the 99% case.

BUG=620359
Committed: https://crrev.com/a84983c58d89dbb83fc97eb24ad70f5780375dd9
Cr-Commit-Position: refs/heads/master@{#401162}

[Devlin, 2016-06-17 18:52:13]

Description was changed from

==========
AL awesome
BUG=
==========

to

==========
[Extensions] Short-circuit activity logging if not enabled 
 
In order to log activity events in extensions, we need to copy all the
arguments that were passed to the method or event. In some cases (such
as messaging), this can be an extreme amount of work. Additionally, this
can require passing objects from the IO to UI thread. In an extreme
case, we would also create copies of arguments sent via the activityLog
API itself - meaning that for any given API call, we created two extra
copies of all arguments (one for the activity log itself, and another
for the record of the activityLog event dispatch). In some cases, this
activity logging resulted in more work than the API call itself.

Instead, store state around which profiles have an actively-recording
ActivityLog and which extensions are whitelisted in a thread-safe manner
so that we can short-circuit the logging in most cases, and avoid
creating excessive copies for the 99% case.

BUG=620359
==========

[Devlin, 2016-06-17 20:13:38]

Patchset #1 (id:1) has been deleted

[Devlin, 2016-06-17 20:13:45]

Patchset #1 (id:20001) has been deleted

[Devlin, 2016-06-17 20:14:24]

rdevlin.cronin@chromium.org changed reviewers:
+ asargent@chromium.org, felt@chromium.org

[Devlin, 2016-06-17 20:14:25]

Adrienne and Antony, mind taking a look?

[asargent_no_longer_on_chrome, 2016-06-20 22:13:11]

lgtm

https://codereview.chromium.org/2077723002/diff/40001/extensions/browser/api_...
File extensions/browser/api_activity_monitor.h (right):

https://codereview.chromium.org/2077723002/diff/40001/extensions/browser/api_...
extensions/browser/api_activity_monitor.h:59: // Called when an extension uses
the web request API.
Looks like this can be called from IO thread - worth mentioning in the comment?

Also, it seems that browser_context we call this with comes from the void* we
use in io_thread_extension_message_filter.h - I guess we use void* instead of
BrowserContext* as a reminder that you're not on the UI thread and the pointer
might not be valid. Is that worth mentioning in the comment for this method?

[Devlin, 2016-06-21 17:07:52]

felt@, friendly ping?

https://codereview.chromium.org/2077723002/diff/40001/extensions/browser/api_...
File extensions/browser/api_activity_monitor.h (right):

https://codereview.chromium.org/2077723002/diff/40001/extensions/browser/api_...
extensions/browser/api_activity_monitor.h:59: // Called when an extension uses
the web request API.
On 2016/06/20 22:13:11, Antony Sargent wrote:
> Looks like this can be called from IO thread - worth mentioning in the
comment?
> 
> Also, it seems that browser_context we call this with comes from the void* we
> use in io_thread_extension_message_filter.h - I guess we use void* instead of
> BrowserContext* as a reminder that you're not on the UI thread and the pointer
> might not be valid. Is that worth mentioning in the comment for this method?
> 

Added more comments.

[felt, 2016-06-21 17:21:49]

lgtm, although I have to admit that it's been almost 4 years since I looked at /
worked on this code.

[felt, 2016-06-21 17:22:54]

did this CL drop the runtime flag check when calculating ShouldLog?

[Devlin, 2016-06-21 19:26:47]

On 2016/06/21 17:21:49, felt wrote:
> lgtm, although I have to admit that it's been almost 4 years since I looked at
/
> worked on this code.

Yeah, but you probably still know it better than us. :)  Figured a sanity check
wouldn't hurt.

On 2016/06/21 17:22:54, felt wrote:
> did this CL drop the runtime flag check when calculating ShouldLog?

Shouldn't have.  db_enabled_ is set to true in the AL ctor if the CL flag is
present, and CheckActive() sets is_active_ to true if db_enabled_ is true.  So
we should be okay, I think?

That said, I should probably add tests for this.

[Devlin, 2016-06-22 00:34:00]

On 2016/06/21 19:26:47, Devlin wrote:
> On 2016/06/21 17:21:49, felt wrote:
> > lgtm, although I have to admit that it's been almost 4 years since I looked
at
> /
> > worked on this code.
> 
> Yeah, but you probably still know it better than us. :)  Figured a sanity
check
> wouldn't hurt.
> 
> On 2016/06/21 17:22:54, felt wrote:
> > did this CL drop the runtime flag check when calculating ShouldLog?
> 
> Shouldn't have.  db_enabled_ is set to true in the AL ctor if the CL flag is
> present, and CheckActive() sets is_active_ to true if db_enabled_ is true.  So
> we should be okay, I think?
> 
> That said, I should probably add tests for this.

Added a test to verify the ShouldLog behavior (and the command line behavior is
already tested in other places).

[Devlin, 2016-06-22 00:34:05]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2016-06-22 00:34:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
asargent@chromium.org, felt@chromium.org
Link to the patchset: https://codereview.chromium.org/2077723002/#ps80001
(title: "Add Test")

[commit-bot: I haz the power, 2016-06-22 00:35:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2077723002/80001

[commit-bot: I haz the power, 2016-06-22 02:06:36]

Description was changed from

==========
[Extensions] Short-circuit activity logging if not enabled 
 
In order to log activity events in extensions, we need to copy all the
arguments that were passed to the method or event. In some cases (such
as messaging), this can be an extreme amount of work. Additionally, this
can require passing objects from the IO to UI thread. In an extreme
case, we would also create copies of arguments sent via the activityLog
API itself - meaning that for any given API call, we created two extra
copies of all arguments (one for the activity log itself, and another
for the record of the activityLog event dispatch). In some cases, this
activity logging resulted in more work than the API call itself.

Instead, store state around which profiles have an actively-recording
ActivityLog and which extensions are whitelisted in a thread-safe manner
so that we can short-circuit the logging in most cases, and avoid
creating excessive copies for the 99% case.

BUG=620359
==========

to

==========
[Extensions] Short-circuit activity logging if not enabled 
 
In order to log activity events in extensions, we need to copy all the
arguments that were passed to the method or event. In some cases (such
as messaging), this can be an extreme amount of work. Additionally, this
can require passing objects from the IO to UI thread. In an extreme
case, we would also create copies of arguments sent via the activityLog
API itself - meaning that for any given API call, we created two extra
copies of all arguments (one for the activity log itself, and another
for the record of the activityLog event dispatch). In some cases, this
activity logging resulted in more work than the API call itself.

Instead, store state around which profiles have an actively-recording
ActivityLog and which extensions are whitelisted in a thread-safe manner
so that we can short-circuit the logging in most cases, and avoid
creating excessive copies for the 99% case.

BUG=620359
==========

[commit-bot: I haz the power, 2016-06-22 02:06:37]

Committed patchset #3 (id:80001)

[commit-bot: I haz the power, 2016-06-22 02:09:11]

Description was changed from

==========
[Extensions] Short-circuit activity logging if not enabled 
 
In order to log activity events in extensions, we need to copy all the
arguments that were passed to the method or event. In some cases (such
as messaging), this can be an extreme amount of work. Additionally, this
can require passing objects from the IO to UI thread. In an extreme
case, we would also create copies of arguments sent via the activityLog
API itself - meaning that for any given API call, we created two extra
copies of all arguments (one for the activity log itself, and another
for the record of the activityLog event dispatch). In some cases, this
activity logging resulted in more work than the API call itself.

Instead, store state around which profiles have an actively-recording
ActivityLog and which extensions are whitelisted in a thread-safe manner
so that we can short-circuit the logging in most cases, and avoid
creating excessive copies for the 99% case.

BUG=620359
==========

to

==========
[Extensions] Short-circuit activity logging if not enabled

In order to log activity events in extensions, we need to copy all the
arguments that were passed to the method or event. In some cases (such
as messaging), this can be an extreme amount of work. Additionally, this
can require passing objects from the IO to UI thread. In an extreme
case, we would also create copies of arguments sent via the activityLog
API itself - meaning that for any given API call, we created two extra
copies of all arguments (one for the activity log itself, and another
for the record of the activityLog event dispatch). In some cases, this
activity logging resulted in more work than the API call itself.

Instead, store state around which profiles have an actively-recording
ActivityLog and which extensions are whitelisted in a thread-safe manner
so that we can short-circuit the logging in most cases, and avoid
creating excessive copies for the 99% case.

BUG=620359

Committed: https://crrev.com/a84983c58d89dbb83fc97eb24ad70f5780375dd9
Cr-Commit-Position: refs/heads/master@{#401162}
==========

[commit-bot: I haz the power, 2016-06-22 02:09:13]

Patchset 3 (id:??) landed as
https://crrev.com/a84983c58d89dbb83fc97eb24ad70f5780375dd9
Cr-Commit-Position: refs/heads/master@{#401162}