Bug 26940 – [Chromium] Add a SQLite VFS for Chromium

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dumitru Daniliuc]

Because of sandboxed renderer processes, Chromium needs a special SQLite VFS implementation.

[Dumitru Daniliuc]

Created attachment 32205 [details]
win patch

SQLite VFS: only the code that deals directly with files was slightly modified.
FileSystem: openDatabase() uses "chromium_vfs" to open files, all other functions are either very simple or delegate to ChromiumBridge.

[Dumitru Daniliuc]

Created attachment 32206 [details]
win patch

Attached the wrong patch...

[Dumitru Daniliuc]

Comment on attachment 32206 [details]
win patch

> +        } else {
> +            return SQLITE_CANTOPEN;
> +	}

'}' is not aligned properly. i will fix that with the other comments you might have (don't want to upload another patch for this only).

[Dimitri Glazkov (Google)]

Comment on attachment 32206 [details]
win patch

> Index: WebCore/platform/sql/chromium/SQLiteFileSystemChromium.cpp

If this is a Windows-specific file, it should be SQLiteFileSystemChromiumWin.cpp

> +        if (desiredFlags & SQLITE_OPEN_READWRITE) {
> +            int newFlags = (desiredFlags | SQLITE_OPEN_READONLY) & ~SQLITE_OPEN_READWRITE;
> +            return chromiumOpen(0, fileName, id, newFlags, usedFlags);
> +        } else {
> +            return SQLITE_CANTOPEN;
> +	}

Fix indentation, no need for brackets for one-liners.

> +#ifndef SQLITE_OMIT_LOAD_EXTENSION
> +// Returns NULL, thus disallowing loading libraries in the renderer process.
> +//
> +// vfs - pointer to the sqlite3_vfs object.
> +// fileName - the name of the shared library file.
> +void* chromiumDlOpen(sqlite3_vfs* vfs, const char* fileName)
> +{
> +    return NULL;

return 0;

> +    static sqlite3_vfs chromium_vfs = {
> +      1,
> +      win32_vfs->szOsFile,
> +      win32_vfs->mxPathname,
> +      0,
> +      "chromium_vfs",
> +      0,
> +      chromiumOpen,
> +      chromiumDelete,
> +      chromiumAccess,
> +      chromiumFullPathname,
> +      chromiumDlOpen,
> +      win32_vfs->xDlError,
> +      win32_vfs->xDlSym,
> +      win32_vfs->xDlClose,
> +      win32_vfs->xRandomness,
> +      win32_vfs->xSleep,
> +      win32_vfs->xCurrentTime,
> +      win32_vfs->xGetLastError

4 space indent.

> +
> +bool SQLiteFileSystem::ensureDatabaseDirectoryExists(const String& path)
> +{

Should probably eliminate unused param for consistency.

> +bool SQLiteFileSystem::ensureDatabaseFileExists(const String& fileName, bool checkPathOnly)
> +{

Ditto.

> +bool SQLiteFileSystem::deleteEmptyDatabaseDirectory(const String& path)
> +{

Ditto.

[Dimitri Glazkov (Google)]

Also, instead of including windows.h and having platform-specific #ifdef in ChromiumBridge, just use PlatformFileHandle from FileSystem.h.

[Dumitru Daniliuc]

Created attachment 32338 [details]
win patch

[Dumitru Daniliuc]

(In reply to comment #4)
> (From update of attachment 32206 [details])
> > Index: WebCore/platform/sql/chromium/SQLiteFileSystemChromium.cpp
> 
> If this is a Windows-specific file, it should be
> SQLiteFileSystemChromiumWin.cpp
> 
> > +        if (desiredFlags & SQLITE_OPEN_READWRITE) {
> > +            int newFlags = (desiredFlags | SQLITE_OPEN_READONLY) & ~SQLITE_OPEN_READWRITE;
> > +            return chromiumOpen(0, fileName, id, newFlags, usedFlags);
> > +        } else {
> > +            return SQLITE_CANTOPEN;
> > +	}
> 
> Fix indentation, no need for brackets for one-liners.

removed the brackets.

> > +#ifndef SQLITE_OMIT_LOAD_EXTENSION
> > +// Returns NULL, thus disallowing loading libraries in the renderer process.
> > +//
> > +// vfs - pointer to the sqlite3_vfs object.
> > +// fileName - the name of the shared library file.
> > +void* chromiumDlOpen(sqlite3_vfs* vfs, const char* fileName)
> > +{
> > +    return NULL;
> 
> return 0;

done.

> > +    static sqlite3_vfs chromium_vfs = {
> > +      1,
> > +      win32_vfs->szOsFile,
> > +      win32_vfs->mxPathname,
> > +      0,
> > +      "chromium_vfs",
> > +      0,
> > +      chromiumOpen,
> > +      chromiumDelete,
> > +      chromiumAccess,
> > +      chromiumFullPathname,
> > +      chromiumDlOpen,
> > +      win32_vfs->xDlError,
> > +      win32_vfs->xDlSym,
> > +      win32_vfs->xDlClose,
> > +      win32_vfs->xRandomness,
> > +      win32_vfs->xSleep,
> > +      win32_vfs->xCurrentTime,
> > +      win32_vfs->xGetLastError
> 
> 4 space indent.

done.

> > +
> > +bool SQLiteFileSystem::ensureDatabaseDirectoryExists(const String& path)
> > +{
> 
> Should probably eliminate unused param for consistency.

fixed everywhere.

> > +bool SQLiteFileSystem::ensureDatabaseFileExists(const String& fileName, bool checkPathOnly)
> > +{
> 
> Ditto.

done.

> > +bool SQLiteFileSystem::deleteEmptyDatabaseDirectory(const String& path)
> > +{
> 
> Ditto.

done.

> Also, instead of including windows.h and having platform-specific #ifdef in
> ChromiumBridge, just use PlatformFileHandle from FileSystem.h.

i didn't change this.

from what i understand, webkit has only one chromium port: PLATFORM(CHROMIUM). it doesn't know how to differentiate between chromium compiled on windows and chromium compiled on linux/mac. so we can't use #defines in FileSystem.h. basically, if we define something in FileSystem.h for PLATFORM(CHROMIUM), we'd have to use those definitions for all OSes we're compiling chromium on, and it just doesn't work.

i talked a bit to john about this and he said that separate, per-OS function signatures should be fine in this case.

let me know what you think.

[Dumitru Daniliuc]

After talking to Darin, it looks like we can use PLATFORM(WIN_OS) (instead of PLATFORM(WIN)) in WebKit to get some code in all ports when built on a Windows OS. Patch pending: https://bugs.webkit.org/show_bug.cgi?id=27013.

[Dumitru Daniliuc]

Created attachment 32476 [details]
win patch

[Dimitri Glazkov (Google)]

Comment on attachment 32476 [details]
win patch

Looking much prettier -- two minor issues below:

> +String SQLiteFileSystem::appendDatabaseFileNameToPath(const String& path, const String& fileName)
> +{
> +    if (path.isEmpty())
> +        return fileName;
> +    else
> +        return path + "\\" + fileName;
> +}

Whoops. This is a Windows-ism. Can we use FileSystem.h magic here?

> +int chromiumDelete(sqlite3_vfs* vfs, const char* fileName, int syncDir)

unused param syncDir?

[Dumitru Daniliuc]

Created attachment 32482 [details]
win patch

[Dimitri Glazkov (Google)]

Land tomorrow?

[Dumitru Daniliuc]

Created attachment 32546 [details]
win patch

minor bug fix in appendDatabaseFileNameToPath() in SQLiteFileSystemChromium.cpp.

[Dumitru Daniliuc]

Created attachment 32588 [details]
win patch

same patch + WebCore/WebCore.gypi changes.

[Dimitri Glazkov (Google)]

Comment on attachment 32588 [details]
win patch

Please add WebCore.gypi changes and we'll be good to land.

[Nate Chapin]

Landed as r45959.

[Dumitru Daniliuc]

Created attachment 32967 [details]
win patch + mac/linux stubs

Same patch + stubs for Mac/Linux.

[Dimitri Glazkov (Google)]

Comment on attachment 32967 [details]
win patch + mac/linux stubs

ok.

[Dimitri Glazkov (Google)]

Eric, can you coordinate w/Dumi to land this?

[Dumitru Daniliuc]

Created attachment 32980 [details]
win VFS + mac/linux stubs

Last patch for this bug hopefully...

[Dumitru Daniliuc]

Created attachment 32983 [details]
same patch

[Dimitri Glazkov (Google)]

Comment on attachment 32983 [details]
same patch

good deal. Let's land this puppy.

[Jeremy Orlow]

A note to any prospective committers: this patch is going to require special landing in conjunction with Chromium.  Right now, we're just waiting for the tree to go green.

[Eric Roman]

landed r46126.

[zooz hada]

Comment on attachment 32206 [details]
win patch

32206

[zooz hada]

32206

[John]

nice