1407936 - Site data is not completely deleted from profile folder after removing data from Fx site data settings

Status: RESOLVED FIXED

(Core :: Storage: Quota Manager, defect, P1)

Description

[Bogdan Maris [:bogdan_maris], Release Desktop QA]

[Affected versions]:
- latest Nightly 58.0a1
- Firefox 57 beta 7

[Affected platforms]:
- Ubuntu 16.04 32bit
- macOS 10.12.6
- Windows 7 32bit

[Steps to reproduce]:
1. Start Firefox
2. Visit twitter.com and login using an existing account
3. Go to about:preferences#privacy
4. Click settings from Site Data section
5. Click Remove All button and Save changes
6. Refresh about:preferences page and revisit settings from Site Data section

[Expected result]:
- No site data is present in Settings

[Actual result]:
- There is 64kb of data left in Settings. Following step 5 from above the second time will remove all data for good.
- Other websites could be affected by this as well not only twitter.

[Regression range]:
- Don't think this is a regression since it reproduces back in Nightly 55 (2017-06-12) as well.

Comment 1

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

The file folder contains cache sqlite database only and it's an empty database;
I don't see QuotaManager error when deleting site data from console logs. I need to dig into why cache sqlite doesn't clear at all.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Is twitter still open when you clear the storage?  It can touch the disk again immediately.

Comment 3

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Ben Kelly [:bkelly] from comment #2)
> Is twitter still open when you clear the storage?  It can touch the disk
> again immediately.

We've tried to close the tab first then clear the storage. Cache still exists.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Twitter has a service worker with push notifications.  Its possible its running when you close the tab and clear the storage.  I'm not sure how we handle an already running worker in that case.  Perhaps verify the worker is stopped before clearing the storage?

Comment 5

[Andrew Overholt [:overholt]]

baku worked on this situation recently so maybe he can answer comment 4.

P1 since we just went through a fire drill trying to fix this :)

Comment 6

[Andrea Marchesini [:baku]]

Documentation of removeAndPropagate() says that:

   * Clears ServiceWorker registrations from memory and disk for the specified                            
   * host.                                                                                                
   * - All ServiceWorker instances change their state to redundant.                                       
   * - Existing ServiceWorker instances handling fetches will keep running.                               
   * - All documents will immediately stop being controlled.                                              
   * - Unregister jobs will be queued for all registrations.                                              
   *   This eventually results in the registration being deleted from disk too.  

Maybe we should add a callback so that we cleanup data only when the operation is completed. Makes sense?

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrea Marchesini [:baku] from comment #6)
> Maybe we should add a callback so that we cleanup data only when the
> operation is completed. Makes sense?

We could do this, but a site could in theory arrange to keep itself a live for a while.

I guess I'd prefer to track that this is a user based override removal and force kill the SW.  We should also make sure push registrations are removed correctly.

Comment 8

[Andrea Marchesini [:baku]]

Attachment: unregister_sw.patch

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8920030 [details] [diff] [review]
unregister_sw.patch

Can you please provide some additional context about what the problem was and how this fixes it?  Thanks.

Comment 10

[Andrea Marchesini [:baku]]

> Can you please provide some additional context about what the problem was
> and how this fixes it?  Thanks.

SWM.removeAndPropagate() doesn't use a callback and we end up removing QuotaManagar data while SW can be up and running.
At the same time, offline function returns before completing the operation instead waiting for SW to be unregistered.

This patch fixes the issue using propagateUnregister() method and promises.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Thanks.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3624656ebe46
Cleaning up QuotaManagar data only when ServiceWorkers are correctly unregistered, r=bkelly

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/02af76d6b30b
Fixing eslint failure, r=me

Comment 14

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3624656ebe46
https://hg.mozilla.org/mozilla-central/rev/02af76d6b30b

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Privacy leak? Is this something we should consider uplifting to Beta?

Comment 16

[Ciprian Georgiu [:ciprian_georgiu], Release Desktop QA]

Hi baku,

It seems that this patch did not fix this bug, there's still cache left in the pofile folder (for https+++www.youtube.com, https+++twitter.com) alongside with .metadata, .metadata-v2 files. I've tried to close the tab first and then clear the storage, but it didn't make any difference. Tested on Win 10 x64 and Mac OS X 10.11 with latest Nightly 58.0a1 (20171020100426).

Can you please take a look at this?

Comment 17

[Christiane Ruetten [INACTIVE]]

Let's try not to mix up "data accessible to websites" and "data stored in the profile folder".

Comment 18

[Andrea Marchesini [:baku]]

> It seems that this patch did not fix this bug, there's still cache left in
> the pofile folder (for https+++www.youtube.com, https+++twitter.com)
> Can you please take a look at this?

Do we still have data into these folders? Is twitter having some SW still registered?

NI janv to know if it's correct to have these files in the storage directory.

Comment 19

[Andrea Marchesini [:baku]]

Comment on attachment 8920030 [details] [diff] [review]
unregister_sw.patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 1047098
[User impact if declined]: FF can freezes on shutdown.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes, at least, manually
[Needs manual test from QE? If yes, steps to reproduce]: See the bug description for the STR
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low
[Why is the change risky/not risky?]: the cleanup of data happens only when ServiceWorkers are correctly unregistered. It's just a different ordering of promises.
[String changes made/needed]: none

Comment 20

[Jan Varga [:janv]]

(In reply to Ciprian Georgiu, QA [:ciprian_georgiu] from comment #16)
> Hi baku,
> 
> It seems that this patch did not fix this bug, there's still cache left in
> the pofile folder (for https+++www.youtube.com, https+++twitter.com)
> alongside with .metadata, .metadata-v2 files. I've tried to close the tab
> first and then clear the storage, but it didn't make any difference. Tested
> on Win 10 x64 and Mac OS X 10.11 with latest Nightly 58.0a1 (20171020100426).
> 
> Can you please take a look at this?

Can you check "create" time of those folders and .metadata/.metadata-v2 files ?
Maybe origin directories are correctly cleared, but there's a request to store data for them right after the clear operation finishes.

Comment 21

[Ciprian Georgiu [:ciprian_georgiu], Release Desktop QA]

(In reply to Andrea Marchesini [:baku] from comment #18)
> Do we still have data into these folders? Is twitter having some SW still
> registered?

I can see the exact behavior described below after following the STR from comment 0. Here is a link with the https+++twitter.com folder: https://goo.gl/3aWCbS.

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #0)
> [Actual result]:
> - There is 64kb of data left in Settings. Following step 5 from above the
> second time will remove all data for good.
> - Other websites could be affected by this as well not only twitter.

Comment 22

[Andrea Marchesini [:baku]]

The folder seems completely empty: no SW registered, no cache stored. This seems correct to me.

Comment 23

[Jan Varga [:janv]]

If there are no subfolders and just .metadata/.metadata-v2, then yes.

Comment 24

[Jan Varga [:janv]]

However, https://goo.gl/3aWCbS contains "cache" subfolder.

Comment 25

[Ciprian Georgiu [:ciprian_georgiu], Release Desktop QA]

(In reply to Jan Varga [:janv] from comment #24)
> However, https://goo.gl/3aWCbS contains "cache" subfolder.

Besides that, in "Setttings - Site Data" the site in question (e.g. youtube.com, twitter.com) is still displayed after clearing all data and refresh the about:preferences#privacy page - as  you can see in this screencast: https://imgur.com/a/0vYLC.

Comment 26

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8920030 [details] [diff] [review]
unregister_sw.patch

Recent regression, fix was verified on nightly, beta57+

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/58c1a09d1a65

Comment 28

[Bogdan Maris [:bogdan_maris], Release Desktop QA]

Yeah, I am still seeing the original issue mentioned in comment 0 in latest Nightly 58.0a1 and today in 57 beta 11 after the uplift.

As Ciprian mentioned in comment 21 and comment 25 this is still reproducible so I don't really know what was fixed here. Andrea can you clear the air here a bit? There is a cache folder left behind which contains a file caches.sqlite (~66kb) and another folder called morgue that's empty. 

Note again that this is the same exact behavior I initially reported in comment 0, and due to that I'm confused of what was fixed here.

Comment 29

[Andrea Marchesini [:baku]]

> As Ciprian mentioned in comment 21 and comment 25 this is still reproducible
> so I don't really know what was fixed here. Andrea can you clear the air
> here a bit? There is a cache folder left behind which contains a file
> caches.sqlite (~66kb) and another folder called morgue that's empty. 

janv, does it seem normal to you? the caches.sqlite db is empty. There is just the scheme.
Should this file be deleted together with the rest?

Comment 30

[Jan Varga [:janv]]

(In reply to Andrea Marchesini [:baku] from comment #29)
> > As Ciprian mentioned in comment 21 and comment 25 this is still reproducible
> > so I don't really know what was fixed here. Andrea can you clear the air
> > here a bit? There is a cache folder left behind which contains a file
> > caches.sqlite (~66kb) and another folder called morgue that's empty. 
> 
> janv, does it seem normal to you? the caches.sqlite db is empty. There is
> just the scheme.
> Should this file be deleted together with the rest?

Quota Manager doesn't filter any files when clearing an origin directory. I guess DOM cache creates a new database right after the origin directory is cleared. Can someone check create time of caches.sqlite ?

Comment 31

[Bogdan Maris [:bogdan_maris], Release Desktop QA]

caches.sqlite is first created when I log on Twitter but it has like 4kb, in 2-3 seconds it's pumped to 66kb. Here is a screencast demonstrating this: https://drive.google.com/open?id=0BzbDkQ2Zlg86TWR4WGh1eHpWNG8 (was too large to upload it directly to bugzilla).

Comment 32

[Ben Kelly [:bkelly, not reviewing]]

It's probably the ServiceWorkerScriptCache creating a Cache storage object which triggers schema creation. If you waited 1+ minutes after closing the tab this probably would not happen.

Just curious, why is an empty sqlite file a critical problem?  Content can't tell if it's there or not.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

It would seem to me that as long as there's nothing privacy-sensitive in there (comment 29 says just schema data?), we wouldn't care much.

Comment 34

[Andrew Sutherland [:asuth] (he/him)]

The empty SQLite file lives in a directory derived from the origin.  The canonical private browsing example is engagement ring shopping.  While it's harder for a non-technically-sophisticated user to locate, using a suffieicntly comprehensive filesystem search for "bluenile" would very likely discover the PROFILE/storage/default/https+++www.bluenile.com/ directory which contains the cache/ directory.  I don't think we make strong guarantees about the profile being resistant to forensic analysis, but we can definitely do better than leaving origins around in cleartext for our history-clearing features.

Comment 35

[Jan Varga [:janv]]

I suspect this can also cause bugs like bug 1533681.

Comment 36

[Andrew Overholt [:overholt]]

Should we do more here to fix things like bug 1533681, Jan? Maybe file a sepearate bug?

Comment 37

[Jan Varga [:janv]]

(In reply to Andrew Overholt [:overholt] from comment #36)

Should we do more here to fix things like bug 1533681, Jan? Maybe file a sepearate bug?

Yes, the official status quo in this bug was that clearing works ok even though there is a sqlite database left with no data.
However some users still complain about it and as I said it can cause problems with some tests.
I filed a new bug for that, bug 1541464.