Remove VfsBackend::FileTypeIs{Journal,MainDB}

Remove VfsBackend::FileTypeIs{Journal,MainDB}

Those methods don't seem to be needed,
and they are potentially making it more difficult
to use system sqlite.

This change should not cause any regressions.

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

[Paweł Hajdan Jr., 2011-07-27 00:05:31]

Please review:

Michael: entire CL
Darin: content/ (OWNERS)

[michaeln, 2011-07-27 00:34:35]

lgtm

[darin (slow to review), 2011-07-27 16:52:06]

OK, LGTM

[Scott Hess - ex-Googler, 2011-07-27 17:31:38]

http://codereview.chromium.org/7480033/diff/1/content/browser/renderer_host/d...
File content/browser/renderer_host/database_message_filter.cc (left):

http://codereview.chromium.org/7480033/diff/1/content/browser/renderer_host/d...
content/browser/renderer_host/database_message_filter.cc:174:
VfsBackend::FileTypeIsJournal(desired_flags))
I took this code to mean that it was whitelisting the things that could
specifically be opened, in case there were other things which shouldn't be
allowed.  AFAICT this would be SQLITE_OPEN_TEMP_DB or SQLITE_OPEN_TRANSIENT_DB.

Though I'm not entirely clear why the test would be after OpenFile() if that was
important.  Just wanted to point it out, in case there are security
considerations or something, here.

[Paweł Hajdan Jr., 2011-07-27 17:54:45]

http://codereview.chromium.org/7480033/diff/1/content/browser/renderer_host/d...
File content/browser/renderer_host/database_message_filter.cc (left):

http://codereview.chromium.org/7480033/diff/1/content/browser/renderer_host/d...
content/browser/renderer_host/database_message_filter.cc:174:
VfsBackend::FileTypeIsJournal(desired_flags))
On 2011/07/27 17:31:38, shess wrote:
> I took this code to mean that it was whitelisting the things that could
> specifically be opened, in case there were other things which shouldn't be
> allowed.  AFAICT this would be SQLITE_OPEN_TEMP_DB or
SQLITE_OPEN_TRANSIENT_DB.
> 
> Though I'm not entirely clear why the test would be after OpenFile() if that
was
> important.  Just wanted to point it out, in case there are security
> considerations or something, here.

Thank you for looking. My reasoning is as follows: desired_flags come from the
renderer. A compromised renderer could pass any flags that would pass our checks
here, so I don't think they're security-related.

[michaeln, 2011-07-28 19:32:20]

http://codereview.chromium.org/7480033/diff/1/content/browser/renderer_host/d...
File content/browser/renderer_host/database_message_filter.cc (left):

http://codereview.chromium.org/7480033/diff/1/content/browser/renderer_host/d...
content/browser/renderer_host/database_message_filter.cc:174:
VfsBackend::FileTypeIsJournal(desired_flags))
> Thank you for looking. My reasoning is as follows: desired_flags come from the
> renderer. A compromised renderer could pass any flags that would pass our
checks
> here, so I don't think they're security-related.

It's not a security-related, just an attempt to reduce the number of handles the
tracker holds open. In looking at this again, i think this test should be
modified rather than removed...

if (!(desired_flags &  SQLITE_OPEN_DELETEONCLOSE))
  SaveIncognitoFileHandle();

[Paweł Hajdan Jr., 2011-07-29 21:06:34]

http://codereview.chromium.org/7480033/diff/1/content/browser/renderer_host/d...
File content/browser/renderer_host/database_message_filter.cc (left):

http://codereview.chromium.org/7480033/diff/1/content/browser/renderer_host/d...
content/browser/renderer_host/database_message_filter.cc:174:
VfsBackend::FileTypeIsJournal(desired_flags))
On 2011/07/28 19:32:20, michaeln wrote:
> > Thank you for looking. My reasoning is as follows: desired_flags come from
the
> > renderer. A compromised renderer could pass any flags that would pass our
> checks
> > here, so I don't think they're security-related.
> 
> It's not a security-related, just an attempt to reduce the number of handles
the
> tracker holds open. In looking at this again, i think this test should be
> modified rather than removed...
> 
> if (!(desired_flags &  SQLITE_OPEN_DELETEONCLOSE))
>   SaveIncognitoFileHandle();

When switching to sqlite hooks the way I planned we won't have sqlite flags in
desired_flags. On POSIX, those would be open flags.

Furthermore, in this "if" branch we're forcing SQLITE_OPEN_DELETEONCLOSE anyway
(in the call to VfsBackend::OpenFile).

[michaeln, 2011-07-30 17:53:07]

http://codereview.chromium.org/7480033/diff/1/content/browser/renderer_host/d...
File content/browser/renderer_host/database_message_filter.cc (left):

http://codereview.chromium.org/7480033/diff/1/content/browser/renderer_host/d...
content/browser/renderer_host/database_message_filter.cc:174:
VfsBackend::FileTypeIsJournal(desired_flags))
On 2011/07/29 21:06:34, Paweł Hajdan Jr. wrote:
> On 2011/07/28 19:32:20, michaeln wrote:
> > > Thank you for looking. My reasoning is as follows: desired_flags come from
> the
> > > renderer. A compromised renderer could pass any flags that would pass our
> > checks
> > > here, so I don't think they're security-related.
> > 
> > It's not a security-related, just an attempt to reduce the number of handles
> the
> > tracker holds open. In looking at this again, i think this test should be
> > modified rather than removed...
> > 
> > if (!(desired_flags &  SQLITE_OPEN_DELETEONCLOSE))
> >   SaveIncognitoFileHandle();
> 
> When switching to sqlite hooks the way I planned we won't have sqlite flags in
> desired_flags. On POSIX, those would be open flags.

That change isn't this change. Right now we have the SQLITE_XXX flags to work
with.

> Furthermore, in this "if" branch we're forcing SQLITE_OPEN_DELETEONCLOSE
anyway
> (in the call to VfsBackend::OpenFile).

Yes we do, and also call SaveInconotoFileHandle to ensure they're not deleted
until the incognito session ends. If the renderer expclitly requests
DELETE_ON_CLOSE, when the renderer closes the handle, the file should be
deleted. For that to happen we can't be stashing another open handle on the
browser side.

Line 171 doesn't alter the value of the caller's desired_flags param. If the
caller said DELETE_ON_CLOSE, we shouldn't save the open handle.