Bug 216962 - IndexedDB Index Corruption after upgrade from iOS 13 to iOS 14
Summary: IndexedDB Index Corruption after upgrade from iOS 13 to iOS 14
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: Safari 14
Hardware: iPhone / iPad Other
: P2 Critical
Assignee: Sihui Liu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-09-24 22:57 PDT by Darryl Pogue
Modified: 2020-09-29 11:00 PDT (History)
10 users (show)

See Also:


Attachments
Testcase file (1.85 KB, text/html)
2020-09-25 13:09 PDT, Darryl Pogue
no flags Details
Patch (41.14 KB, patch)
2020-09-28 11:04 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (41.42 KB, patch)
2020-09-28 14:28 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (41.43 KB, patch)
2020-09-28 16:28 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darryl Pogue 2020-09-24 22:57:02 PDT
r255318 introduced a change to how IDB Indexes are stored in the SQLite backing store. It moves from per-ObjectStore index IDs to per-Database index IDs, and attempts to perform a migration. This migration is run on iOS 14 when launching an app whose WKWebView IndexedDB databases were created on iOS 13.

There's a flaw in the migration process that can result in multiple indexes being updated to have the same ID, and corrupting the associated IndexRecords data. Once this happens, it appears to be impossible to recover the correct indexes.

A detailed walkthrough of how this happens to the IDB indices in our app can be found here: https://gist.github.com/dpogue/53d529310355697a0bfdf17644a17840

It only happens in some pretty specific situations, but it's a pretty serious problem.
Comment 1 Sihui Liu 2020-09-25 12:21:27 PDT
Thanks for reporting! This is pretty bad since the corrupted records cannot be recovered.
Comment 2 Radar WebKit Bug Importer 2020-09-25 12:21:35 PDT
<rdar://problem/69587004>
Comment 3 Darryl Pogue 2020-09-25 13:09:38 PDT
Created attachment 409726 [details]
Testcase file

Here's a testcase file which can be used to demonstrate the problem.

I ran this in an iOS simulator on iOS 13.5 and opened it in Safari, then used `xcrun simctl upgrade` to upgrade the simulator to iOS 14.0 and opened it again. Looking at it with the Safari Inspector shows that one of the indexes is now missing.
Comment 4 Sihui Liu 2020-09-28 11:04:31 PDT
Created attachment 409900 [details]
Patch
Comment 5 Sihui Liu 2020-09-28 14:28:12 PDT
Created attachment 409924 [details]
Patch
Comment 6 Darin Adler 2020-09-28 14:54:41 PDT
Comment on attachment 409924 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:242
> +static String indexInfoTableSchema(const String& tableName)

This argument should be ASCIILiteral or const char*, not const String&. It’s wastefully inefficient to turn an ASCII literal into a String and then destroy that string.
Comment 7 Darryl Pogue 2020-09-28 15:45:43 PDT
I tested this patch in both Safari and in my WKWebView-based app on the iOS simulator, and the corruption was fixed in both cases.
Thanks Sihui for jumping on this! :)
Comment 8 Sihui Liu 2020-09-28 16:28:21 PDT
Created attachment 409928 [details]
Patch
Comment 9 Sihui Liu 2020-09-28 16:32:06 PDT
(In reply to Darin Adler from comment #6)
> Comment on attachment 409924 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=409924&action=review
> 
> > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:242
> > +static String indexInfoTableSchema(const String& tableName)
> 
> This argument should be ASCIILiteral or const char*, not const String&. It’s
> wastefully inefficient to turn an ASCII literal into a String and then
> destroy that string.

Right, changed to use ASCIILiteral.
Comment 10 EWS 2020-09-29 11:00:43 PDT
Committed r267753: <https://trac.webkit.org/changeset/267753>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 409928 [details].