Bug 133496 - WebKit should adopt journal_mode=wal for all SQLite databases.
Summary: WebKit should adopt journal_mode=wal for all SQLite databases.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gavin Barraclough
URL:
Keywords: InRadar
Depends on: 133552 155340
Blocks:
  Show dependency treegraph
 
Reported: 2014-06-03 18:43 PDT by Andreas Kling
Modified: 2016-04-18 17:23 PDT (History)
8 users (show)

See Also:


Attachments
Patch (2.00 KB, patch)
2014-06-03 18:44 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Fix (2.12 KB, patch)
2016-03-09 13:36 PST, Gavin Barraclough
beidson: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-yosemite (925.52 KB, application/zip)
2016-03-09 14:35 PST, Build Bot
no flags Details
Updated (3.19 KB, patch)
2016-03-09 17:57 PST, Gavin Barraclough
darin: review+
Details | Formatted Diff | Diff
With testing fix (9.56 KB, patch)
2016-04-10 14:55 PDT, Gavin Barraclough
no flags Details | Formatted Diff | Diff
With style fix (9.55 KB, patch)
2016-04-10 14:58 PDT, Gavin Barraclough
no flags Details | Formatted Diff | Diff
With style fix (9.55 KB, patch)
2016-04-10 15:43 PDT, Gavin Barraclough
darin: review+
Details | Formatted Diff | Diff
With review fixes (9.04 KB, patch)
2016-04-11 00:51 PDT, Gavin Barraclough
no flags Details | Formatted Diff | Diff
Patch with review comment fixes for EWS (17.80 KB, patch)
2016-04-11 11:51 PDT, Gavin Barraclough
no flags Details | Formatted Diff | Diff
With review fixes for EWS (17.82 KB, patch)
2016-04-11 12:06 PDT, Gavin Barraclough
no flags Details | Formatted Diff | Diff
Patch for EWS (17.86 KB, patch)
2016-04-11 12:23 PDT, Gavin Barraclough
no flags Details | Formatted Diff | Diff
Enable on iOS (3.29 KB, patch)
2016-04-18 15:53 PDT, Gavin Barraclough
beidson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2014-06-03 18:43:01 PDT
<rdar://problem/15600186>
Comment 1 Andreas Kling 2014-06-03 18:44:57 PDT
Created attachment 232457 [details]
Patch
Comment 2 Brady Eidson 2014-06-03 20:56:59 PDT
Comment on attachment 232457 [details]
Patch

/me slaps forehead
Comment 3 WebKit Commit Bot 2014-06-04 09:24:34 PDT
Comment on attachment 232457 [details]
Patch

Clearing flags on attachment: 232457

Committed r169587: <http://trac.webkit.org/changeset/169587>
Comment 4 WebKit Commit Bot 2014-06-04 09:24:37 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Csaba Osztrogonác 2014-06-05 08:25:30 PDT
(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.
Comment 6 Alexey Proskuryakov 2014-06-05 10:17:55 PDT
And so EWS is now 24 patches behind. Finally rolling out.
Comment 7 WebKit Commit Bot 2014-06-05 10:19:06 PDT
Re-opened since this is blocked by bug 133552
Comment 9 Gavin Barraclough 2016-03-09 13:36:48 PST
Created attachment 273467 [details]
Fix
Comment 10 Build Bot 2016-03-09 14:35:02 PST
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
Comment 11 Build Bot 2016-03-09 14:35:06 PST
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
Comment 12 Gavin Barraclough 2016-03-09 17:57:27 PST
Created attachment 273523 [details]
Updated
Comment 13 WebKit Commit Bot 2016-03-09 17:59:34 PST
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.
Comment 14 Darin Adler 2016-03-09 20:31:36 PST
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.
Comment 15 Gavin Barraclough 2016-03-09 21:18:12 PST
Transmitting file data ...
Committed revision 197922.
Comment 16 Alexey Proskuryakov 2016-03-09 21:54:33 PST
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
Comment 17 WebKit Commit Bot 2016-03-10 17:28:10 PST
Re-opened since this is blocked by bug 155340
Comment 18 Gavin Barraclough 2016-04-10 14:55:12 PDT
Created attachment 276111 [details]
With testing fix
Comment 19 WebKit Commit Bot 2016-04-10 14:55:46 PDT
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.
Comment 20 Gavin Barraclough 2016-04-10 14:58:55 PDT
Created attachment 276112 [details]
With style fix
Comment 21 WebKit Commit Bot 2016-04-10 15:00:16 PDT
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.
Comment 22 Gavin Barraclough 2016-04-10 15:43:46 PDT
Created attachment 276114 [details]
With style fix
Comment 23 Darin Adler 2016-04-10 16:34:45 PDT
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.
Comment 24 Gavin Barraclough 2016-04-11 00:51:06 PDT
Created attachment 276132 [details]
With review fixes
Comment 25 Gavin Barraclough 2016-04-11 01:01:05 PDT
(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.
Comment 26 Gavin Barraclough 2016-04-11 11:17:39 PDT
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.
Comment 27 Gavin Barraclough 2016-04-11 11:51:24 PDT
Created attachment 276161 [details]
Patch with review comment fixes for EWS
Comment 28 Gavin Barraclough 2016-04-11 12:06:11 PDT
Created attachment 276163 [details]
With review fixes for EWS
Comment 29 Gavin Barraclough 2016-04-11 12:23:45 PDT
Created attachment 276164 [details]
Patch for EWS
Comment 30 Gavin Barraclough 2016-04-11 13:11:09 PDT
Committed revision 199309.
Comment 31 Gavin Barraclough 2016-04-12 12:38:12 PDT
Broke storage/websql/alter-to-info-table.html on iOS.
Disabled in r199381.
Comment 32 Gavin Barraclough 2016-04-18 15:53:51 PDT
Created attachment 276676 [details]
Enable on iOS
Comment 33 Alexey Proskuryakov 2016-04-18 16:03:34 PDT
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?
Comment 34 Gavin Barraclough 2016-04-18 17:23:00 PDT
Committed revision 199697.