Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(622)

Issue 7480033: Remove VfsBackend::FileTypeIs{Journal,MainDB} (Closed)

Created:
9 years, 5 months ago by Paweł Hajdan Jr.
Modified:
9 years, 4 months ago
CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+1 line, -19 lines) Patch
M content/browser/renderer_host/database_message_filter.cc View 1 chunk +1 line, -3 lines 5 comments Download
M webkit/database/vfs_backend.h View 1 chunk +0 lines, -2 lines 0 comments Download
M webkit/database/vfs_backend.cc View 1 chunk +0 lines, -14 lines 0 comments Download

Messages

Total messages: 8 (0 generated)
Paweł Hajdan Jr.
Please review: Michael: entire CL Darin: content/ (OWNERS)
9 years, 5 months ago (2011-07-27 00:05:31 UTC) #1
michaeln
lgtm
9 years, 5 months ago (2011-07-27 00:34:35 UTC) #2
darin (slow to review)
OK, LGTM
9 years, 5 months ago (2011-07-27 16:52:06 UTC) #3
Scott Hess - ex-Googler
http://codereview.chromium.org/7480033/diff/1/content/browser/renderer_host/database_message_filter.cc File content/browser/renderer_host/database_message_filter.cc (left): http://codereview.chromium.org/7480033/diff/1/content/browser/renderer_host/database_message_filter.cc#oldcode174 content/browser/renderer_host/database_message_filter.cc:174: VfsBackend::FileTypeIsJournal(desired_flags)) I took this code to mean that it ...
9 years, 5 months ago (2011-07-27 17:31:38 UTC) #4
Paweł Hajdan Jr.
http://codereview.chromium.org/7480033/diff/1/content/browser/renderer_host/database_message_filter.cc File content/browser/renderer_host/database_message_filter.cc (left): http://codereview.chromium.org/7480033/diff/1/content/browser/renderer_host/database_message_filter.cc#oldcode174 content/browser/renderer_host/database_message_filter.cc:174: VfsBackend::FileTypeIsJournal(desired_flags)) On 2011/07/27 17:31:38, shess wrote: > I took ...
9 years, 5 months ago (2011-07-27 17:54:45 UTC) #5
michaeln
http://codereview.chromium.org/7480033/diff/1/content/browser/renderer_host/database_message_filter.cc File content/browser/renderer_host/database_message_filter.cc (left): http://codereview.chromium.org/7480033/diff/1/content/browser/renderer_host/database_message_filter.cc#oldcode174 content/browser/renderer_host/database_message_filter.cc:174: VfsBackend::FileTypeIsJournal(desired_flags)) > Thank you for looking. My reasoning is ...
9 years, 4 months ago (2011-07-28 19:32:20 UTC) #6
Paweł Hajdan Jr.
http://codereview.chromium.org/7480033/diff/1/content/browser/renderer_host/database_message_filter.cc File content/browser/renderer_host/database_message_filter.cc (left): http://codereview.chromium.org/7480033/diff/1/content/browser/renderer_host/database_message_filter.cc#oldcode174 content/browser/renderer_host/database_message_filter.cc:174: VfsBackend::FileTypeIsJournal(desired_flags)) On 2011/07/28 19:32:20, michaeln wrote: > > Thank ...
9 years, 4 months ago (2011-07-29 21:06:34 UTC) #7
michaeln
9 years, 4 months ago (2011-07-30 17:53:07 UTC) #8
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.

Powered by Google App Engine
This is Rietveld 408576698