Bug 190795 – Regression (r232410): StorageTracker.db file gets unlinked while in use

• Component: New Bugs
• Status: RESOLVED FIXED

[Sihui Liu]

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.

[Sihui Liu]

<rdar://problem/45409880>

[Sihui Liu]

Created attachment 352893 [details]
Patch

[Chris Dumez]

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

[Sihui Liu]

(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.

[Sihui Liu]

Created attachment 352896 [details]
Patch

[Chris Dumez]

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.

[Geoffrey Garen]

When we make this file name change, won't LegacyWebKit apps "forget" all the LocalStorage entries that were recorded in StorageTracker.db?

[Sihui Liu]

(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().

[Sihui Liu]

Comment on attachment 352896 [details]
Patch

Seems like disk access error on wincairo.

[Chris Dumez]

(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.

[Chris Dumez]

(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.

[WebKit Commit Bot]

Comment on attachment 352896 [details]
Patch

Clearing flags on attachment: 352896

Committed r237330: <https://trac.webkit.org/changeset/237330>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Sihui Liu]

(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.