Bug 201002 - Try to recover nicely when getting an unexpected schema in the service workers database
Summary: Try to recover nicely when getting an unexpected schema in the service worker...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-08-21 15:35 PDT by Chris Dumez
Modified: 2019-08-22 16:28 PDT (History)
5 users (show)

See Also:


Attachments
Patch (9.55 KB, patch)
2019-08-21 15:38 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (11.32 KB, patch)
2019-08-21 15:49 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (13.13 KB, patch)
2019-08-22 12:03 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (13.13 KB, patch)
2019-08-22 12:31 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2019-08-21 15:35:28 PDT
Try to recover nicely when getting an unexpected schema in the service workers database instead of crashing the network process.
Comment 1 Radar WebKit Bug Importer 2019-08-21 15:35:47 PDT
<rdar://problem/54574991>
Comment 2 Chris Dumez 2019-08-21 15:38:08 PDT
Created attachment 376935 [details]
Patch
Comment 3 Chris Dumez 2019-08-21 15:49:25 PDT
Created attachment 376941 [details]
Patch
Comment 4 youenn fablet 2019-08-22 00:54:42 PDT
Comment on attachment 376941 [details]
Patch

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

> Source/WebCore/workers/service/server/RegistrationDatabase.cpp:173
> +        openSQLiteDatabase(fullFilename);

It seems a bit strange to be able to recover ensureValidRecordsTable but not importRecords error.
An error in importRecords is probably unrecoverable as well. In that case, we currently proceed with zero registration but we will no longer be able to push changes since push changes try to import before pushing changes.

m_database->open error cases seem more difficult.
Some error cases are recoverable (process suspension) but others are probably not recoverable.
It might be safer to not flush it for now.

We could cover all these cases by calling SQLiteFileSystem::deleteDatabaseFile(fullFilename) in scopeExit conditionally on a boolean storing whether recoverable or not.

As a side note, it seems that if process suspension happens exactly at the time we try to open the database to import the service worker registrations, we will load zero registrations.
Then, when being unsuspended, we will consider that there are zero registrations. At the first push change, we will import the registrations before pushing the changes.
I am not sure this code path is correct. Maybe we should make sure to restart import at unsuspension time.
Comment 5 Chris Dumez 2019-08-22 12:03:49 PDT
Created attachment 377032 [details]
Patch
Comment 6 Chris Dumez 2019-08-22 12:31:48 PDT
Created attachment 377034 [details]
Patch
Comment 7 WebKit Commit Bot 2019-08-22 16:28:52 PDT
Comment on attachment 377034 [details]
Patch

Clearing flags on attachment: 377034

Committed r249035: <https://trac.webkit.org/changeset/249035>
Comment 8 WebKit Commit Bot 2019-08-22 16:28:53 PDT
All reviewed patches have been landed.  Closing bug.