1377104 - Should clear all stored site data dynamically

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P2)

Description

[Ioana Crisan]

Attachment: index.html

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170629103102

Steps to reproduce:
1. Launch Nightly 56.0a1
2. Navigate to Preferences -> Privacy & Security 
3. Open an url in a new tab(you can open the index.htm attachment and you can use it in the next steps)
4. Persist the site from step 3 and store some data
5. From the already opened Preferences tab observe the "Site Data" section
6. Without refreshing the page click the "Clear All Data" button displayed next to the "Site Data" section and then the "Clear Now" button
7. Refresh the page
8. Continue to store data via indexeddb
9. Refresh the page


Expected results:
After step 5: The "Site Data" section dynamically updates and shows the amount of bytes stored.
After step 6 and 7: The amount of bytes stored is equal to "0".
After step 9: The amount of bytes stored starts counting from "0".

Actual results:
After step 5: The "Site Data" section does not update dynamically. Without page refresh the amount of bytes stored is equal to "0".
After step 6 and 7: Data has not been cleaned up(displays for e.g: 2GB)
After step 9: Data displayed is cumulated(displays for e.g: 3GB)

Notes: 
Issue does not reproduce if the "Preferences" tab is refreshed before deletion.
Issue reproduces also if the site is not persisted.
The "Cached Web Content" section does not update dynamically also.

Comment 1

[Fischer [:Fischer]]

Since this is about the Clear All function in the about:preferences Site Data, update the bug title to reflect this more explicitly.

Comment 2

[Fischer [:Fischer]]

Attachment: Bug 1377104 - Should clear all stored site data dynamically,

Review commit: https://reviewboard.mozilla.org/r/171542/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/171542/

Comment 3

[Fischer [:Fischer]]

Comment on attachment 8900150 [details]
Bug 1377104 - Should clear all stored site data dynamically,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171542/diff/1-2/

Comment 4

[Fischer [:Fischer]]

Comment on attachment 8900150 [details]
Bug 1377104 - Should clear all stored site data dynamically,

https://reviewboard.mozilla.org/r/171542/#review176780

::: browser/components/preferences/SiteDataManager.jsm:254
(Diff revision 2)
>      let promises = [];
>      for (let site of this._sites.values()) {
>        this._removePermission(site);
>        promises.push(this._removeQuotaUsage(site));
>      }
> -    Services.cache2.clear();
> +    return Promise.all(promises).then(() => this.updateSites());

In fact, it is fine not returning `Promise.all(…)` just like the old way because all the callers don’t depend on that [1]. Why returning that is that we changed the `removeAll` to a async function. Since it is an async function, its *truly* resolved timing would be `Promise.all(…)` getting resolved. So we do this to reflect this change.

[1] http://searchfox.org/mozilla-central/search?q=SiteDataManager.removeAll&case=false&regexp=false&path=

Comment 5

[Fischer [:Fischer]]

(In reply to Fischer [:Fischer] from comment #3)
> Comment on attachment 8900150 [details]
> Bug 1377104 - Should clear all stored site data dynamically,
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/171542/diff/1-2/
TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c759af458c95ee1d6fa2acceea96abe9642048b

Comment 6

[:Gijs (he/him)]

Comment on attachment 8900150 [details]
Bug 1377104 - Should clear all stored site data dynamically,

https://reviewboard.mozilla.org/r/171542/#review176786

::: browser/components/preferences/SiteDataManager.jsm:246
(Diff revision 2)
> +    // Why we don't do "Clear All" on the quota manager like the cookie, appcache, http cache above
> +    // is because that would clear more than needed, see the bug 1312361 comment 9.

English nit: In this sentence, leave out "Why" and "is".

Also, maybe just give a brief description of what else is being deleted (e.g. instead of "more than needed", "browser data as well").

Finally, please include a link to the comment in question (instead of just the description of comment 9) so it's easy to get there again. :-)

::: browser/components/preferences/in-content/tests/head.js:24
(Diff revision 2)
>  
> -  _originalGetQuotaUsage: null,
> +  _originalQMS: null,
>    _originalRemoveQuotaUsage: null,
>  
> -  _getQuotaUsage() {
> -    let results = [];
> +  getUsage(onUsageResult) {
> +    let result = [];

Nit: Seems like leaving this variable name `results` would be more accurate, as there's more than 1 item. 

Also, a slightly more efficient way of writing this would be:

```js
let results = this.fakeSites.map(site => ({
  origin: site.principal.origin,
  usage: site.usage,
  persisted: site.persisted,
}));
```

(here and in the other head.js the same, of course)

Comment 7

[Fischer [:Fischer]]

Comment on attachment 8900150 [details]
Bug 1377104 - Should clear all stored site data dynamically,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/171542/diff/2-3/

Comment 8

[Fischer [:Fischer]]

Comment on attachment 8900150 [details]
Bug 1377104 - Should clear all stored site data dynamically,

https://reviewboard.mozilla.org/r/171542/#review177298

::: browser/components/preferences/SiteDataManager.jsm:49
(Diff revision 3)
>      // Clear old data and requests first
>      this._sites.clear();
>      this._cancelGetQuotaUsage();
> -
> -    this._getQuotaUsage()
> -        .then(results => {
> +    this._getQuotaUsagePromise = new Promise(resolve => {
> +      let onUsageResult = request => {
> +        let items = request.result;

Changed the variable name so to reduce some confusion

Comment 9

[Fischer [:Fischer]]

(In reply to :Gijs from comment #6)
> Comment on attachment 8900150 [details]
> Bug 1377104 - Should clear all stored site data dynamically,
> 
> https://reviewboard.mozilla.org/r/171542/#review176786
> 
> ::: browser/components/preferences/SiteDataManager.jsm:246 (Diff revision 2)
> > +    // Why we don't do "Clear All" on the quota manager like the cookie, appcache, http cache above
> > +    // is because that would clear more than needed, see the bug 1312361 comment 9.
> 
> English nit: In this sentence, leave out "Why" and "is".
> 
> Also, maybe just give a brief description of what else is being deleted
> (e.g. instead of "more than needed", "browser data as well").
> 
> Finally, please include a link to the comment in question (instead of just
> the description of comment 9) so it's easy to get there again. :-)
> 
Thanks, all addressed.

> ::: browser/components/preferences/in-content/tests/head.js:24
> (Diff revision 2)
> >  
> > -  _originalGetQuotaUsage: null,
> > +  _originalQMS: null,
> >    _originalRemoveQuotaUsage: null,
> >  
> > -  _getQuotaUsage() {
> > -    let results = [];
> > +  getUsage(onUsageResult) {
> > +    let result = [];
> 
> Nit: Seems like leaving this variable name `results` would be more accurate, as there's more than 1 item. 
> 
Quota Manager Service passed `request.result` to `onUsageResult` callback[1], so have to adhere to that.
Just changed `let results = request.result;` to `let items = request.result;`. Maybe could reduce some confusion.

[1] http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/dom/quota/QuotaRequests.cpp#134

> Also, a slightly more efficient way of writing this would be:
> 
> ```js
> let results = this.fakeSites.map(site => ({
>   origin: site.principal.origin,
>   usage: site.usage,
>   persisted: site.persisted,
> }));
> ```
> 
> (here and in the other head.js the same, of course)
Thanks updated.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Autoland can't push this until all pending issues in MozReview are marked as resolved.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8a9034f32b9c
Should clear all stored site data dynamically, r=Gijs

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8a9034f32b9c

Comment 13

[Paul Silaghi, QA [:pauly]]

(In reply to Ioana Crisan from comment #0)
> Expected results:
> After step 5: The "Site Data" section dynamically updates and shows the
> amount of bytes stored.
This part is still reproducible - the "Site Data" section doesn't refresh on switching tabs, if the Preferences window is already opened.
Did you want to fix this too?
Tested on 57.0a1 (2017-08-27), Win 10.

Comment 14

[Fischer [:Fischer]]

(In reply to Paul Silaghi, QA [:pauly] from comment #13)
> (In reply to Ioana Crisan from comment #0)
> > Expected results:
> > After step 5: The "Site Data" section dynamically updates and shows the
> > amount of bytes stored.
> This part is still reproducible - the "Site Data" section doesn't refresh on
> switching tabs, if the Preferences window is already opened.
> Did you want to fix this too?
> Tested on 57.0a1 (2017-08-27), Win 10.
We only do dynamically refresh on "Clear All Dara" so the verification steps should be as the below:
1. Launch Nightly
2. Navigate to Preferences -> Privacy & Security 
3. Open an url in a new tab(you can open the index.htm attachment and you can use it in the next steps)
4. Persist the site from step 3 and store some data
5. Back to the Preferences "Site Data" section
6. Without refreshing the page click the "Clear All Data" button displayed next to the "Site Data" section and then the "Clear Now" button
7. Observe all the site data cleared
8. Refresh the Preferences page (Don't store any site data again)
9. Observe all the site data still cleared

p.s The dynamical refresh is expensive so only do dynamically refresh on "Clear All Dara".

Thanks

Comment 15

[Paul Silaghi, QA [:pauly]]

Thanks, Fischer.
Verified fixed 57.0a1 (2017-08-27) Win 10, Win 7, Ubuntu 14.04, OS X 10.13, based on comment 14.