Bug 45416 – Allow Chromium port to link with system-provided SQLite

• Component: Platform
• Status: RESOLVED WONTFIX

[Paweł Hajdan, Jr.]

It should be possible to link the Chromium port with system-provided SQLite on Linux. Currently it relies on added chromium_sqlite3 hacks.

out/Release/obj/app/libapp_base.a(connection.o): In function
`sql::Connection::Preload()':
connection.cc:(.text._ZN3sql10Connection7PreloadEv+0xcb): undefined
reference to `sqlite3Preload'
out/Release/obj/webkit/libwebcore.a(SQLiteFileSystemChromiumPosix.o): In
function `(anonymous namespace)::chromiumOpen(sqlite3_vfs*, char const*,
sqlite3_file*, int, int*)':
SQLiteFileSystemChromiumPosix.cpp:(.text._ZN12_GLOBAL__N_112chromiumOpenEP11sqlite3_vfsPKcP12sqlite3_fileiPi+0x3a):
undefined reference to `initUnixFile'
SQLiteFileSystemChromiumPosix.cpp:(.text._ZN12_GLOBAL__N_112chromiumOpenEP11sqlite3_vfsPKcP12sqlite3_fileiPi+0xfd):
undefined reference to `fillInUnixFile'

For more info see http://code.google.com/p/chromium/issues/detail?id=22208

[Paweł Hajdan, Jr.]

Created attachment 66952 [details]
patch

[Dumitru Daniliuc]

Comment on attachment 66952 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=66952&action=prettypatch

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:46
>  // Chromium's Posix implementation of SQLite VFS
we should probably mention here that this code was mostly copy-pasted from os_unix.c.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:47
>  namespace {
webkit prefers static functions/structs over anonymous namespaces.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:120
> +    
no need for empty line.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:127
> +    
no need for empty line.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:131
> +            || (sqliteIOErr == SQLITE_IOERR_UNLOCK)
keep || at the end of lines. i know the webkit style guide says to put them at the beginning of the line, but there's a lot of code that violates this rule, and i've never seen it enforced.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:133
> +            || (sqliteIOErr == SQLITE_IOERR_CHECKRESERVEDLOCK)) {
no need for {}.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:139
> +    
no need for empty line.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:142
> +    
no need for empty line.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:205
> +    struct stat statbuf;
don't think you need 'struct' here (but i might be wrong).

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:316
> +        } else if (lock.l_type != F_UNLCK) {
no need for {}.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:330
> +    struct flock lock;
don't think we need 'struct'.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:334
> +    lock.l_len = SQLiteSharedSize;
why is there no case for (pFile->fileFlags & SQLITE_WHOLE_FILE_LOCKING != 0)?

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:409
> +        && (pLock->locktype >= SQLITE_LOCK_PENDING || locktype > SQLITE_LOCK_SHARED)) {
move && to the end of the previous line. also, no need for {}.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:416
> +        && (pLock->locktype == SQLITE_LOCK_SHARED || pLock->locktype == SQLITE_LOCK_RESERVED)) {
&& on previous line.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:431
> +        || (locktype == SQLITE_LOCK_EXCLUSIVE && pFile->locktype < SQLITE_LOCK_PENDING)) {
|| on previous line.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:527
> +        } else {
no need for {}.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:543
> +
remove extra line.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:559
> +            if (rangeLock(pFile, F_RDLCK, &tErrno) == -1) {
combine these 2 if-blocks into a single one.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:623
> +static int closeChromiumFile(sqlite3_file* id)
change name to chromiumCloseNoLock(), or something similar.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:636
> +        if (close(pFile->h)) {
combine these 2 if-blocks into a single one.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:768
> +    if (fsync(pFile->h)) {
why did full_fsync() get replaced by fsync()?

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:812
> +#if PLATFORM(MAC)
this code should not be #ifdef'd. it's not specific to Mac OS in os_unix.c.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:842
> +    return 512;
define a constant for this right on top of this function. something like SQliteDefaultSectorSize should do. add a comment that it's the same as SQLITE_DEFAULT_SECTOR_SIZE, which is defined in os.h.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:928
> +    } else {
no need for {}.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:940
> +    struct stat sStat;
probably don't need 'struct' here.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:973
> +    ChromiumFile* unixSQLite3File = reinterpret_cast<ChromiumFile*>(id);
s/unixSQLite3File/chromiumFile/g.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:1009
> +    int result = fillInChromiumFile(vfs, fd, -1, id, fileName, noLock);
s/result/rc/, for consistency.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:-125
> -        *res = 1;   // if the file doesn't exist, attr < 0
leave this comment here.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:1090
>      sqlite3_vfs* unix_vfs = sqlite3_vfs_find("unix");
we don't need the "unix" VFS anymore; delete this line.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:1094
>          unix_vfs->mxPathname,
define a constant PosixMaxPathName = 512, and use that here. add a comment that it corresponds to MAX_PATHNAME defined in os_unix.c.

[Paweł Hajdan, Jr.]

Created attachment 67824 [details]
patch

Fixes review comments, will post responses to your other comments shortly.

[WebKit Review Bot]

Attachment 67824 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:129:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:130:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:131:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:403:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:409:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:424:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 7 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Paweł Hajdan, Jr.]

View in context: https://bugs.webkit.org/attachment.cgi?id=66952&action=prettypatch

Thanks for the review. The updated patch is already uploaded (from a WebKit checkout), and ready for another review. Please take a look.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:47
>  namespace {
Converted to statics. Please note the anonymous namespace was there before.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:205
> +    struct stat statbuf;
We need it for struct stat. Removal results in a compile error.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:330
> +    struct flock lock;
This one could be removed. Done.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:334
> +    lock.l_len = SQLiteSharedSize;
We don't use SQLite whole file locking, so I have removed it from this patch.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:768
> +    if (fsync(pFile->h)) {
That was the simplest implementation. We can do something more fancy on Mac, but it's more risky (I was developing this on Linux). I can add a TODO(phajdan.jr) to port the Mac logic here when I switch to the Mac checkout.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:940
> +    struct stat sStat;
Removal results in a compile error.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:1090
>      sqlite3_vfs* unix_vfs = sqlite3_vfs_find("unix");
We still need it for other things, like xDlError, xSleep, xCurrentTime, and so on.

> platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:1094
>          unix_vfs->mxPathname,
While we have unix_vfs here, I thought it would be useful to reuse it. Do you still think it's worth to duplicate this?

[Dumitru Daniliuc]

> > platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:47
> >  namespace {
> Converted to statics. Please note the anonymous namespace was there before.

yeah, i know, was one of my first webkit patches...

> > platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:768
> > +    if (fsync(pFile->h)) {
> That was the simplest implementation. We can do something more fancy on Mac, but it's more risky (I was developing this on Linux). I can add a TODO(phajdan.jr) to port the Mac logic here when I switch to the Mac checkout.

we _have_ to do exactly what sqlite's vfs does (on linux _and_ on mac). otherwise, we'll end up with subtle bugs that will take months to debug.

> > platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:1090
> >      sqlite3_vfs* unix_vfs = sqlite3_vfs_find("unix");
> We still need it for other things, like xDlError, xSleep, xCurrentTime, and so on.

we should copy-paste + modify those too, and remove any references to the "unix" VFS: either we reuse as much of it as possible, or we don't use it at all.

[Paweł Hajdan, Jr.]

Created attachment 68080 [details]
patch

Dumitru, thanks for review. I detected an additional problem with chromiumSync in previous patch versions (missing dirfd fsync call). I have ported the original logic more carefully, and tested it on the Mac. Please take another look.

[WebKit Review Bot]

Attachment 68080 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:130:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:131:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:132:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:404:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:410:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:425:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:1088:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 8 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dumitru Daniliuc]

Comment on attachment 68080 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68080&action=review

looks pretty good. a few more comments. and please check that all layout tests in LayoutTests/storage pass in chromium on linux and mac.

> WebCore/platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:749
> +static int syncWrapper(int fd, bool fullSync)

change the return type of this function to 'bool', and rename 'rc' to 'synced', or something like that.

> WebCore/platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:757
> +#if PLATFORM(MAC)
> +    if (fullSync)
> +        rc = fcntl(fd, F_FULLSYNC, 0);
> +#endif
> +    if (rc)
> +        rc = fsync(fd);

i don't think this code mirrors what's in full_fsync(). i think it should be more like:

   int rc = 1;
#if PLATFORM(MAC)
    if (fullSync)
        rc = fcntl(fd, F_FULLSYNC, 0);
    if (rc)
        rc = fsync(fd);
#else
    rc = fdatasync(fd);
#endif
    return rc;

(with the appropriate changes to make it return a boolean)

> WebCore/platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:769
> +    if (syncWrapper(pFile->h, (flags & 0x0F) == SQLITE_SYNC_FULL)) {

let's add a isFullSync = flags & 0x0F == SQLITE_SYNC_FULL variable, since we need it a few lines below too.

> WebCore/platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:1080
> +// We disallow loading DSOs inside the renderer process, so the following
> +// procedures are no-op.

1 line.

> WebCore/platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:1085
> +static void chromiumDlError(sqlite3_vfs*, int, char*)

empty lines between these functions.

> WebCore/platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:1153
> +static const int ChromiumMaxPathname = 512;

chromiumMaxPathname

[Paweł Hajdan, Jr.]

Created attachment 68364 [details]
patch

PTAL

[WebKit Review Bot]

Attachment 68364 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebCore/platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:37:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:130:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:131:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:132:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:404:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:410:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:425:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:1092:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 8 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dumitru Daniliuc]

Comment on attachment 68364 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=68364&action=review

r=me, but please patch this change into chromium __on linux and mac__, and make sure all LayoutTests/storage tests pass before you submit it. if you're not a webkit committer, please address my comment and upload a new patch, and i'll r+/cq+ it.

> WebCore/platform/sql/chromium/SQLiteFileSystemChromiumPosix.cpp:756
> +    bool success = false;
> +    if (fullSync)
> +        success = !fcntl(fd, F_FULLSYNC, 0);
> +    if (!success)
> +        success = !fsync(fd);

missing return statement for PLATFORM(MAC). please test the patch on both linux and mac.

[Paweł Hajdan, Jr.]

Created attachment 68693 [details]
patch

I have tested on Mac and Linux. It works. I have fixed one additional problem (inverted condition when checking for syncWrapper's return value - I detected it when testing).

[Dumitru Daniliuc]

Comment on attachment 68693 [details]
patch

r=me. adding to commit queue.

[WebKit Commit Bot]

Comment on attachment 68693 [details]
patch

Clearing flags on attachment: 68693

Committed r68310: <http://trac.webkit.org/changeset/68310>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Xianzhu Wang]

As the patch has been reverted (bug 53185), should we reopen this bug or open a new bug?

[Michael Nordman]

(In reply to comment #17)
> As the patch has been reverted (bug 53185), should we reopen this bug or open a new bug?

Or should we mark this as WONT_FIX? What is the compelling reason to do this?

[Xianzhu Wang]

(In reply to comment #18)
> Or should we mark this as WONT_FIX? What is the compelling reason to do this?

It's useful to reduce binary size, just like we can use many other system libraries like icu, skia, libjpeg etc. on certain platforms.

Specifically, we need this for chromium-android.

[Xianzhu Wang]

I'm sure we should reopen this bug, according to https://lists.webkit.org/pipermail/webkit-dev/2011-June/017169.html

I'll work on this bug, fix the tsan reported racing issues.

[Michael Nordman]

> It's useful to reduce binary size, just like we can use many other system libraries like icu, skia, libjpeg etc. on certain platforms.
> 
> Specifically, we need this for chromium-android.

Q1: Other than reducing size, why is this needed for chromium-android? What is different about android's library that forces the need to dynamically link with it? Or is the need purely based on reducing size?

Q2: Can we modify android's sqlite system library? That might change the shape of the answer. Specifically can android's system library export these symbols... 

// Defined in Chromium's codebase in third_party/sqlite/src/os_unix.c
extern "C" {
void chromium_sqlite3_initialize_unix_sqlite3_file(sqlite3_file* file);
int chromium_sqlite3_fill_in_unix_sqlite3_file(sqlite3_vfs* vfs, int fd, int dirfd, sqlite3_file* file, const char* fileName, int noLock);
int chromium_sqlite3_get_reusable_file_handle(sqlite3_file* file, const char* fileName, int flags, int* fd);
void chromium_sqlite3_update_reusable_file_handle(sqlite3_file* file, int fd, int flags);
void chromium_sqlite3_destroy_reusable_file_handle(sqlite3_file* file);
}

[Xianzhu Wang]

(In reply to comment #21)

> Q1: Other than reducing size, why is this needed for chromium-android? What is different about android's library that forces the need to dynamically link with it? Or is the need purely based on reducing size?
> 
> Q2: Can we modify android's sqlite system library?
> That might change the shape of the answer. Specifically can android's system library export these symbols... 
> 
> // Defined in Chromium's codebase in third_party/sqlite/src/os_unix.c
> extern "C" {
> void chromium_sqlite3_initialize_unix_sqlite3_file(sqlite3_file* file);
> int chromium_sqlite3_fill_in_unix_sqlite3_file(sqlite3_vfs* vfs, int fd, int dirfd, sqlite3_file* file, const char* fileName, int noLock);
> int chromium_sqlite3_get_reusable_file_handle(sqlite3_file* file, const char* fileName, int flags, int* fd);
> void chromium_sqlite3_update_reusable_file_handle(sqlite3_file* file, int fd, int flags);
> void chromium_sqlite3_destroy_reusable_file_handle(sqlite3_file* file);
> }

I just started to look at the issue, and haven't got much detail yet. Just think there may ght be demand for use_system_sqlite=1, with reasons just like use_system_skia=1 and use_system_libjpeg, etc.

Just read http://code.google.com/p/chromium/issues/detail?id=22208 and noticed that this is still a pending issue at chromium side. As the bug at chromium side is still open, I think we'd better also keep this one open if fixing at chromium side is not enough to fix the issue.

[Michael Nordman]

> I just started to look at the issue, and haven't got much detail yet. Just think there may ght be demand for use_system_sqlite=1, with reasons just like use_system_skia=1 and use_system_libjpeg, etc.
> 
> Just read http://code.google.com/p/chromium/issues/detail?id=22208 and noticed that this is still a pending issue at chromium side. As the bug at chromium side is still open, I think we'd better also keep this one open if fixing at chromium side is not enough to fix the issue.

Proceed with caution :)

The presence of a change request in a tracking system does not mean there is a compelling reason to do it. The reason I'm probing for a justification is because resolving this has hair on it. Pawel was the motivating force for USE_SYSTEM_SQLITE for quite some time but has since dropped it (but only after some lengthy efforts).

[Xianzhu Wang]

(In reply to comment #20)
> I'm sure we should reopen this bug, according to https://lists.webkit.org/pipermail/webkit-dev/2011-June/017169.html
> 
> I'll work on this bug, fix the tsan reported racing issues.

Sorry that I won't work on this in the coming months as chromium-android doesn't have the requirement for now.

[Eric Seidel (no email)]

Comment on attachment 68364 [details]
patch

Cleared Dumitru Daniliuc's review+ from obsolete attachment 68364 [details] so that this bug does not appear in http://webkit.org/pending-commit.