Extension activity log database refactoring (step 1)

Extension activity log database refactoring (step 1)

This is the first of several patches to refactor the activity log
database, policy, and make changes to the database schema.

We move control over the database schema to the activity log policy,
since different policies might need different database layouts for
whatever summarization is implemented.

Schema setup is implemented by a call from the ActivityDatabase back
into the policy object.  Possible callbacks are made explicit in the
ActivityPolicyCallbacks interface; in the future there will be
additional callbacks related to writing actions to the database.  The
lifetime of the policy objects is tied to the ActivityDatabase object.

The table layout is not currently changed and code for creating the
tables is still in the Action classes; that will be changed in a future
patch.

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

[mvrable, 2013-07-09 17:56:22]

This is the first step of the database refactoring patches, could you review?

[mvrable, 2013-07-09 21:41:02]

The patch is updated; I also have a couple of questions about what the interface
should look like, if you have opinions on those.

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/activity_log/activity_database.h (right):

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_database.h:35: protected:
Interfaces are normally public instead of protected, but we only want these
methods to be called by ActivityDatabase.  I'm unsure whether this is the right
visibility to use.

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_database.h:45: virtual void
DatabaseCloseCallback() = 0;
Alternately, it could make sense to drop the use of a CloseCallback() and just
have ActivityDatabase call the destructor (which it can do since it is a friend
class).

[felt, 2013-07-09 22:14:51]

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/activity_log/activity_database.cc (right):

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_database.cc:211:
RecordBatchedActions();
Is it going to be a problem to call DatabaseCloseCallback on the DB thread, even
though the rest of a policy object's lifecycle happens on the UI thread?

One potential issue is that ActivityLogPolicy has its own RepeatingTimer, which
needs to be started and stopped on the same thread since it's not threadsafe. We
therefore need to destruct and construct ActivityLogPolicies on the same thread.
 (For that reason, I think this code as currently written should DCHECK.)
However, it seems like we can get rid of that second timer anyway.

Other than that, it seems like it would be OK... since no one else should be
calling any methods on the object at that point. (But I want to make sure you've
thought about it?)

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_database.cc:212:
policy_callbacks_->DatabaseCloseCallback();
Note that if we've hit a HardFailureClose(), already_closed_ is set to true and
this code won't get called. already_closed_ currently refers to the status of
the database connection. I'd suggest (1) moving this outside of the if
statement, and (2) making a new boolean to track the status of calling ::Close()
if you think it's needed.

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/activity_log/activity_database.h (right):

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_database.h:35: protected:
This seems right to me.

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/activity_log/activity_log.cc (right):

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_log.cc:136: policy_->Close();
What if Close() hasn't finished before policy is reassigned to a new policy?
Since ActivityLogPolicy->Close() asynchronously calls ActivityDatabase->Close(),
which then needs to flush things to disk before deleting the policy.

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/activity_log/activity_log_policy.h (right):

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_log_policy.h:155:
DISALLOW_COPY_AND_ASSIGN(ActivityLogPolicy);
yikes. is this missing in other policy files too?

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/activity_log/fullstream_ui_policy.h (right):

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/fullstream_ui_policy.h:20: public
ActivityPolicyCallbacks {
Maybe the name should have both Database and Policy in it? So it's clear that
these callbacks are specifically for using the ActivityDatabase.

[mvrable, 2013-07-09 23:03:22]

Thanks for taking a look.  I made some updates, and a few of your comments are
still unresolved since I had some questions.  Once you're happy I can send this
on for further review.

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/activity_log/activity_database.cc (right):

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_database.cc:211:
RecordBatchedActions();
On 2013/07/09 22:14:51, felt wrote:
> Is it going to be a problem to call DatabaseCloseCallback on the DB thread,
even
> though the rest of a policy object's lifecycle happens on the UI thread?
> 
> One potential issue is that ActivityLogPolicy has its own RepeatingTimer,
which
> needs to be started and stopped on the same thread since it's not threadsafe.
We
> therefore need to destruct and construct ActivityLogPolicies on the same
thread.
>  (For that reason, I think this code as currently written should DCHECK.)
> However, it seems like we can get rid of that second timer anyway.
> 
> Other than that, it seems like it would be OK... since no one else should be
> calling any methods on the object at that point. (But I want to make sure
you've
> thought about it?)

I think we want this to happen on the database thread is best, but I could be
missing some issues.

I'd forgotten about the ActivityLogPolicy having its own timer, but I agree that
we should get rid of that and let the database manage the timer.  I'll do that
(deleting the timer from the policy) as part of this patch for safety.

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_database.cc:212:
policy_callbacks_->DatabaseCloseCallback();
On 2013/07/09 22:14:51, felt wrote:
> Note that if we've hit a HardFailureClose(), already_closed_ is set to true
and
> this code won't get called. already_closed_ currently refers to the status of
> the database connection. I'd suggest (1) moving this outside of the if
> statement, and (2) making a new boolean to track the status of calling
::Close()
> if you think it's needed.

You are right about already_closed_ and HardFailureClose().

I updated this but I don't think a boolean is needed: the lifetime of the policy
should match that of the database and so we can unconditionally call
DatabaseCloseCallback in Close().  Close() itself should be called exactly once
for the ActivityDatabase (otherwise we'd be seeing either memory leaks or double
frees already).

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/activity_log/activity_log.cc (right):

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_log.cc:136: policy_->Close();
On 2013/07/09 22:14:51, felt wrote:
> What if Close() hasn't finished before policy is reassigned to a new policy?
> Since ActivityLogPolicy->Close() asynchronously calls
ActivityDatabase->Close(),
> which then needs to flush things to disk before deleting the policy. 

That occurred to me earlier; I think the current code has no more races than
before but is still suboptimal.

If the two policies use different databases there's no problem, as the old
database contents can finish being flushed even after the new database is
opened.  If they share the same file, we'll run into problems.

We'd either need some way to block until the database contents are all flushed
(there's no notification mechanism currently for this), or we could just
disallow switching policies at runtime--make it an error to set a new default
policy after one has been initialized.  But would enforcing a single policy (per
ActivityLog instance) cause trouble for unit tests?

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/activity_log/activity_log_policy.h (right):

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_log_policy.h:155:
DISALLOW_COPY_AND_ASSIGN(ActivityLogPolicy);
On 2013/07/09 22:14:51, felt wrote:
> yikes. is this missing in other policy files too?

Yes, it was missing in the ActivityLogPolicy subclasses too. 
DISALLOW_COPY_AND_ASSIGN is inherited, but I can add it to the subclasses for
clarity if you'd like.

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/activity_log/fullstream_ui_policy.h (right):

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/fullstream_ui_policy.h:20: public
ActivityPolicyCallbacks {
On 2013/07/09 22:14:51, felt wrote:
> Maybe the name should have both Database and Policy in it? So it's clear that
> these callbacks are specifically for using the ActivityDatabase.

ActivityLogPolicyDatabaseCallbacks?  That's getting a bit long but could be used
(it doesn't need to be typed in many places, at least).  Or if you can think of
a better name...

[felt, 2013-07-10 00:14:36]

Thanks Michael, a few more comments.

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/activity_log/activity_database.cc (right):

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_database.cc:211:
RecordBatchedActions();
OK, can you add a comment to ~ActivityLogPolicy() and the callback method,
specifying that they are called from the Db thread?

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/activity_log/activity_database.h (right):

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_database.h:45: virtual void
DatabaseCloseCallback() = 0;
On 2013/07/09 21:41:02, mvrable wrote:
> Alternately, it could make sense to drop the use of a CloseCallback() and just
> have ActivityDatabase call the destructor (which it can do since it is a
friend
> class).

I personally like the current way better because you can tell from looking at
the ActivityLogPolicy where the destruction happens, but not a strong preference
if Matt feels differently.

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/activity_log/activity_log.cc (right):

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_log.cc:136: policy_->Close();
At present, I don't expect this code path will be triggered anytime except
during unit tests. Maybe we should make that explicit, by adding a flag or
something that is set by the unit tests and DCHECKs otherwise if policy_ is
already set when SetDefaultPolicy is called. At the very least can you add a
comment specifying that it's dangerous to call SetDefaultPolicy more than once?

However, don't the unit tests currently use the same file but switch policies?
The one mitigation here is that I think we have batching turned off in the unit
tests, so there would be very little delay.

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/activity_log/activity_log_policy.h (right):

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_log_policy.h:155:
DISALLOW_COPY_AND_ASSIGN(ActivityLogPolicy);
If it's inherited, then nevermind. Thanks for catching it.

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/activity_log/fullstream_ui_policy.h (right):

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/fullstream_ui_policy.h:20: public
ActivityPolicyCallbacks {
ActivityDatabasePolicyCallback? 3 characters shorter...

I guess I want to make it clear from the name that all the callbacks are related
specifically to database writing. I might even prefer just
"ActivityDatabaseCallbacks".

On 2013/07/09 23:03:22, mvrable wrote:
> On 2013/07/09 22:14:51, felt wrote:
> > Maybe the name should have both Database and Policy in it? So it's clear
that
> > these callbacks are specifically for using the ActivityDatabase.
> 
> ActivityLogPolicyDatabaseCallbacks?  That's getting a bit long but could be
used
> (it doesn't need to be typed in many places, at least).  Or if you can think
of
> a better name...

[mvrable, 2013-07-10 17:49:17]

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/activity_log/activity_database.cc (right):

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_database.cc:211:
RecordBatchedActions();
On 2013/07/10 00:14:36, felt wrote:
> OK, can you add a comment to ~ActivityLogPolicy() and the callback method,
> specifying that they are called from the Db thread?

Done.

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/activity_log/activity_log.cc (right):

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_log.cc:136: policy_->Close();
On 2013/07/10 00:14:36, felt wrote:
> At present, I don't expect this code path will be triggered anytime except
> during unit tests. Maybe we should make that explicit, by adding a flag or
> something that is set by the unit tests and DCHECKs otherwise if policy_ is
> already set when SetDefaultPolicy is called. At the very least can you add a
> comment specifying that it's dangerous to call SetDefaultPolicy more than
once?
> 
> However, don't the unit tests currently use the same file but switch policies?
> The one mitigation here is that I think we have batching turned off in the
unit
> tests, so there would be very little delay.

I've changed this to a CHECK to catch cases where we switch policies when we
shouldn't, and it doesn't even seem that the unit tests are doing so.

If we need to support it in the future, perhaps we can consider using different
database files for different policies in the unit tests.

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/activity_log/fullstream_ui_policy.h (right):

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/fullstream_ui_policy.h:20: public
ActivityPolicyCallbacks {
On 2013/07/10 00:14:36, felt wrote:
> ActivityDatabasePolicyCallback? 3 characters shorter...
> 
> I guess I want to make it clear from the name that all the callbacks are
related
> specifically to database writing. I might even prefer just
> "ActivityDatabaseCallbacks".
> 
> On 2013/07/09 23:03:22, mvrable wrote:
> > On 2013/07/09 22:14:51, felt wrote:
> > > Maybe the name should have both Database and Policy in it? So it's clear
> that
> > > these callbacks are specifically for using the ActivityDatabase.
> > 
> > ActivityLogPolicyDatabaseCallbacks?  That's getting a bit long but could be
> used
> > (it doesn't need to be typed in many places, at least).  Or if you can think
> of
> > a better name...
> 

I'm going to change this to ActivityDatabaseCallbacks since that is short but I
think reasonably clear.

[felt, 2013-07-10 18:35:16]

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/activity_log/activity_log.cc (right):

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_log.cc:136: policy_->Close();
LogWithoutArguments should be triggering this code. You can see it sets the
policy twice: once at the beginning and again at the end. If it's not triggering
this code then there must be a bug. :/

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/activity_log/fullstream_ui_policy.h (right):

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/fullstream_ui_policy.h:20: public
ActivityPolicyCallbacks {
Thanks, sorry to be picky on this. :)

https://codereview.chromium.org/18660004/diff/19001/chrome/browser/extensions...
File chrome/browser/extensions/activity_log/activity_log.cc (right):

https://codereview.chromium.org/18660004/diff/19001/chrome/browser/extensions...
chrome/browser/extensions/activity_log/activity_log.cc:139: CHECK(policy_ ==
NULL);
Can you make this a DCHECK?

[mvrable, 2013-07-10 18:53:56]

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/activity_log/activity_log.cc (right):

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_log.cc:136: policy_->Close();
On 2013/07/10 18:35:17, felt wrote:
> LogWithoutArguments should be triggering this code. You can see it sets the
> policy twice: once at the beginning and again at the end. If it's not
triggering
> this code then there must be a bug. :/

I somehow missed this since I wasn't running that particular test locally. 
Investigating a bit more what changes to make.

[mvrable, 2013-07-10 20:48:40]

Another update, allowing policies to be switched for running tests.

Adrienne, is this code looking better now?  Should I add Matt as a reviewer at
this point, or do you think it is better to reach out and give him the big
picture for these code changes first?

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/activity_log/activity_log.cc (right):

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/activity_log.cc:136: policy_->Close();
On 2013/07/09 22:14:51, felt wrote:
> What if Close() hasn't finished before policy is reassigned to a new policy?
> Since ActivityLogPolicy->Close() asynchronously calls
ActivityDatabase->Close(),
> which then needs to flush things to disk before deleting the policy. 

I convinced myself (and Adrienne, in an offline discussion) that this code is
actually safe.  I've added a comment explaining.

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
File chrome/browser/extensions/activity_log/fullstream_ui_policy.h (right):

https://codereview.chromium.org/18660004/diff/3001/chrome/browser/extensions/...
chrome/browser/extensions/activity_log/fullstream_ui_policy.h:20: public
ActivityPolicyCallbacks {
On 2013/07/10 18:35:17, felt wrote:
> Thanks, sorry to be picky on this. :)

Not a problem, coming up with good names is important.

[felt, 2013-07-10 20:51:55]

Yes, I'm happy with it now. Please mail Matt. I think your issue description
does a good job but maybe in your e-mail offer that you & I can chat with him if
he wants us to explain further?

[mvrable, 2013-07-10 20:59:47]

Matt,

This is the first in what will be a series of extension activity log patches. 
The end goal is to better summarize/compress the activity log on disk to save
disk space.

To get there, I am refactoring the code so that activity log policies can
control the database schema (different summarization methods might want
different database layouts), then simplifying the activity action types (merging
all types into a single class and database table; fields will be optional
depending on whether they are needed for a particular log entry).

This is the first patch in that series of work.  If it would be helpful to have
a quick chat with Adrienne and me to get more context we can set something up.

--Michael Vrable

[Matt Perry, 2013-07-10 23:19:29]

Some comments on the ownership graph would be helpful. From what I can tell,
it's like this:
- ActivityLog "owns" ActivityPolicy, and kills it by calling
ActivityPolicy::Close().
- ActivityPolicy "owns" ActivityDatabase, and kills it by calling
ActivityDatabase::Close().
- ActivityDatabase finalizes destruction of ActivityPolicy when it calls
Delegate::DatabaseCloseCallback.

Would be good to put comments on each of the raw pointers in ActivityLog,
ActivityPolicy, and ActivityDatabase.

https://codereview.chromium.org/18660004/diff/8/chrome/browser/extensions/act...
File chrome/browser/extensions/activity_log/activity_database.h (right):

https://codereview.chromium.org/18660004/diff/8/chrome/browser/extensions/act...
chrome/browser/extensions/activity_log/activity_database.h:34: class
ActivityDatabaseCallbacks {
I would not refer to this class or its methods as "callbacks". It's an
interface. Chrome commonly refers to a class like this as a Delegate or Client.
I'd say ActivityDatabaseDelegate is appropriate. Likewise, the methods would be
OnDatabaseInit, etc.

Also, it's common to make this an inner class of ActivityDatabase, named simply
Delegate. See e.g.
https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur...

https://codereview.chromium.org/18660004/diff/8/chrome/browser/extensions/act...
chrome/browser/extensions/activity_log/activity_database.h:38: // Initializes
the database schema; this gives a policy a change to create or
chance*

https://codereview.chromium.org/18660004/diff/8/chrome/browser/extensions/act...
File chrome/browser/extensions/activity_log/fullstream_ui_policy.cc (right):

https://codereview.chromium.org/18660004/diff/8/chrome/browser/extensions/act...
chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:53: // Create the
DOMAction database.
nit: these comments are redundant

[mvrable, 2013-07-11 17:30:33]

I've made some updates based on your comments.  The big changes are renaming
ActivityDatabaseCallbacks to ActivityDatabase::Delegate, and adding more
detailed comments discussing object lifetimes.

https://codereview.chromium.org/18660004/diff/8/chrome/browser/extensions/act...
File chrome/browser/extensions/activity_log/activity_database.h (right):

https://codereview.chromium.org/18660004/diff/8/chrome/browser/extensions/act...
chrome/browser/extensions/activity_log/activity_database.h:34: class
ActivityDatabaseCallbacks {
On 2013/07/10 23:19:30, Matt Perry wrote:
> I would not refer to this class or its methods as "callbacks". It's an
> interface. Chrome commonly refers to a class like this as a Delegate or
Client.
> I'd say ActivityDatabaseDelegate is appropriate. Likewise, the methods would
be
> OnDatabaseInit, etc.
> 
> Also, it's common to make this an inner class of ActivityDatabase, named
simply
> Delegate. See e.g.
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur...

I've tried renaming this to be an inner class, ActivityDatabase::Delegate. 
Adrienne, does this look okay to you as well?

https://codereview.chromium.org/18660004/diff/8/chrome/browser/extensions/act...
chrome/browser/extensions/activity_log/activity_database.h:38: // Initializes
the database schema; this gives a policy a change to create or
On 2013/07/10 23:19:30, Matt Perry wrote:
> chance*

Done.

https://codereview.chromium.org/18660004/diff/8/chrome/browser/extensions/act...
File chrome/browser/extensions/activity_log/fullstream_ui_policy.cc (right):

https://codereview.chromium.org/18660004/diff/8/chrome/browser/extensions/act...
chrome/browser/extensions/activity_log/fullstream_ui_policy.cc:53: // Create the
DOMAction database.
On 2013/07/10 23:19:30, Matt Perry wrote:
> nit: these comments are redundant

Deleted, though all this code will be gone soon in one of the later refactoring
patches.

[Matt Perry, 2013-07-11 17:49:16]

lgtm

[felt, 2013-07-11 18:59:25]

lgtm

[commit-bot: I haz the power, 2013-07-11 20:02:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mvrable@chromium.org/18660004/46001

[commit-bot: I haz the power, 2013-07-11 22:26:51]

Change committed as 211234