Support WebSQLDatabases in incognito mode.

Support WebSQLDatabases in incognito mode.

BUG=43232
TEST=none


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

[michaeln, 2010-06-08 16:29:37]

Nice looking CL! Can you also try this on the layout bots.

[darin (slow to review), 2010-06-08 19:04:05]

LGTM2

http://codereview.chromium.org/2746003/diff/17001/18002
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/2746003/diff/17001/18002#newcode173
chrome/browser/renderer_host/database_dispatcher_host.cc:173: //    a handle to
this DB file. If the database tracker knows has an open
nit: "knows has" -> "knows it has" ?

[dumi, 2010-06-08 20:35:35]

michael: the layout tests cannot test this code path, because there's no way to
force them to run in incognito mode. however, as we discussed, i changed the
code to always think it's in incognito mode and all layout tests passed when run
manually in chrome or when run with run_webkit_tests in test_shell.

http://codereview.chromium.org/2746003/diff/17001/18002
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/2746003/diff/17001/18002#newcode173
chrome/browser/renderer_host/database_dispatcher_host.cc:173: //    a handle to
this DB file. If the database tracker knows has an open
On 2010/06/08 19:04:05, darin wrote:
> nit: "knows has" -> "knows it has" ?

done: "knows has" -> "has".

[michaeln, 2010-06-08 23:33:07]

Right... there's no good way to test this via test_shell since the dup'ing logic
db_dispatcher_host.

Please think thru how to handle journal files too like we talked about, a
journal file left over by a crashing renderer should not get deleted when the
renderer crashes. A later consumer may need that file to 'rollback' what was in
progress at the time of the crash.

http://codereview.chromium.org/2746003/diff/17001/18005
File webkit/database/database_tracker.h (right):

http://codereview.chromium.org/2746003/diff/17001/18005#newcode164
webkit/database/database_tracker.h:164: void SaveFileHandle(const string16&
vfs_file_path,
Since these methods are only used for incognito, consider renaming them...
Get/SaveIncoginitoFileHandle()

http://codereview.chromium.org/2746003/diff/17001/18005#newcode258
webkit/database/database_tracker.h:258: FileHandlesMap file_handles_;
consider renaming to incognito_file_handles_

http://codereview.chromium.org/2746003/diff/17001/18007
File webkit/database/vfs_backend.cc (right):

http://codereview.chromium.org/2746003/diff/17001/18007#newcode30
webkit/database/vfs_backend.cc:30: // Duplicate the file handle.
indents are off inside the #if block, should be backed up one tab stop

[dumi, 2010-06-09 02:56:50]

added a bit more code to handle journal files, like we discussed: hold an open
DELETE_ON_CLOSE handle to them too and close it when the renderer asks to delete
it; if the renderer crashes, the handle will still be open, and the next
renderer that wants to use that DB will see it, roll it back and close it.
tested the code on a crashing renderer and everything worked nicely.

http://codereview.chromium.org/2746003/diff/17001/18005
File webkit/database/database_tracker.h (right):

http://codereview.chromium.org/2746003/diff/17001/18005#newcode164
webkit/database/database_tracker.h:164: void SaveFileHandle(const string16&
vfs_file_path,
On 2010/06/08 23:33:08, michaeln wrote:
> Since these methods are only used for incognito, consider renaming them...
> Get/SaveIncoginitoFileHandle()

done.

http://codereview.chromium.org/2746003/diff/17001/18005#newcode258
webkit/database/database_tracker.h:258: FileHandlesMap file_handles_;
On 2010/06/08 23:33:08, michaeln wrote:
> consider renaming to incognito_file_handles_

done.

http://codereview.chromium.org/2746003/diff/17001/18007
File webkit/database/vfs_backend.cc (right):

http://codereview.chromium.org/2746003/diff/17001/18007#newcode30
webkit/database/vfs_backend.cc:30: // Duplicate the file handle.
On 2010/06/08 23:33:08, michaeln wrote:
> indents are off inside the #if block, should be backed up one tab stop

fixed.

[dumi, 2010-06-09 03:04:45]

Forgot to mention that it looks like we don't need to worry about the user
trying to clear browsing data in incognito mode, because the cookie tree is
always built using the non-incognito profile.

[michaeln, 2010-06-09 19:07:53]

http://codereview.chromium.org/2746003/diff/20004/31007
File webkit/database/database_util.cc (right):

http://codereview.chromium.org/2746003/diff/20004/31007#newcode47
webkit/database/database_util.cc:47: static const string16 JOURNAL_FILE_SUFFIX =
ASCIIToUTF16("-journal");
Please don't use a global non-primitive type for this constant. We want to avoid
running ctor and string conversion logic at library load time. I think you could
define the const value in the method body.

http://codereview.chromium.org/2746003/diff/38002/41002
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/2746003/diff/38002/41002#newcode209
chrome/browser/renderer_host/database_dispatcher_host.cc:209: bool auto_close =
Maybe this logic could be simplified and better kept in sync with the logic that
saves the handles above by...

auto_close = !tracker->HasSavedIncognitoFile(vfs_file_name);

wdyt?

http://codereview.chromium.org/2746003/diff/38002/41004
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/2746003/diff/38002/41004#newcode60
webkit/database/database_tracker.cc:60: // so it's a good time to check if
there's any thingleft from the
thingleft

http://codereview.chromium.org/2746003/diff/38002/41004#newcode62
webkit/database/database_tracker.cc:62: DeleteIncognitoDBDirectory();
Since this object is constructed on the UI thread we probably want to avoid file
system calls. Ditto for the dtor.

[dumi, 2010-06-10 02:36:26]

other changes:
1. in incognito mode the directory names are not the same as the
origin_identifiers. instead, they're just numbers.
2. in incognito mode the tracker database is stored in memory, so we don't need
to encrypt/hash the data we store in it.

http://codereview.chromium.org/2746003/diff/20004/31007
File webkit/database/database_util.cc (right):

http://codereview.chromium.org/2746003/diff/20004/31007#newcode47
webkit/database/database_util.cc:47: static const string16 JOURNAL_FILE_SUFFIX =
ASCIIToUTF16("-journal");
On 2010/06/09 19:07:54, michaeln wrote:
> Please don't use a global non-primitive type for this constant. We want to
avoid
> running ctor and string conversion logic at library load time. I think you
could
> define the const value in the method body.

done.

http://codereview.chromium.org/2746003/diff/38002/41002
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/2746003/diff/38002/41002#newcode209
chrome/browser/renderer_host/database_dispatcher_host.cc:209: bool auto_close =
On 2010/06/09 19:07:54, michaeln wrote:
> Maybe this logic could be simplified and better kept in sync with the logic
that
> saves the handles above by...
> 
> auto_close = !tracker->HasSavedIncognitoFile(vfs_file_name);
> 
> wdyt?

done.

http://codereview.chromium.org/2746003/diff/38002/41004
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/2746003/diff/38002/41004#newcode60
webkit/database/database_tracker.cc:60: // so it's a good time to check if
there's any thingleft from the
On 2010/06/09 19:07:54, michaeln wrote:
> thingleft

fixed.

http://codereview.chromium.org/2746003/diff/38002/41004#newcode62
webkit/database/database_tracker.cc:62: DeleteIncognitoDBDirectory();
On 2010/06/09 19:07:54, michaeln wrote:
> Since this object is constructed on the UI thread we probably want to avoid
file
> system calls. Ditto for the dtor.

fixed. see the changes to profile.cc.

[michaeln, 2010-06-11 00:31:53]

My biggest question is do we want to incur the extra file io at startup time for
this? I would vote "no".

http://codereview.chromium.org/2746003/diff/2002/11002
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/2746003/diff/2002/11002#newcode312
chrome/browser/profile.cc:312: false));
Since we've already done this when constructing the 'real' profile, i wonder if
we can skip doing it here.

http://codereview.chromium.org/2746003/diff/2002/11002#newcode984
chrome/browser/profile.cc:984:
&webkit_database::DatabaseTracker::DeleteIncognitoDBDirectory,
Since the only artifacts that can be left lingering are empty directories with
unidentifiable names, maybe we can skip this call at construction time
altogether to avoid a little file io at startup time.

http://codereview.chromium.org/2746003/diff/2002/11003
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/2746003/diff/2002/11003#newcode177
chrome/browser/renderer_host/database_dispatcher_host.cc:177: //    we store it
in the database tracker in case we need it later.
These comments describe what code is doing more than why its doing it. What the
code is doing is pretty apparent from reading the code. The notable bit is the
use of the DELETEONCLOSE flag for all files when in incognito. Consider removing
most of these comments and just leaving the smaller comment about that flags
usage for incognito.

http://codereview.chromium.org/2746003/diff/2002/11003#newcode256
chrome/browser/renderer_host/database_dispatcher_host.cc:256: error_code =
SQLITE_OK;
The return value is a little tricky, because we don't actually know if we've
deleted the file or not, and the nature of the plaform differences make it
difficult to test one way or the other?

If this tracker's handle is the last open handle, then we will have deleted the
file... but if another handle remains open elsewhere... the file will not
actually have been deleted.

Not sure what we can do about that?

http://codereview.chromium.org/2746003/diff/2002/11003#newcode258
chrome/browser/renderer_host/database_dispatcher_host.cc:258: error_code =
VfsBackend::DeleteFile(db_file, sync_dir);
Do we ever want to run this DeleteFile call when IsIncognito? All of the files
are 'deleteOnClosed'. Maybe structure this method more like...

if (IsIncoginito()) {

   CloseIncognitoFileHandle();
   // make up a return value???

} else {
  error_code = VfsBackend::DeleteFile(db_file, sync_dir);
}

... we may not need the DatabaseUtil::IsJournalFile method?

http://codereview.chromium.org/2746003/diff/2002/11005
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/2746003/diff/2002/11005#newcode189
webkit/database/database_tracker.cc:189: const string16& origin_identifier) {
indent

http://codereview.chromium.org/2746003/diff/2002/11005#newcode618
webkit/database/database_tracker.cc:618: incognito_file_handles_.end());
Not sure we want this DCHECK given how we're using it. The renderer could
conceivalby ask us to delete something that was never opened.

http://codereview.chromium.org/2746003/diff/2002/11005#newcode644
webkit/database/database_tracker.cc:644: if (is_initialized_) {
Should we also reset meta_table_ scoped ptr and others in here?

http://codereview.chromium.org/2746003/diff/2002/11006
File webkit/database/database_tracker.h (right):

http://codereview.chromium.org/2746003/diff/2002/11006#newcode271
webkit/database/database_tracker.h:271: // this map to assign directory names
that do not reveal this information.
Nice comments!

http://codereview.chromium.org/2746003/diff/2002/11010
File webkit/database/vfs_backend.cc (right):

http://codereview.chromium.org/2746003/diff/2002/11010#newcode26
webkit/database/vfs_backend.cc:26: if (file_handle ==
base::kInvalidPlatformFileValue)
Maybe DCHECK(*target_handle == kInvalidPlatformFileValue) or make that
assignment here.

[dumi, 2010-06-11 03:32:34]

http://codereview.chromium.org/2746003/diff/2002/11002
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/2746003/diff/2002/11002#newcode312
chrome/browser/profile.cc:312: false));
On 2010/06/11 00:31:54, michaeln wrote:
> Since we've already done this when constructing the 'real' profile, i wonder
if
> we can skip doing it here.

removed from both constructors.

http://codereview.chromium.org/2746003/diff/2002/11002#newcode984
chrome/browser/profile.cc:984:
&webkit_database::DatabaseTracker::DeleteIncognitoDBDirectory,
On 2010/06/11 00:31:54, michaeln wrote:
> Since the only artifacts that can be left lingering are empty directories with
> unidentifiable names, maybe we can skip this call at construction time
> altogether to avoid a little file io at startup time.

done.

http://codereview.chromium.org/2746003/diff/2002/11003
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/2746003/diff/2002/11003#newcode177
chrome/browser/renderer_host/database_dispatcher_host.cc:177: //    we store it
in the database tracker in case we need it later.
On 2010/06/11 00:31:54, michaeln wrote:
> These comments describe what code is doing more than why its doing it. What
the
> code is doing is pretty apparent from reading the code. The notable bit is the
> use of the DELETEONCLOSE flag for all files when in incognito. Consider
removing
> most of these comments and just leaving the smaller comment about that flags
> usage for incognito.

done.

http://codereview.chromium.org/2746003/diff/2002/11003#newcode256
chrome/browser/renderer_host/database_dispatcher_host.cc:256: error_code =
SQLITE_OK;
On 2010/06/11 00:31:54, michaeln wrote:
> The return value is a little tricky, because we don't actually know if we've
> deleted the file or not, and the nature of the plaform differences make it
> difficult to test one way or the other?
> 
> If this tracker's handle is the last open handle, then we will have deleted
the
> file... but if another handle remains open elsewhere... the file will not
> actually have been deleted.
> 
> Not sure what we can do about that?

it's a journal file, there's only one renderer that can have a handle to it. so
unless the user manually opens a handle to this file, we should be fine (but
then the user can also copy the files to a more permanent location and we
wouldn't know about them either). so i think returning SQLITE_OK here is
perfectly fine.

http://codereview.chromium.org/2746003/diff/2002/11003#newcode258
chrome/browser/renderer_host/database_dispatcher_host.cc:258: error_code =
VfsBackend::DeleteFile(db_file, sync_dir);
On 2010/06/11 00:31:54, michaeln wrote:
> Do we ever want to run this DeleteFile call when IsIncognito? All of the files
> are 'deleteOnClosed'. Maybe structure this method more like...
> 
> if (IsIncoginito()) {
> 
>    CloseIncognitoFileHandle();
>    // make up a return value???
> 
> } else {
>   error_code = VfsBackend::DeleteFile(db_file, sync_dir);
> }
> 
> ... we may not need the DatabaseUtil::IsJournalFile method?

removed the DatabaseUtil::IsJournalFile().

http://codereview.chromium.org/2746003/diff/2002/11005
File webkit/database/database_tracker.cc (right):

http://codereview.chromium.org/2746003/diff/2002/11005#newcode189
webkit/database/database_tracker.cc:189: const string16& origin_identifier) {
On 2010/06/11 00:31:54, michaeln wrote:
> indent

fixed... i hate VS's editor...

http://codereview.chromium.org/2746003/diff/2002/11005#newcode618
webkit/database/database_tracker.cc:618: incognito_file_handles_.end());
On 2010/06/11 00:31:54, michaeln wrote:
> Not sure we want this DCHECK given how we're using it. The renderer could
> conceivalby ask us to delete something that was never opened.

these calls come from the VFS layer only, and a delete without an open should
never happen.

http://codereview.chromium.org/2746003/diff/2002/11005#newcode644
webkit/database/database_tracker.cc:644: if (is_initialized_) {
On 2010/06/11 00:31:54, michaeln wrote:
> Should we also reset meta_table_ scoped ptr and others in here?

i don't think it matters. all methods that use meta_table_ & co. should call
LazyInit() first to make sure these tables are initialized, and since
shutting_down_ == true, LazyInit() will return false. the only reason
db_->Close() was here was to make sure we can remove databases-incognito/, but
now that the tracker DB is in memory, we don't need this either.

http://codereview.chromium.org/2746003/diff/2002/11010
File webkit/database/vfs_backend.cc (right):

http://codereview.chromium.org/2746003/diff/2002/11010#newcode26
webkit/database/vfs_backend.cc:26: if (file_handle ==
base::kInvalidPlatformFileValue)
On 2010/06/11 00:31:54, michaeln wrote:
> Maybe DCHECK(*target_handle == kInvalidPlatformFileValue) or make that
> assignment here.

added the assignment.

[michaeln, 2010-06-11 19:44:37]

Please look at this pair of comments... otherwise LGTM

http://codereview.chromium.org/2746003/diff/7003/54003
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/2746003/diff/7003/54003#newcode252
chrome/browser/renderer_host/database_dispatcher_host.cc:252: if ((error_code ==
SQLITE_IOERR_DELETE) && reschedule_count) {
I'm not sure if this retry logic is relevant for incognito or not (although i
think not)? If not, consider moving this into the else clause above.

http://codereview.chromium.org/2746003/diff/7003/54008
File webkit/database/database_util.cc (right):

http://codereview.chromium.org/2746003/diff/7003/54008#newcode7
webkit/database/database_util.cc:7: #include "base/string_util.h"
needed?

[dumi, 2010-06-11 20:16:17]

http://codereview.chromium.org/2746003/diff/7003/54003
File chrome/browser/renderer_host/database_dispatcher_host.cc (right):

http://codereview.chromium.org/2746003/diff/7003/54003#newcode252
chrome/browser/renderer_host/database_dispatcher_host.cc:252: if ((error_code ==
SQLITE_IOERR_DELETE) && reschedule_count) {
On 2010/06/11 19:44:37, michaeln wrote:
> I'm not sure if this retry logic is relevant for incognito or not (although i
> think not)? If not, consider moving this into the else clause above.

i don't know if we have any guarantees that closing a file handle will always
succeed, so i think we should keep the logic here. also, as we discussed, i
don't think we should run into any case where we're trying to close a handle to
a file that's not stored in database tracker (in which case retrying would be
pointless).

http://codereview.chromium.org/2746003/diff/7003/54008
File webkit/database/database_util.cc (right):

http://codereview.chromium.org/2746003/diff/7003/54008#newcode7
webkit/database/database_util.cc:7: #include "base/string_util.h"
On 2010/06/11 19:44:37, michaeln wrote:
> needed?

nope, removed.

[michaeln, 2010-06-11 20:18:39]

still LGTM