Closed
Bug 1372592
Opened 6 years ago
Closed 6 years ago
Should not display non-persistent-storage sites which stores 0 byte in Preferences Site Data section
Categories
(Firefox :: Settings UI, defect, P2)
Tracking
()
VERIFIED
FIXED
Firefox 57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: roxana.leitan, Assigned: Fischer)
References
(Blocks 1 open bug)
Details
(Whiteboard: [storage-v1])
Attachments
(2 files)
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
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)
Assignee | ||
Comment 3•6 years ago
|
||
(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)
Updated•6 years ago
|
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)
Assignee | ||
Comment 5•6 years ago
|
||
(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)
Comment 6•6 years ago
|
||
(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)
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).
Comment 8•6 years ago
|
||
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)
Assignee | ||
Comment 9•6 years ago
|
||
(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)
Updated•6 years ago
|
Component: DOM → Preferences
Flags: needinfo?(htsai)
Product: Core → Firefox
Comment 10•6 years ago
|
||
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)
Comment 11•6 years ago
|
||
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)
Updated•6 years ago
|
Flags: needinfo?(fliu)
Assignee | ||
Comment 12•6 years ago
|
||
(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 | ||
Updated•6 years ago
|
Assignee: nobody → fliu
Status: NEW → ASSIGNED
Priority: -- → P2
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(fliu)
Updated•6 years ago
|
Whiteboard: [storage-v1][triage] → [storage-v1]
Assignee | ||
Comment 13•6 years ago
|
||
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
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 16•6 years ago
|
||
(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 17•6 years ago
|
||
mozreview-review |
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?
Assignee | ||
Comment 18•6 years ago
|
||
(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)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 20•6 years ago
|
||
(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 21•6 years ago
|
||
mozreview-review |
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+
Updated•6 years ago
|
Flags: needinfo?(jaws)
Assignee | ||
Comment 22•6 years ago
|
||
This bug would have code conflict with the bug 1377104 so will check-in after the bug 1377104.
Depends on: 1377104
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 25•6 years ago
|
||
(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
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 26•6 years ago
|
||
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
![]() |
||
Comment 27•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6257f5ada181
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment 28•6 years ago
|
||
verified by nightly build today. No 0 bytes site showed with persist permission.
Updated•6 years ago
|
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•