Closed Bug 1421737 Opened 6 years ago Closed 6 years ago

Include cookies in the site data manager

Categories

(Firefox :: Settings UI, enhancement, P1)

58 Branch
enhancement

Tracking

()

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: johannh, Assigned: johannh)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [storage-v2] )

Attachments

(4 files)

Users should be able to see sites with only cookies in the site data manager and view the number of cookies that is set for the individual site.
Depends on: 569627
Blocks: 1432743
Assignee: nobody → jhofmann
No longer blocks: storage-v2
Status: NEW → ASSIGNED
No longer depends on: 569627
Priority: P2 → P1
Summary: Include cookies in the in the site data manager → Include cookies in the site data manager
Comment on attachment 8948551 [details]
Bug 1421737 - Part 1 - Include cookies in SiteDataManager.jsm.

https://reviewboard.mozilla.org/r/217968/#review223780


Static analysis found 1 defect in this patch.
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: browser/components/preferences/SiteDataManager.jsm:224
(Diff revision 1)
> -            cookie.host, cookie.name, cookie.path, false, cookie.originAttributes);
> +        cookie.host, cookie.name, cookie.path, false, cookie.originAttributes);
> -        }
> -      }
>  
> -      Services.obs.notifyObservers(null, "browser:purge-domain-data", principal.URI.host);
> +      // TODO: what does this do?
> +      //Services.obs.notifyObservers(null, "browser:purge-domain-data", principal.URI.host);

Error: Expected space or tab after '//' in comment. [eslint: spaced-comment]
Blocks: 1406901
Comment on attachment 8948551 [details]
Bug 1421737 - Part 1 - Include cookies in SiteDataManager.jsm.

https://reviewboard.mozilla.org/r/217968/#review224818

When you do patches like this, please split them up more. Also, please add more detailed comments about what you're doing in the extended (ie non-first-line) commit message.

Some of the refactoring was no doubt necessary, but mixing it in with all the other stuff makes this patch exceptionally hard to review. You seem to have changed the type of `_sites` (but not everywhere), how it's used (but not everywhere), changed the name and exactly what `_removeCookie` does (but not clarified why), and changed both the UI and backend code in one patch (so now I'm scrolling through this giant thing and grepping for `_sites` to try to work out what's up with the Map/array changes is useless because it gets me frontend/backend references mixed together).

I've only reviewed the backend part of this because I already found a number of things that are confusing. If you can split the UI changes out that would still be helpful in terms of keeping the patch size manageable and logical.

::: browser/components/preferences/SiteDataManager.jsm:66
(Diff revision 2)
> +    try {
> +      result = Services.eTLD.getBaseDomainFromHost(host);
> +    } catch (e) {
> +      if (e.result == Cr.NS_ERROR_HOST_IS_IP_ADDRESS ||
> +          e.result == Cr.NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS) {
> +        // For this 2 expected errors, just take the host as the result.

Moved, I know, but please correct the spelling/grammar here while we're here (s/this/these/)

::: browser/components/preferences/SiteDataManager.jsm:176
(Diff revision 2)
>      return this._getQuotaUsagePromise;
>    },
>  
> +  _getAllCookies() {
> +    let cookiesEnum = Services.cookies.enumerator;
> +    while (cookiesEnum.hasMoreElements()) {

This includes private cookies, the previous implementation did not. Can you elaborate on why you changed this? Was it deliberate?

::: browser/components/preferences/SiteDataManager.jsm:178
(Diff revision 2)
>  
> +  _getAllCookies() {
> +    let cookiesEnum = Services.cookies.enumerator;
> +    while (cookiesEnum.hasMoreElements()) {
> +      let cookie = cookiesEnum.getNext().QueryInterface(Ci.nsICookie2);
> +

Nit: I love my blank lines, but I don't think there's a need for one here.

::: browser/components/preferences/SiteDataManager.jsm:181
(Diff revision 2)
> +    while (cookiesEnum.hasMoreElements()) {
> +      let cookie = cookiesEnum.getNext().QueryInterface(Ci.nsICookie2);
> +
> +      let site = this._getOrInsertSite(cookie.rawHost);
> +      site.cookies.push(cookie);
> +      this._sites.set(cookie.rawHost, site);

r- for this and not having a comment. `_getOrInsertSite` is doing this already, right? Why do we need to re-set it here? It seems superfluous.

::: browser/components/preferences/SiteDataManager.jsm:291
(Diff revision 2)
> -        let cookie = e.getNext();
> -        if (cookie instanceof Components.interfaces.nsICookie) {
> -          if (this.isPrivateCookie(cookie)) {
> -            continue;
> -          }
> -          Services.cookies.remove(
> +      Services.cookies.remove(

Ugh. We really need a better batch removal API on the cookie service/manager. Anyway, this will do for now, I guess.

Why all the changes to this method? What specifically were you aiming to accomplish?

::: browser/components/preferences/SiteDataManager.jsm:323
(Diff revision 2)
>      return Promise.all(promises);
>    },
>  
>    remove(hosts) {
>      let unknownHost = "";
> -    let targetSites = [];
> +    let targetSites = new Map();

Why this change? It seems to have made code here strictly longer.

::: browser/components/preferences/SiteDataManager.jsm:330
(Diff revision 2)
>        let site = this._sites.get(host);
>        if (site) {
>          this._removePermission(site);
>          this._removeAppCache(site);
> -        this._removeCookie(site);
> -        targetSites.push(site);
> +        this._removeCookies(site);
> +        Services.obs.notifyObservers(null, "browser:purge-domain-data", host);

Why push this into here?

::: browser/components/preferences/SiteDataManager.jsm:427
(Diff revision 2)
>      //   4. User goes back to the Site Data section and commands to clear all site data.
>      // For this case, we should refresh the site list so not to miss the website in the step 3.
>      // We don't do "Clear All" on the quota manager like the cookie, appcache, http cache above
>      // because that would clear browser data as well too,
>      // see https://bugzilla.mozilla.org/show_bug.cgi?id=1312361#c9
> +    this._sites.clear();

Why do we need to clear the list of sites more regularly now? What difference does this make given what else this code does?
Attachment #8948551 - Flags: review?(gijskruitbosch+bugs) → review-
Ah, good point, I considered it small enough to be reviewable as a single patch, but I can see that I was wrong. I'll follow up with the remaining questions after splitting up. Sorry for that!
Comment on attachment 8948551 [details]
Bug 1421737 - Part 1 - Include cookies in SiteDataManager.jsm.

https://reviewboard.mozilla.org/r/217968/#review224818

> This includes private cookies, the previous implementation did not. Can you elaborate on why you changed this? Was it deliberate?

The enumerator does not include private cookies (but apparently does include other OAs): https://searchfox.org/mozilla-central/rev/b9f1a4ecba48b2d8c686669e32d109c40e927b48/netwerk/cookie/nsICookieManager.idl#43

I added a test for that.

> r- for this and not having a comment. `_getOrInsertSite` is doing this already, right? Why do we need to re-set it here? It seems superfluous.

Yes, thanks for catching that, it was left over from when I factored that code out into _getOrInsertSite.

> Ugh. We really need a better batch removal API on the cookie service/manager. Anyway, this will do for now, I guess.
> 
> Why all the changes to this method? What specifically were you aiming to accomplish?

We're not getting cookies for the host for deletion anymore, since we already have those now, including hosts with OAs (we have a test for it).

> Why this change? It seems to have made code here strictly longer.

Using a Map is for _removeServiceWorkersForSites because we need to pass it the hosts. It used to get its hosts from the principals associated with quota management, but we're not collecting principals for cookies (because it isn't needed except for this one function).

> Why push this into here?

purge-domain-data purges more than just cookie related stuff, it felt weird to have it in removeCookies

> Why do we need to clear the list of sites more regularly now? What difference does this make given what else this code does?

We used to clear all data in _getQuotaUsage() but that isn't correct anymore, since we also get cookies now.
I see that I have failed some tests on try. I'll investigate.
Comment on attachment 8949670 [details]
Bug 1421737 - Part 4 - Update site data manager tests to include cookies.

https://reviewboard.mozilla.org/r/219010/#review225206

r=me, though I don't understand the cancelButton change. Can you elaborate on that?

::: browser/components/preferences/in-content/tests/browser_siteData.js:206
(Diff revision 2)
> +    let siteItems = frameDoc.getElementsByTagName("richlistitem");
> +    is(siteItems.length, 1, "Should list one site with cookies");
> +    let sitesList = frameDoc.getElementById("sitesList");
> +    let site2 = sitesList.querySelector(`richlistitem[host="example.org"]`);
> +
> +    let columns = site2.querySelectorAll(".item-box > label");
> +    is(columns[0].value, "example.org", "Should show the correct host.");
> +    is(columns[2].value, "1", "Should show the correct number of cookies.");
> +    is(columns[3].value, "", "Should show no site data.");
> +
> +    let removeBtn = frameDoc.getElementById("removeSelected");
> +    let saveBtn = frameDoc.getElementById("save");
> +    site2.click();

Nitty nitpicking: It's odd that there's only 1 site here and it's being called `site2`. Like, it kind of makes sense in terms of being consistent with the previous item, and kind of doesn't.

If you remove `site2` first in the first test, then this bit of the test would 'naturally' make sense and could just refer to the same item as `site1` consistently? :-)

::: browser/components/preferences/in-content/tests/browser_siteData2.js:95
(Diff revision 2)
> +  cancelBtn = frameDoc.getElementById("cancel");
>    removeAllSitesOneByOne();
>    assertAllSitesNotListed(win);
>    saveBtn.doCommand();
>    await cancelPromise;
> +  cancelBtn.doCommand();

Why was this not necessary before?
Attachment #8949670 - Flags: review?(gijskruitbosch+bugs) → review+
Try failure was because I assumed the isPrivateCookie function was no longer needed, when in fact it was still used in the cookies dialog. I added it as a private method there. Try looks good now!
Comment on attachment 8949670 [details]
Bug 1421737 - Part 4 - Update site data manager tests to include cookies.

https://reviewboard.mozilla.org/r/219010/#review225206

> Nitty nitpicking: It's odd that there's only 1 site here and it's being called `site2`. Like, it kind of makes sense in terms of being consistent with the previous item, and kind of doesn't.
> 
> If you remove `site2` first in the first test, then this bit of the test would 'naturally' make sense and could just refer to the same item as `site1` consistently? :-)

Yeah, I agree, I can make it a little clearer, I guess.

> Why was this not necessary before?

Previously, cancelling the confirm dialog would cancel the entire site data manager as well. We found that confusing.
Comment on attachment 8948551 [details]
Bug 1421737 - Part 1 - Include cookies in SiteDataManager.jsm.

https://reviewboard.mozilla.org/r/217968/#review225550
Attachment #8948551 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8950158 [details]
Bug 1421737 - Part 2 - Convert the siteListItem XBL binding to plain JS and add a cookies row.

https://reviewboard.mozilla.org/r/219416/#review225552

::: commit-message-e2d75:3
(Diff revision 2)
> +This commit primarily intends to add a cookies to the site data manager,
> +but while touching this code I transformed the siteListItem XBL binding to plain JS.

Nit: fix English for "to add a cookies" :-)

::: browser/components/preferences/siteDataSettings.js:34
(Diff revision 2)
>  
> +  _createSiteListItem(site) {
> +    let item = document.createElement("richlistitem");
> +    item.setAttribute("host", site.host);
> +    let container = document.createElement("hbox");
> +    container.setAttribute("flex", "1");

Could set this in CSS.

::: browser/components/preferences/siteDataSettings.js:42
(Diff revision 2)
> +      label.setAttribute("crop", "end");
> +      label.setAttribute("flex", "1");

Could set these in CSS?

::: browser/components/preferences/siteDataSettings.js:60
(Diff revision 2)
> +    // Add "Status" column
> +    addColumnItem(site.persisted ?
> +      this._prefStrBundle.getString("persistent") : null, "2");
> +
> +    // Add "Cookies" column.
> +    addColumnItem(site.cookies.length, "1");

This will show an empty field instead of '0' for sites without cookies. Is that desired?

::: browser/components/preferences/siteDataSettings.js:64
(Diff revision 2)
> +    let str = this._prefStrBundle.getFormattedString("siteUsage", size);
> +    addColumnItem(str[0] > 0 ? str : null, "1");

This condition is unreliable. You have no control over the locale string here and so this will just be wrong in some locales where there's a non-numeric prefix.

Instead, this condition should almost certainly be on `size` itself, but I'm not sure what it should be because in fact it's not clear to me what the current condition is aiming to accomplish...

::: browser/components/preferences/siteDataSettings.js:128
(Diff revision 2)
>      if (isCurrentSortCol) {
>        // Sort on the current column, flip the sorting direction
>        sortDirection = sortDirection === "ascending" ? "descending" : "ascending";
>      }

Also in theory if we're flipping the sorting direction we should just reverse the items instead of re-sorting them (twice, after your patch)! Anyway, that's a pre-existing bug so I guess can go in a followup where we make this use a <tree> anyway.

::: browser/components/preferences/siteDataSettings.js:139
(Diff revision 2)
> -        sortFunc = (a, b) => {
> +      let bHost = b.baseDomain.toLowerCase();
> -          let aHost = a.host.toLowerCase();
> -          let bHost = b.host.toLowerCase();
> -          return aHost.localeCompare(bHost);
> +      return aHost.localeCompare(bHost);
> -        };
> +    };
> +    sites.sort(sortFunc);

Generally, if you're doing:

```js
x.sort(foo);
x.sort(bar);
```

You're spending twice as much time sorting as if you do:

```js
x.sort(foo (composition) bar);
```

because of additional comparison operations between items.

Given this list can actually be quite long in a 'normal' browser, especially now that we include cookies, we should make sure this code isn't a performance liability. I don't know exactly in what situations the previous code here didn't work for you, but please try to implement what you're doing without sorting repeatedly.

More generally, because this list can be quite long, it should probably use a <tree> rather than a rich list with a gazillion DOM nodes...)
Attachment #8950158 - Flags: review?(gijskruitbosch+bugs) → review-
(In reply to :Gijs from comment #26)
> More generally, because this list can be quite long, it should probably use
> a <tree> rather than a rich list with a gazillion DOM nodes...)

To be clear, this would be a follow-up, I don't expect you to fix that here.
Comment on attachment 8950159 [details]
Bug 1421737 - Part 3 - Simplify the "remove selected sites" dialog in site data management.

https://reviewboard.mozilla.org/r/219418/#review225554

This is already a <tree>, can we keep it as a <tree> instead of a <richlistbox> ?
Attachment #8950159 - Flags: review?(gijskruitbosch+bugs)
(In reply to :Gijs from comment #28)
> This is already a <tree>, can we keep it as a <tree> instead of a
> <richlistbox> ?

To be clear, happy with removing the nesting, but could still use a <tree> which will be faster when there's a lot of stuff in there, plus will make the patch smaller. :-)
Several of your points touch on this, so I'll make a top-level comment instead of replying to each of them.

I think the richlistbox we're using here is likely the performance bottleneck of this whole thing. Any operation we do is probably bounded by the performance of building and displaying those DOM rows. I believe it will be okay for now (the list is built only when opening the dialog and doesn't delay e.g. preferences opening) but I'm certainly not happy with it. We should totally file a follow-up bug for measuring and if necessary improving the performance of this dialog on slower machines. I'd prefer not to block on that.

We can still optimize the cases you mentioned (using a tree and not sorting twice), of course.
It's a bit unfortunate that if we go for using a tree for this (in a different bug), we lose the ability to add more styling to the individual rows. We didn't include any of that in our MVP, but we were considering adding labels or tags for things like persistent status, third party cookies and containers.
(In reply to Johann Hofmann [:johannh] from comment #31)
> It's a bit unfortunate that if we go for using a tree for this (in a
> different bug), we lose the ability to add more styling to the individual
> rows. We didn't include any of that in our MVP, but we were considering
> adding labels or tags for things like persistent status, third party cookies
> and containers.

You can still add images in trees, I believe, using CSS that is then based on the properties that you return from getCell/getRowProperties.
(In reply to :Gijs from comment #28)
> Comment on attachment 8950159 [details]
> Bug 1421737 - Part 3 - Simplify the "remove selected sites" dialog in site
> data management.
> 
> https://reviewboard.mozilla.org/r/219418/#review225554
> 
> This is already a <tree>, can we keep it as a <tree> instead of a
> <richlistbox> ?

I'm not really sure about the tree vs listbox. I think worrying about listbox performance for this dialog is a bit... premature and definitely not our worst real-life perf problem here, but I can certainly try and see if I can get a tree view with a similar amount of code done.

Thanks!
Comment on attachment 8950158 [details]
Bug 1421737 - Part 2 - Convert the siteListItem XBL binding to plain JS and add a cookies row.

https://reviewboard.mozilla.org/r/219416/#review225552

> Generally, if you're doing:
> 
> ```js
> x.sort(foo);
> x.sort(bar);
> ```
> 
> You're spending twice as much time sorting as if you do:
> 
> ```js
> x.sort(foo (composition) bar);
> ```
> 
> because of additional comparison operations between items.
> 
> Given this list can actually be quite long in a 'normal' browser, especially now that we include cookies, we should make sure this code isn't a performance liability. I don't know exactly in what situations the previous code here didn't work for you, but please try to implement what you're doing without sorting repeatedly.
> 
> More generally, because this list can be quite long, it should probably use a <tree> rather than a rich list with a gazillion DOM nodes...)

You're right, there's a more efficient way to do it so let's go with that. Sorting twice we're still on the same level of big O complexity (which is why I didn't care to be more sophisticated initially) but I guess it's better to optimize.
(In reply to Johann Hofmann [:johannh] from comment #34)
> Sorting twice we're still on the same level of big O complexity

Maybe in computational theory - I've forgotten too much of it by now. But in practice, that really depends on the sorting algorithm. The list sorts alphabetically, but then has to re-sort based on "something else". Depending on the sorting algorithm that may dramatically increase the number of comparisons required, which ends up being much slower than doing the entire compare in one place.

I learned some of this the hard way in bug 315913, 11 years ago - because we were adding an item to an already-sorted list, which in quicksort is pathetically slow. Much better to just insert in the right place "manually". And sure, in terms of big-O, re-sorting that list isn't that bad... but if you do it 200 times in a row after a netsplit then the difference between O(n log n) and O(.5n) starts to count, even if they're both P-time.

</grumpy-old-man>
Blocks: 1438147
Comment on attachment 8950158 [details]
Bug 1421737 - Part 2 - Convert the siteListItem XBL binding to plain JS and add a cookies row.

https://reviewboard.mozilla.org/r/219416/#review225552

> This will show an empty field instead of '0' for sites without cookies. Is that desired?

Yes, but I'll check with UX again to be sure.

> This condition is unreliable. You have no control over the locale string here and so this will just be wrong in some locales where there's a non-numeric prefix.
> 
> Instead, this condition should almost certainly be on `size` itself, but I'm not sure what it should be because in fact it's not clear to me what the current condition is aiming to accomplish...

I have no idea what my brain was doing while I wrote this. It should be dependent on site.usage.

> Also in theory if we're flipping the sorting direction we should just reverse the items instead of re-sorting them (twice, after your patch)! Anyway, that's a pre-existing bug so I guess can go in a followup where we make this use a <tree> anyway.

Yeah, it's a good point.

> You're right, there's a more efficient way to do it so let's go with that. Sorting twice we're still on the same level of big O complexity (which is why I didn't care to be more sophisticated initially) but I guess it's better to optimize.

I ended up dropping the double-sorting entirely. This was never a UX requirement (just me trying to be clever) and the more I think about this the more it's probably rather confusing that helpful. We could still do it in another bug instead of adding complexity to this one...
Comment on attachment 8950158 [details]
Bug 1421737 - Part 2 - Convert the siteListItem XBL binding to plain JS and add a cookies row.

https://reviewboard.mozilla.org/r/219416/#review226124
Attachment #8950158 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8950159 [details]
Bug 1421737 - Part 3 - Simplify the "remove selected sites" dialog in site data management.

https://reviewboard.mozilla.org/r/219418/#review226128

::: browser/components/preferences/siteDataRemoveSelected.js:50
(Diff revision 3)
>      },
> -    isSorted(index) {
> +    isSeparator(index) {
>        return false;
>      },
> -    canDrop() {
> +    isSorted() {
>        return false;

If we keep this, technically this list is sorted, right? :D

::: browser/components/preferences/siteDataRemoveSelected.js:55
(Diff revision 3)
>      getRowProperties() {},
>      getCellProperties() {},
>      getColumnProperties() {},
> -    hasPreviousSibling(index) {},
> +  },
> -    getImageSrc() {},
> -    getCellValue() {},

I don't really care, but was there a particular reason you left some of these as no-ops and removed others? The IDL includes all of them. Perhaps we can remove some more of these? Or did you test practically and are these minimum required set, as it were? :-)
Attachment #8950159 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8950159 [details]
Bug 1421737 - Part 3 - Simplify the "remove selected sites" dialog in site data management.

https://reviewboard.mozilla.org/r/219418/#review226128

> If we keep this, technically this list is sorted, right? :D

Hm, it seems only relevant for drag and drop, which isn't available here. Let's see if we can drop it.

> I don't really care, but was there a particular reason you left some of these as no-ops and removed others? The IDL includes all of them. Perhaps we can remove some more of these? Or did you test practically and are these minimum required set, as it were? :-)

Not really, what functions we have to implement here is sort of arcane to me, I can try removing as much as possible and seeing how that goes...
Comment on attachment 8950158 [details]
Bug 1421737 - Part 2 - Convert the siteListItem XBL binding to plain JS and add a cookies row.

https://reviewboard.mozilla.org/r/219416/#review226418
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a00a0ef8bfcb309fc1f67e94b33d248f82f061b
Bug 1421737 - Part 1 - Include cookies in SiteDataManager.jsm. r=Gijs

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a9b1b488c9426d5a9f07ca143e22486f67e2c77
Bug 1421737 - Part 2 - Convert the siteListItem XBL binding to plain JS and add a cookies row. r=Gijs

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9aa01a177517a7b0bdf1fb18b3fa8b634b56584
Bug 1421737 - Part 3 - Simplify the "remove selected sites" dialog in site data management. r=Gijs

https://hg.mozilla.org/integration/mozilla-inbound/rev/7fe3e62d6413a53e1ee4522b48a63af799b7acdb
Bug 1421737 - Part 4 - Update site data manager tests to include cookies. r=Gijs
Depends on: 1439171
Regressions: 1723525
You need to log in before you can comment on or make changes to this bug.