Bug 133496 – WebKit should adopt journal_mode=wal for all SQLite databases.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Andreas Kling]

<rdar://problem/15600186>

[Andreas Kling]

Created attachment 232457 [details]
Patch

[Brady Eidson]

Comment on attachment 232457 [details]
Patch

/me slaps forehead

[WebKit Commit Bot]

Comment on attachment 232457 [details]
Patch

Clearing flags on attachment: 232457

Committed r169587: <http://trac.webkit.org/changeset/169587>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #3)
> (From update of attachment 232457 [details])
> Clearing flags on attachment: 232457
> 
> Committed r169587: <http://trac.webkit.org/changeset/169587>

FYI: It broke many tests on Apple Mac WK2 bots and killed Mac WK2 EWS too.

[Alexey Proskuryakov]

And so EWS is now 24 patches behind. Finally rolling out.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 133552

[Alexey Proskuryakov]

This looks like the best crash trace from the bots: http://build.webkit.org/results/Apple%20MountainLion%20Debug%20WK2%20(Tests)/r169614%20(17957)/fast/workers/storage/open-database-while-transaction-in-progress-crash-log.txt

[Gavin Barraclough]

Created attachment 273467 [details]
Fix

[Build Bot]

Comment on attachment 273467 [details]
Fix

Attachment 273467 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/950114

New failing tests:
js/function-apply.html

[Build Bot]

Created attachment 273482 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Gavin Barraclough]

Created attachment 273523 [details]
Updated

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/sql/SQLiteFileSystem.cpp:53:  Use nullptr instead of NULL.  [readability/null] [5]
Total errors found: 1 in 3 files


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

[Darin Adler]

Comment on attachment 273523 [details]
Updated

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

> Source/WebCore/platform/sql/SQLiteDatabase.cpp:123
>              LOG_ERROR("journal_mode of database should be 'wal', but is '%s'", mode.utf8().data());

I think this should say 'WAL', not 'wal'.

> Source/WebCore/platform/sql/SQLiteFileSystem.cpp:53
> +    return sqlite3_open_v2(fileSystemRepresentation(filename).data(), database, SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE | SQLITE_OPEN_AUTOPROXY, NULL);

I agree with the style checker, we should use nullptr instead of NULL.

[Gavin Barraclough]

Transmitting file data ...
Committed revision 197922.

[Alexey Proskuryakov]

I think that this broke several WebSQL tests on iOS: https://build.webkit.org/results/Apple%20iOS%209%20Simulator%20Release%20WK2%20(Tests)/r197922%20(3855)/results.html

[WebKit Commit Bot]

Re-opened since this is blocked by bug 155340

[Gavin Barraclough]

Created attachment 276111 [details]
With testing fix

[WebKit Commit Bot]

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


ERROR: Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:1173:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:1175:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:1176:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
ERROR: Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:1172:  An else statement can be removed when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
ERROR: Source/WebCore/platform/sql/SQLiteFileSystem.cpp:53:  Use nullptr instead of NULL.  [readability/null] [5]
Total errors found: 5 in 5 files


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

[Gavin Barraclough]

Created attachment 276112 [details]
With style fix

[WebKit Commit Bot]

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


ERROR: Source/WebCore/platform/sql/SQLiteFileSystem.cpp:53:  Use nullptr instead of NULL.  [readability/null] [5]
Total errors found: 1 in 5 files


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

[Gavin Barraclough]

Created attachment 276114 [details]
With style fix

[Darin Adler]

Comment on attachment 276114 [details]
With style fix

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

> Source/WebCore/ChangeLog:33
> +            - should et SQLITE_OPEN_AUTOPROXY flag when opening database.

missing "s" in "set"

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:824
> +// This method is only intended for use by DumpRenderTree / WebKitTestRunner.
> +// Actually deleting the databases is necessary to reset to a known state before running
> +// each test case, but may be unsafe in deployment use cases (where multiple applications
> +// may be accessing the same databases concurrently).

This comment should be more closely associated with the IOSDeletionMode::Delete usage. That’s what needs the comment. In fact, I assume this is the *only* place using that flag.

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:825
>  void DatabaseTracker::deleteAllDatabases()

This should be renamed so it’s clear it’s only appropriate to use it for testing. That’s likely to be more effective than the comment.

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:831
> +        deleteOrigin(origin.get(), IOSDeletionMode::Delete);

Since this is an enum, not an enum class, there’s no need to use the IOSDeletionMode prefix here. Or we could use "enum class".

> Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:1183
>  #if !PLATFORM(IOS)
> -    return SQLiteFileSystem::deleteDatabaseFile(fullPath);
> +    UNUSED_PARAM(iOSDeletionMode);
>  #else
> -    // On the phone, other background processes may still be accessing this database.  Deleting the database directly
> -    // would nuke the POSIX file locks, potentially causing Safari/WebApp to corrupt the new db if it's running in the background.
> -    // We'll instead truncate the database file to 0 bytes.  If another process is operating on this same database file after
> -    // the truncation, it should get an error since the database file is no longer valid.  When Safari is launched
> -    // next time, it'll go through the database files and clean up any zero-bytes ones.
> -    SQLiteDatabase database;
> -    if (database.open(fullPath))
> -        return SQLiteFileSystem::truncateDatabaseFile(database.sqlite3Handle());
> -    return false;
> +    if (iOSDeletionMode == IOSDeletionMode::Truncate) {
> +        // On the phone, other background processes may still be accessing this database. Deleting the database directly
> +        // would nuke the POSIX file locks, potentially causing Safari/WebApp to corrupt the new db if it's running in the background.
> +        // We'll instead truncate the database file to 0 bytes. If another process is operating on this same database file after
> +        // the truncation, it should get an error since the database file is no longer valid. When Safari is launched
> +        // next time, it'll go through the database files and clean up any zero-bytes ones.
> +        SQLiteDatabase database;
> +        if (database.open(fullPath))
> +            return SQLiteFileSystem::truncateDatabaseFile(database.sqlite3Handle());
> +        return false;
> +    }
>  #endif

Does this code really need to be platform-specific? Why isn’t it needed on any platform other than iOS? Why, for example, does it not affect the Mac?

I also think the code would read better with early return when open fails, rather than nesting the call to truncateDatabaseFile in an if statement body.

> Source/WebCore/Modules/webdatabase/DatabaseTracker.h:101
> +    enum IOSDeletionMode {
> +        Delete,
> +        Truncate
> +    };

If we need an enum, then I think we can come up with a better name for this than iOS deletion mode. Maybe something like enum FileLockPreservation { PreserveFileLocks, BlowAwayFileLocks }? Also, I’d put this all on a single line. Vertical formatting doesn’t seem helpful.

> Source/WebCore/Modules/webdatabase/DatabaseTracker.h:105
> +    WEBCORE_EXPORT bool deleteOrigin(SecurityOrigin*, IOSDeletionMode = IOSDeletionMode::Truncate);

Since this is an enum, not an enum class, there’s no need to repeat the name of the enum; "IOSDeletionMode = Truncate" would work.

Seems a shame to expose this functionality in a public function. Can we leave the public one without the deletion mode and add a private overload? That way the enum could be private too.

> Source/WebCore/platform/sql/SQLiteDatabase.cpp:123
>              LOG_ERROR("journal_mode of database should be 'wal', but is '%s'", mode.utf8().data());

Log message should say WAL rather than 'wal', I think.

[Gavin Barraclough]

Created attachment 276132 [details]
With review fixes

[Gavin Barraclough]

(In reply to comment #23)

Most review comments addressed. Will consider renames for deleteAllDatabases & the new enum.

> Does this code really need to be platform-specific? Why isn’t it needed on
> any platform other than iOS? Why, for example, does it not affect the Mac?

I think that the current handling both for iOS and other platforms seems potentially problematic - this should be looked at further. Of the two mechanisms currently implemented, immediate deletion is most appropriate for testing needs on all platforms. For other purposes, things are less clear cut.

[Gavin Barraclough]

Okay, so I get the problem with the names Delete/Truncate – it's a little too specific about what we're doing, and not why.

However I think PreserveFileLocks/BlowAwayFileLocks are equally problematic, for the opposite reason – they speak too much to our intended outcome, and hide important specifics of the implementation that may have unintended consequences (i.e. that deleteDatabaseFile may not actually delete the file!).

I'm currently leaning towards the terminology of Soon/Immediately. 'Immediately' may also be a descriptive postfix for deleteAllDatabases to call out its different behavior from other methods.

[Gavin Barraclough]

Created attachment 276161 [details]
Patch with review comment fixes for EWS

[Gavin Barraclough]

Created attachment 276163 [details]
With review fixes for EWS

[Gavin Barraclough]

Created attachment 276164 [details]
Patch for EWS

[Gavin Barraclough]

Committed revision 199309.

[Gavin Barraclough]

Broke storage/websql/alter-to-info-table.html on iOS.
Disabled in r199381.

[Gavin Barraclough]

Created attachment 276676 [details]
Enable on iOS

[Alexey Proskuryakov]

Comment on attachment 276676 [details]
Enable on iOS

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

> LayoutTests/platform/ios-simulator/TestExpectations:2528
> +storage/websql/alter-to-info-table.html [ Failure ]

Could you please add a bugzilla/radar reference here?

[Gavin Barraclough]

Committed revision 199697.