Bug 190795 - Regression (r232410): StorageTracker.db file gets unlinked while in use
Summary: Regression (r232410): StorageTracker.db file gets unlinked while in use
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sihui Liu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-10-22 08:59 PDT by Sihui Liu
Modified: 2018-10-23 09:58 PDT (History)
4 users (show)

See Also:


Attachments
Patch (1.68 KB, patch)
2018-10-22 09:07 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (1.68 KB, patch)
2018-10-22 10:29 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 Sihui Liu 2018-10-22 08:59:17 PDT
In r232410, WebKit stopped to use StorageTracker.db because of performance, and it would delete this file for safety.
r232410 didn't change the behavior of WebKitLegacy, as to keep the old things working. 
It turned out WebKitLegacy could be using the same file as WebKit, so app built on WebKit could delete the db file while apps with WebKitLegacy is using it.
Comment 1 Sihui Liu 2018-10-22 09:00:19 PDT
<rdar://problem/45409880>
Comment 2 Sihui Liu 2018-10-22 09:07:37 PDT
Created attachment 352893 [details]
Patch
Comment 3 Chris Dumez 2018-10-22 09:33:18 PDT
Comment on attachment 352893 [details]
Patch

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

Is this an issue for apps using both WK1 and WK2?

> Source/WebKitLegacy/ChangeLog:8
> +        WK2 stopped to use StorageTracker.db file in r232410 and would delete 

stopped to use -> stopped using
Comment 4 Sihui Liu 2018-10-22 10:26:06 PDT
(In reply to Chris Dumez from comment #3)
> Comment on attachment 352893 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=352893&action=review
> 
> Is this an issue for apps using both WK1 and WK2?
> 
Yes, if the app uses WK1's Storage API.

> > Source/WebKitLegacy/ChangeLog:8
> > +        WK2 stopped to use StorageTracker.db file in r232410 and would delete 
> 
> stopped to use -> stopped using
Okay.
Comment 5 Sihui Liu 2018-10-22 10:29:05 PDT
Created attachment 352896 [details]
Patch
Comment 6 Chris Dumez 2018-10-22 10:50:36 PDT
Comment on attachment 352896 [details]
Patch

One concern: Does this mean that WK1-only apps will now have the historical StorageTracker.db and the new LegacyStorageTracker.db when updating WebKit? If so, we'd probably need to remove the historical StorageTracker.db file.
Comment 7 Geoffrey Garen 2018-10-22 12:00:53 PDT
When we make this file name change, won't LegacyWebKit apps "forget" all the LocalStorage entries that were recorded in StorageTracker.db?
Comment 8 Sihui Liu 2018-10-22 13:36:55 PDT
(In reply to Geoffrey Garen from comment #7)
> When we make this file name change, won't LegacyWebKit apps "forget" all the
> LocalStorage entries that were recorded in StorageTracker.db?

It would. But during the initialization of StorageTracker, the origins get re-imported by syncFileSystemAndTrackerDatabase().
Comment 9 Sihui Liu 2018-10-22 13:40:53 PDT
Comment on attachment 352896 [details]
Patch

Seems like disk access error on wincairo.
Comment 10 Chris Dumez 2018-10-22 13:41:37 PDT
(In reply to Chris Dumez from comment #6)
> Comment on attachment 352896 [details]
> Patch
> 
> One concern: Does this mean that WK1-only apps will now have the historical
> StorageTracker.db and the new LegacyStorageTracker.db when updating WebKit?
> If so, we'd probably need to remove the historical StorageTracker.db file.

You did not address my concern.
Comment 11 Chris Dumez 2018-10-22 13:42:14 PDT
(In reply to Chris Dumez from comment #10)
> (In reply to Chris Dumez from comment #6)
> > Comment on attachment 352896 [details]
> > Patch
> > 
> > One concern: Does this mean that WK1-only apps will now have the historical
> > StorageTracker.db and the new LegacyStorageTracker.db when updating WebKit?
> > If so, we'd probably need to remove the historical StorageTracker.db file.
> 
> You did not address my concern.

i.e. we do not want legacy files to use up space on customers' devices.
Comment 12 WebKit Commit Bot 2018-10-22 14:05:29 PDT
Comment on attachment 352896 [details]
Patch

Clearing flags on attachment: 352896

Committed r237330: <https://trac.webkit.org/changeset/237330>
Comment 13 WebKit Commit Bot 2018-10-22 14:05:31 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Sihui Liu 2018-10-23 09:58:55 PDT
(In reply to Chris Dumez from comment #11)
> (In reply to Chris Dumez from comment #10)
> > (In reply to Chris Dumez from comment #6)
> > > Comment on attachment 352896 [details]
> > > Patch
> > > 
> > > One concern: Does this mean that WK1-only apps will now have the historical
> > > StorageTracker.db and the new LegacyStorageTracker.db when updating WebKit?
> > > If so, we'd probably need to remove the historical StorageTracker.db file.
> > 
> > You did not address my concern.
> 
> i.e. we do not want legacy files to use up space on customers' devices.

Sorry I missed your comment...

Yes, I assumed that the users always have some WK2 app to do the cleanup, but there is a chance the user only use WK1 apps so the historical file is left. It won't pose a threat like using up the space, but I should do the cleanup.