Move code to clear web databases on shutdown to the database tracker

Move code to clear web databases on shutdown to the database tracker

BUG=86928
TEST=test_shell_tests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91817

[jochen (gone - plz use gerrit), 2011-06-22 16:29:21]

this is pending http://codereview.chromium.org/7233007/, then I'll remove the
comment in vfs_backend.cc

Michael, Miranda plz review

Brett, Carlos fyi

[Miranda Callahan, 2011-06-22 16:40:48]

On 2011/06/22 16:29:21, jochen wrote:
> this is pending http://codereview.chromium.org/7233007/, then I'll remove the
> comment in vfs_backend.cc
> 
> Michael, Miranda plz review
> 
> Brett, Carlos fyi

LGTM from a multi-profile point of view -- but wait for michaeln, as the meat of
this code is stuff I'm not familiar with.

[Miranda Callahan, 2011-06-22 16:40:54]

http://codereview.chromium.org/7234014/diff/1/webkit/database/vfs_backend.cc
File webkit/database/vfs_backend.cc (right):

http://codereview.chromium.org/7234014/diff/1/webkit/database/vfs_backend.cc#...
webkit/database/vfs_backend.cc:108: //flags |= base::PLATFORM_FILE_SHARE_DELETE;
Should this be removed?

[jochen (gone - plz use gerrit), 2011-06-22 16:47:02]

On Jun 22, 2011 6:40 PM, <mirandac@chromium.org> wrote:
>
>
>
http://codereview.chromium.org/7234014/diff/1/webkit/database/vfs_backend.cc
> File webkit/database/vfs_backend.cc (right):
>
>
http://codereview.chromium.org/7234014/diff/1/webkit/database/vfs_backend.cc#...
> webkit/database/vfs_backend.cc:108: //flags |=
> base::PLATFORM_FILE_SHARE_DELETE;
> Should this be removed?
>

I'll remove the // once the cl adding this flag has landed

>
> http://codereview.chromium.org/7234014/

[michaeln, 2011-06-22 19:43:27]

I like the shape of this change...

http://codereview.chromium.org/7234014/diff/1/webkit/database/database_tracke...
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/7234014/diff/1/webkit/database/database_tracke...
webkit/database/database_tracker.cc:139: base::ClosePlatformFile(file_handle);
I'm not sure this will actually result in the file's deletion IF there are other
handles open at the time this handle is closed? If you don't use windows
regularly, i can compose a test and let you know of the results.

http://codereview.chromium.org/7234014/diff/1/webkit/database/database_tracke...
webkit/database/database_tracker.cc:141:
databases_table_->DeleteOrigin(*origin);
As a matter of privacy, i think we need to delete the directory for the origin
as well since the host can be determined by the name.

[michaeln, 2011-06-22 20:15:33]

http://codereview.chromium.org/7234014/diff/1/webkit/database/database_tracke...
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/7234014/diff/1/webkit/database/database_tracke...
webkit/database/database_tracker.cc:118: if (clear_local_state_on_exit_ &&
LazyInit()) {
Thought of another issue that has to be considered.

This dtor is not guaranteed to be run on any particular thread. Upon true
browser shutdown, it'd probably be ok to do this on the IO thread, but in the
multiprofile case where a profile can go away while the browser is still
running... we probably have to ensure this logic is executed on the
db_tracker_thread (that's the FILE thread in chrome).

http://codereview.chromium.org/7234014/diff/1/webkit/database/database_tracke...
webkit/database/database_tracker.cc:139: base::ClosePlatformFile(file_handle);
On 2011/06/22 19:43:28, michaeln wrote:
> I'm not sure this will actually result in the file's deletion IF there are
other
> handles open at the time this handle is closed? If you don't use windows
> regularly, i can compose a test and let you know of the results.

Hmmm... if we were guaranteed that the IPC channel and it's associated
MessageFilters like the DatabaseMessageFilter weren't deleted until the child
process had terminated, we should be able to safely these files (or rather, open
handles in renderers or workers would not get in the way of deletion).

There's the OnChannelClosing() method, but i don't think that guarantees the
remote process has actually exitted yet.

[jochen (gone - plz use gerrit), 2011-06-22 21:44:11]

http://codereview.chromium.org/7234014/diff/1/webkit/database/database_tracke...
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/7234014/diff/1/webkit/database/database_tracke...
webkit/database/database_tracker.cc:118: if (clear_local_state_on_exit_ &&
LazyInit()) {
On 2011/06/22 20:15:33, michaeln wrote:
> Thought of another issue that has to be considered.
> 
> This dtor is not guaranteed to be run on any particular thread. Upon true
> browser shutdown, it'd probably be ok to do this on the IO thread, but in the
> multiprofile case where a profile can go away while the browser is still
> running... we probably have to ensure this logic is executed on the
> db_tracker_thread (that's the FILE thread in chrome).

The profiles (which own the db tracker) are deleted before the threads are taken
down, so we should posttask this to the db thread

http://codereview.chromium.org/7234014/diff/1/webkit/database/database_tracke...
webkit/database/database_tracker.cc:139: base::ClosePlatformFile(file_handle);
On 2011/06/22 19:43:28, michaeln wrote:
> I'm not sure this will actually result in the file's deletion IF there are
other
> handles open at the time this handle is closed? If you don't use windows
> regularly, i can compose a test and let you know of the results.

With the change in vfs_backend.cc, you can mark open files as delete on close.
If the last file handle to the file is closed, it will be deleted.

http://codereview.chromium.org/7234014/diff/1/webkit/database/database_tracke...
webkit/database/database_tracker.cc:141:
databases_table_->DeleteOrigin(*origin);
On 2011/06/22 19:43:28, michaeln wrote:
> As a matter of privacy, i think we need to delete the directory for the origin
> as well since the host can be determined by the name.

that is, however, more difficult because the files in the directory might not be
deleted immediately. I could rename them if I can't delete them, wdyt?

[michaeln, 2011-06-22 22:35:14]

http://codereview.chromium.org/7234014/diff/1/webkit/database/database_tracke...
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/7234014/diff/1/webkit/database/database_tracke...
webkit/database/database_tracker.cc:118: if (clear_local_state_on_exit_ &&
LazyInit()) {
> The profiles (which own the db tracker) are deleted before the threads are
taken
> down, so we should posttask this to the db thread

DatabaseMessageFilters also hold references to this object so it's not strictly
"owned". I'm not real found of DeleteOnThisOrThat thread trait, but it might be
an option for this class to get it deleted on the FILE thread.

Marja is looking at posting a task to the AppcacheService for this "delete on
exit" purpose too. Maybe the profile just needs to post a task to set the
"clear_on_exit" flag (no need to set the flag until then really) and the dtor
can take it from there.

http://codereview.chromium.org/7234014/diff/1/webkit/database/database_tracke...
webkit/database/database_tracker.cc:139: base::ClosePlatformFile(file_handle);
> With the change in vfs_backend.cc, you can mark open files as delete on close.
> If the last file handle to the file is closed, it will be deleted.

But what if the last file handle to go is not one that was opened with the
DELETE_ON_CLOSE flag? If the file is still deleted in that case, great... but is
that the case?

http://codereview.chromium.org/7234014/diff/1/webkit/database/database_tracke...
webkit/database/database_tracker.cc:141:
databases_table_->DeleteOrigin(*origin);
On 2011/06/22 21:44:12, jochen wrote:
> On 2011/06/22 19:43:28, michaeln wrote:
> > As a matter of privacy, i think we need to delete the directory for the
origin
> > as well since the host can be determined by the name.
> 
> that is, however, more difficult because the files in the directory might not
be
> deleted immediately. I could rename them if I can't delete them, wdyt?

we'd have to move them in order to delete the directory i suppose

[jochen (gone - plz use gerrit), 2011-06-28 10:01:24]

the CL is now complete, PTAL

http://codereview.chromium.org/7234014/diff/1/webkit/database/database_tracke...
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/7234014/diff/1/webkit/database/database_tracke...
webkit/database/database_tracker.cc:118: if (clear_local_state_on_exit_ &&
LazyInit()) {
On 2011/06/22 22:35:14, michaeln wrote:
> > The profiles (which own the db tracker) are deleted before the threads are
> taken
> > down, so we should posttask this to the db thread
> 
> DatabaseMessageFilters also hold references to this object so it's not
strictly
> "owned". I'm not real found of DeleteOnThisOrThat thread trait, but it might
be
> an option for this class to get it deleted on the FILE thread.
> 
> Marja is looking at posting a task to the AppcacheService for this "delete on
> exit" purpose too. Maybe the profile just needs to post a task to set the
> "clear_on_exit" flag (no need to set the flag until then really) and the dtor
> can take it from there.

Done.

http://codereview.chromium.org/7234014/diff/1/webkit/database/database_tracke...
webkit/database/database_tracker.cc:139: base::ClosePlatformFile(file_handle);
On 2011/06/22 22:35:14, michaeln wrote:
> > With the change in vfs_backend.cc, you can mark open files as delete on
close.
> > If the last file handle to the file is closed, it will be deleted.
> 
> But what if the last file handle to go is not one that was opened with the
> DELETE_ON_CLOSE flag? If the file is still deleted in that case, great... but
is
> that the case?

if the file was originally opened with SHARE_DELETE, this is ok

http://codereview.chromium.org/7234014/diff/1/webkit/database/database_tracke...
webkit/database/database_tracker.cc:141:
databases_table_->DeleteOrigin(*origin);
On 2011/06/22 22:35:14, michaeln wrote:
> On 2011/06/22 21:44:12, jochen wrote:
> > On 2011/06/22 19:43:28, michaeln wrote:
> > > As a matter of privacy, i think we need to delete the directory for the
> origin
> > > as well since the host can be determined by the name.
> > 
> > that is, however, more difficult because the files in the directory might
not
> be
> > deleted immediately. I could rename them if I can't delete them, wdyt?
> 
> we'd have to move them in order to delete the directory i suppose

Done.

[michaeln, 2011-07-01 03:02:34]

http://codereview.chromium.org/7234014/diff/9001/webkit/database/database_tra...
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/7234014/diff/9001/webkit/database/database_tra...
webkit/database/database_tracker.cc:39: void
ClearLocalState(webkit_database::DatabaseTracker* tracker) {
What guarantees are there that the tracker has not been deleted here? Did you
mean to make the param be a scoped_refptr<>? Also, see comments below about
making this a class method instead of a separate function.

http://codereview.chromium.org/7234014/diff/9001/webkit/database/database_tra...
webkit/database/database_tracker.cc:475: // If there are left-over directoryfrom
failed deletion attempts, clean
s/b directories from

http://codereview.chromium.org/7234014/diff/9001/webkit/database/database_tra...
webkit/database/database_tracker.cc:819: if (clear_local_state_on_exit_ &&
LazyInit()) {
We shouldn't call LazyInit() on the UI thread. It'd be safer to invoke this
method on the FILE thread.

The OffTheRecordProfileImpl does something very similar to what this CL is
doing, but it calls DeleteIncognitoDBDirectory() (being careful to schedule the
call on the FILE thread) in it's dtor instead of calling Shutdown().

Maybe we can use the Shutdown() method for both purposes?

void Shutdown(bool clear_local_state) {
  DCHECK(db_tracker_thread_->BelongsToCurrentThread());
  shutting_down_ = true;
  is_initialized_ = false;
  if (is_incognito_)
    DeleteIncognitoDBDirectory();
  else if (clear_local_state)
    ClearLocalState();
}

wdyt?

http://codereview.chromium.org/7234014/diff/9001/webkit/database/database_tra...
webkit/database/database_tracker.cc:832: void
DatabaseTracker::SetClearLocalStateOnExit(bool clear_local_state_on_exit) {
It'd be safer to test and set these data members exclusively on the FILE thread.

 if (!db_tracker_thread_->BelongsToCurrentThread()) {
    db_tracker_thread_->PostTask(FROM_HERE,
        NewRunnableMethod(this,
                          &DatabaseTracker::SetClearLocalStateOnExit,
                          clear_local_state_on_exit));
    return;
  }
  clear_local_state_on_exit_ = clear_local_state_on_exit;

http://codereview.chromium.org/7234014/diff/9001/webkit/database/database_tra...
File webkit/database/database_tracker_unittest.cc (right):

http://codereview.chromium.org/7234014/diff/9001/webkit/database/database_tra...
webkit/database/database_tracker_unittest.cc:522: static void
DatabaseTrackerClearLocalStateOnExit() {
Can you alter the test such that a file is held open when Shutdown() is called,
and then closed after Shutdown() has returned.

http://codereview.chromium.org/7234014/diff/9001/webkit/database/database_tra...
webkit/database/database_tracker_unittest.cc:545:
tracker->DatabaseOpened(kOrigin1, kDB1, kDescription, 0,
Having these methods be called on something other than the 'tracker thread' will
prevent adding DCHECKs in the tracker class that things are being called on that
thread. I don't think you actually need a new thread to conduct this test. Can
we use a 'proxy' for the current thread's message loop instead and call
RunAllPending() as needed.

http://codereview.chromium.org/7234014/diff/9001/webkit/database/database_tra...
webkit/database/database_tracker_unittest.cc:558:
FilePath::FromWStringHack(UTF16ToWide(
Let's avoid adding more usages of FromWStringHack.

 db_file = tracker->GetFullDBFilePath(kOriginId, kName);
 EXPECT_TRUE(CreateDirectory(db_file.DirName()));
 EXPECT_TRUE(EnsureFileOfSize(db_file, 1));

http://codereview.chromium.org/7234014/diff/9001/webkit/database/vfs_backend.cc
File webkit/database/vfs_backend.cc (right):

http://codereview.chromium.org/7234014/diff/9001/webkit/database/vfs_backend....
webkit/database/vfs_backend.cc:108: flags |= base::PLATFORM_FILE_SHARE_DELETE;
Please try this on the layout test try bots too. I'm wondering if SHARE_DELETE
will butt heads with EXCLUSIVE_READ and EXCLUSIVE_WRITE?

gcl try XXX --bot mac_layout,win_layout,linux_layout

[jochen (gone - plz use gerrit), 2011-07-06 09:24:52]

http://codereview.chromium.org/7234014/diff/9001/webkit/database/database_tra...
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/7234014/diff/9001/webkit/database/database_tra...
webkit/database/database_tracker.cc:39: void
ClearLocalState(webkit_database::DatabaseTracker* tracker) {
On 2011/07/01 03:02:34, michaeln wrote:
> What guarantees are there that the tracker has not been deleted here? Did you
> mean to make the param be a scoped_refptr<>? Also, see comments below about
> making this a class method instead of a separate function.

Done.

http://codereview.chromium.org/7234014/diff/9001/webkit/database/database_tra...
webkit/database/database_tracker.cc:475: // If there are left-over directoryfrom
failed deletion attempts, clean
On 2011/07/01 03:02:34, michaeln wrote:
> s/b directories from

Done.

http://codereview.chromium.org/7234014/diff/9001/webkit/database/database_tra...
webkit/database/database_tracker.cc:819: if (clear_local_state_on_exit_ &&
LazyInit()) {
On 2011/07/01 03:02:34, michaeln wrote:
> We shouldn't call LazyInit() on the UI thread. It'd be safer to invoke this
> method on the FILE thread.
> 
> The OffTheRecordProfileImpl does something very similar to what this CL is
> doing, but it calls DeleteIncognitoDBDirectory() (being careful to schedule
the
> call on the FILE thread) in it's dtor instead of calling Shutdown().
> 
> Maybe we can use the Shutdown() method for both purposes?
> 
> void Shutdown(bool clear_local_state) {
>   DCHECK(db_tracker_thread_->BelongsToCurrentThread());
>   shutting_down_ = true;
>   is_initialized_ = false;
>   if (is_incognito_)
>     DeleteIncognitoDBDirectory();
>   else if (clear_local_state)
>     ClearLocalState();
> }
> 
> wdyt?

Done.

http://codereview.chromium.org/7234014/diff/9001/webkit/database/database_tra...
webkit/database/database_tracker.cc:832: void
DatabaseTracker::SetClearLocalStateOnExit(bool clear_local_state_on_exit) {
On 2011/07/01 03:02:34, michaeln wrote:
> It'd be safer to test and set these data members exclusively on the FILE
thread.
> 
>  if (!db_tracker_thread_->BelongsToCurrentThread()) {
>     db_tracker_thread_->PostTask(FROM_HERE,
>         NewRunnableMethod(this,
>                           &DatabaseTracker::SetClearLocalStateOnExit,
>                           clear_local_state_on_exit));
>     return;
>   }
>   clear_local_state_on_exit_ = clear_local_state_on_exit;

Done.

http://codereview.chromium.org/7234014/diff/9001/webkit/database/database_tra...
File webkit/database/database_tracker_unittest.cc (right):

http://codereview.chromium.org/7234014/diff/9001/webkit/database/database_tra...
webkit/database/database_tracker_unittest.cc:522: static void
DatabaseTrackerClearLocalStateOnExit() {
On 2011/07/01 03:02:34, michaeln wrote:
> Can you alter the test such that a file is held open when Shutdown() is
called,
> and then closed after Shutdown() has returned.

Done.

http://codereview.chromium.org/7234014/diff/9001/webkit/database/database_tra...
webkit/database/database_tracker_unittest.cc:545:
tracker->DatabaseOpened(kOrigin1, kDB1, kDescription, 0,
On 2011/07/01 03:02:34, michaeln wrote:
> Having these methods be called on something other than the 'tracker thread'
will
> prevent adding DCHECKs in the tracker class that things are being called on
that
> thread. I don't think you actually need a new thread to conduct this test. Can
> we use a 'proxy' for the current thread's message loop instead and call
> RunAllPending() as needed.

Done.

http://codereview.chromium.org/7234014/diff/9001/webkit/database/database_tra...
webkit/database/database_tracker_unittest.cc:558:
FilePath::FromWStringHack(UTF16ToWide(
On 2011/07/01 03:02:34, michaeln wrote:
> Let's avoid adding more usages of FromWStringHack.
> 
>  db_file = tracker->GetFullDBFilePath(kOriginId, kName);
>  EXPECT_TRUE(CreateDirectory(db_file.DirName()));
>  EXPECT_TRUE(EnsureFileOfSize(db_file, 1));

Done.

http://codereview.chromium.org/7234014/diff/9001/webkit/database/vfs_backend.cc
File webkit/database/vfs_backend.cc (right):

http://codereview.chromium.org/7234014/diff/9001/webkit/database/vfs_backend....
webkit/database/vfs_backend.cc:108: flags |= base::PLATFORM_FILE_SHARE_DELETE;
On 2011/07/01 03:02:34, michaeln wrote:
> Please try this on the layout test try bots too. I'm wondering if SHARE_DELETE
> will butt heads with EXCLUSIVE_READ and EXCLUSIVE_WRITE?
> 
> gcl try XXX --bot mac_layout,win_layout,linux_layout
> 
I'll do that, however, test_shell_tests is already executed on the regular bots

[jochen (gone - plz use gerrit), 2011-07-06 11:29:08]

meh, looking into it

[jochen (gone - plz use gerrit), 2011-07-07 18:23:39]

On 2011/07/06 11:29:08, jochen wrote:
> meh, looking into it

ok, should work now \o/

PTAL

[michaeln, 2011-07-07 20:25:19]

LGTM and thnx for doing this work!

[commit-bot: I haz the power, 2011-07-08 08:00:53]

Change committed as 91817