Closed Bug 1372592 Opened 5 years ago Closed 5 years ago

Should not display non-persistent-storage sites which stores 0 byte in Preferences Site Data section

Categories

(Firefox :: Preferences, defect, P2)

Product:
Firefox
Component:
Preferences
Version:
56 Branch
Platform:
x86_64
Windows
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: roxana.leitan, Assigned: Fischer)

References

(Blocks 1 open bug)

Details

(Whiteboard: [storage-v1])

Attachments

(2 files)

Site Data.png
56.30 KB, image/png
Details
Bug 1372592 - Should not display non-persistent-storage sites which stores 0 byte in Preferences Site Data section,
59 bytes, text/x-review-board-request
jaws
: review+
Details 
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170613030203

[Affected versions]:
Nightly 56.0a1

[Affected platforms]:
Windows 10 x64

[Steps to reproduce]:
1.Launch Nightly 56.0a1 with a new profile
2.Navigate to mozilla.org
3.Open Developer-> Web Console and type "navigator.storage.persist().then(persisted => console.log(persisted));"
4.Select "Don't Allow" 
5.Go to Options -> Privacy &Security -> Site Data -> Settings

[Expected result]:
mozilla.org website should be displayed in Settings-Site Data list  


[Actual result]:
mozilla.org website is not displayed in Settings-Site Data list
NI myself to follow up with Storage team.
Flags: needinfo?(htsai)
I'm a little bit not sure about this test case.
So "mozilla.org" is not persisted yet, and if the website doesn't store anything (indexeddb, cache, serviceworker).
Do we expect that it's in the Settings-Site Data list?
Fischer, can you confirm this?
Flags: needinfo?(fliu)
(In reply to Shawn Huang [:shawnjohnjr] from comment #2)
> I'm a little bit not sure about this test case.
> So "mozilla.org" is not persisted yet, and if the website doesn't store
> anything (indexeddb, cache, serviceworker).
> Do we expect that it's in the Settings-Site Data list?
> Fischer, can you confirm this?
In the current spec, only sites which have stored sth in Appcache or QuotaManger(IndexedDB etc) would appear on the list.
If only request the permission, it will not.
Flags: needinfo?(fliu)
Whiteboard: [storage-v1][triage]
(In reply to Fischer [:Fischer] from comment #3)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #2)
> > I'm a little bit not sure about this test case.
> > So "mozilla.org" is not persisted yet, and if the website doesn't store
> > anything (indexeddb, cache, serviceworker).
> > Do we expect that it's in the Settings-Site Data list?
> > Fischer, can you confirm this?
> In the current spec, only sites which have stored sth in Appcache or
> QuotaManger(IndexedDB etc) would appear on the list.
> If only request the permission, it will not.

Shall we explicitly see this in ux spec? Or we shall change this bug status to wontfix?
Flags: needinfo?(fliu)
(In reply to Shawn Huang [:shawnjohnjr] from comment #4)
> (In reply to Fischer [:Fischer] from comment #3)
> > (In reply to Shawn Huang [:shawnjohnjr] from comment #2)
> > > I'm a little bit not sure about this test case.
> > > So "mozilla.org" is not persisted yet, and if the website doesn't store
> > > anything (indexeddb, cache, serviceworker).
> > > Do we expect that it's in the Settings-Site Data list?
> > > Fischer, can you confirm this?
> > In the current spec, only sites which have stored sth in Appcache or
> > QuotaManger(IndexedDB etc) would appear on the list.
> > If only request the permission, it will not.
> 
> Shall we explicitly see this in ux spec? Or we shall change this bug status to wontfix?
Hi Mark,
Would you please update the specs, thanks.
Flags: needinfo?(fliu) → needinfo?(mliang)
(In reply to Fischer [:Fischer] from comment #5)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #4)
> > (In reply to Fischer [:Fischer] from comment #3)
> > > (In reply to Shawn Huang [:shawnjohnjr] from comment #2)
> > > > I'm a little bit not sure about this test case.
> > > > So "mozilla.org" is not persisted yet, and if the website doesn't store
> > > > anything (indexeddb, cache, serviceworker).
> > > > Do we expect that it's in the Settings-Site Data list?
> > > > Fischer, can you confirm this?
> > > In the current spec, only sites which have stored sth in Appcache or
> > > QuotaManger(IndexedDB etc) would appear on the list.
> > > If only request the permission, it will not.
> > 
> > Shall we explicitly see this in ux spec? Or we shall change this bug status to wontfix?
> Hi Mark,
> Would you please update the specs, thanks.

I added the following description in the spec:

"Websites that are granted permission to store data persistedly but haven’t stored any data yet will not display on this list since there are nothing to remove."

Thanks,
Mark
Flags: needinfo?(mliang)
Attached image Site Data.png — 
This bug is filed for websites with blocked persistent storage and the specs update is related to websites allowed to store persistent data which in the latest Nighly are displayed in the Settings list even haven’t stored any data yet. (please see the attached picture).
I'd expect that if there's no data stored, the website won't be listed in the settings, doesn't matter if the permission is granted or blocked. Not sure how the current implementation displays the 0 bytes, will need to ask Fischer.
Flags: needinfo?(fliu)
(In reply to Mark Liang(:mark_liang) from comment #8)
> I'd expect that if there's no data stored, the website won't be listed in
> the settings, doesn't matter if the permission is granted or blocked. Not
> sure how the current implementation displays the 0 bytes, will need to ask
> Fischer.
I wasn't able to see 0-bytes "bugzilla.mozilla.org" or "www.mozilla.org" here.
Then I tried to call `var DBOpenRequest = window.indexedDB.open("toDoList");` on one website. This would create an empty "toDoList" indexedDB. Then deleted that "toDoList" indexedDB through web console's storage panel. Then opened the Settings list and could see "bugzilla.mozilla.org" with "0 bytes". This is because in profile's storage > default folder, the https+++bugzilla.mozilla.org/idb folder exists, bugzilla.mozilla.org would be treated as still using indexedDB.
We may need a deeper look and discussion about in what kind of use cases might result this phenomenon and how to treat site storing 0 bytes.
Flags: needinfo?(fliu)
Component: DOM → Preferences
Flags: needinfo?(htsai)
Product: Core → Firefox
hi Mark,

we need a complete UX design to deal with this issue.
workaround may not be proper ways to deal with it.

thank you very much
Flags: needinfo?(mark.tianyi)
From UX perspective, the list in the setting panel is used to
1. Show which sites are storing data and allow users to remove them.
2. Show which sites are set to store data persistedly.

In this case I would suggest that we shouldn't show websites that are not persisted and are storing 0 bytes in the list since there are nothing to remove and no status to inform users. That says, if a website is granted to store persistedly but has not storing anything yet, we can still display them in the list with 0 bytes. (But this is probably a rare case?)
Flags: needinfo?(mark.tianyi)
Flags: needinfo?(fliu)
(In reply to Mark Liang(:mark_liang) from comment #11)
> From UX perspective, the list in the setting panel is used to
> 1. Show which sites are storing data and allow users to remove them.
> 2. Show which sites are set to store data persistedly.
> 
> In this case I would suggest that we shouldn't show websites that are not
> persisted and are storing 0 bytes in the list since there are nothing to
> remove and no status to inform users. That says, if a website is granted to
> store persistedly but has not storing anything yet, we can still display
> them in the list with 0 bytes. (But this is probably a rare case?)
Hi Mark,
OK, let's got for this approach.
Assignee: nobody → fliu
Status: NEW → ASSIGNED
Priority: -- → P2
Flags: needinfo?(fliu)
Whiteboard: [storage-v1][triage] → [storage-v1]
Per comment 7 and comment 12, change the bug title to reflect what this bug is addressing
Summary: Persistent storage blocked websites are not displayed in Preferences Site Data section → Should not display non-persistent-storage sites which stores 0 byte in Preferences Site Data section
(In reply to Fischer [:Fischer] from comment #15)
> Comment on attachment 8899349 [details]
> Bug 1372592 - Should not display non-persistent-storage sites which stores 0
> byte in Preferences Site Data section,
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/170572/diff/1-2/
TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5d002ad6b6b1d18c424ac634cf021c6bdd0648e
Comment on attachment 8899349 [details]
Bug 1372592 - Should not display non-persistent-storage sites which stores 0 byte in Preferences Site Data section,

https://reviewboard.mozilla.org/r/170572/#review175954

::: browser/components/preferences/SiteDataManager.jsm:77
(Diff revision 2)
> +          let redundantSites = [];
> +          for (let [host, site] of this._sites) {
> +            if (!site.persisted && this._getUsagePerSite(site) <= 0) {
> +              redundantSites.push(host);
> +            }
> +          }
> +          for (let host of redundantSites) {
> +            this._sites.delete(host);

Instead of removing sites that don't store anything, can we just not add them to the map in the first place?
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #17)
> Comment on attachment 8899349 [details]
> Bug 1372592 - Should not display non-persistent-storage sites which stores 0
> byte in Preferences Site Data section,
> 
> https://reviewboard.mozilla.org/r/170572/#review175954
> 
> ::: browser/components/preferences/SiteDataManager.jsm:77
> (Diff revision 2)
> > +          let redundantSites = [];
> > +          for (let [host, site] of this._sites) {
> > +            if (!site.persisted && this._getUsagePerSite(site) <= 0) {
> > +              redundantSites.push(host);
> > +            }
> > +          }
> > +          for (let host of redundantSites) {
> > +            this._sites.delete(host);
> 
> Instead of removing sites that don't store anything, can we just not add them to the map in the first place?
Hi Jared,
Because when counting usage it counts not only quota usage but also appcache usage (do `_getQuotaUsage` then `updateAppCache`), we couldn't rule out the 0-byte quota usage case in the 1st place. Have to check this out after `_updateAppCache`. How do you think, thanks.
Flags: needinfo?(jaws)
(In reply to Fischer [:Fischer] from comment #18)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #17)
> > Comment on attachment 8899349 [details]
> > Bug 1372592 - Should not display non-persistent-storage sites which stores 0
> > byte in Preferences Site Data section,
> > 
> > https://reviewboard.mozilla.org/r/170572/#review175954
> > 
> > ::: browser/components/preferences/SiteDataManager.jsm:77
> > (Diff revision 2)
> > > +          let redundantSites = [];
> > > +          for (let [host, site] of this._sites) {
> > > +            if (!site.persisted && this._getUsagePerSite(site) <= 0) {
> > > +              redundantSites.push(host);
> > > +            }
> > > +          }
> > > +          for (let host of redundantSites) {
> > > +            this._sites.delete(host);
> > 
> > Instead of removing sites that don't store anything, can we just not add them to the map in the first place?
> Hi Jared,
> Because when counting usage it counts not only quota usage but also appcache
> usage (do `_getQuotaUsage` then `updateAppCache`), we couldn't rule out the
> 0-byte quota usage case in the 1st place. Have to check this out after
> `_updateAppCache`. How do you think, thanks.
Sorry, after the 2nd thought, that is possible. We could do the checks along the way during collecting sites. Please see the updated patch.
Comment on attachment 8899349 [details]
Bug 1372592 - Should not display non-persistent-storage sites which stores 0 byte in Preferences Site Data section,

https://reviewboard.mozilla.org/r/170572/#review177034

Great! Much nicer :)
Attachment #8899349 - Flags: review?(jaws) → review+
Flags: needinfo?(jaws)
This bug would have code conflict with the bug 1377104 so will check-in after the bug 1377104.
Depends on: 1377104
(In reply to Fischer [:Fischer] from comment #24)
> Comment on attachment 8899349 [details]
> Bug 1372592 - Should not display non-persistent-storage sites which stores 0
> byte in Preferences Site Data section,
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/170572/diff/4-5/
Saw should-split-up-tests issue[1]. Split up the tests.
[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=dbe9b18e60e9&selectedJob=126327679
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6257f5ada181
Should not display non-persistent-storage sites which stores 0 byte in Preferences Site Data section, r=jaws
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6257f5ada181
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
verified by nightly build today. No 0 bytes site showed with persist permission.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.