1337964 - Contextual Identity seem to break the Cookie Manager

Status: VERIFIED FIXED

(Firefox :: Settings UI, defect, P1)

Description

[Théo Chevalier]

Attachment: Screenshot

Clicking on Privacy > “remove individual cookies” opens an empty, broken panel (See screenshot).

This triggers the following error in the Browser console:

TypeError: ContextualIdentityService.getIdentityFromId(...) is undefined[En savoir plus]  SiteDataManager.jsm:253:30
	isPrivateCookie resource://app/modules/SiteDataManager.jsm:253:30
	_loadCookies chrome://browser/content/preferences/cookies.js:480:13
	_populateList chrome://browser/content/preferences/cookies.js:52:5
	init chrome://browser/content/preferences/cookies.js:35:5
	onload chrome://browser/content/preferences/cookies.xul:1:1
websocket closed  main.598ce99103ba6aaf3db4.js:29
websocket re-established connection  main.598ce99103ba6aaf3db4.js:29

Tried with a fresh profile and can’t reproduce, so I’m guessing this has something to do with having a custom Contextual Identity, and having cookies set with this identity. I’m experiencing the issue with the latest Nightly.

Not sure if Contextual Indentity’s fault, or Preferences, so leaving untriaged.

Comment 1

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

This is ContextualIdentityService.jsm's fault: The stack is from loading the cookies into the cookie manager of the preferences.

I was also able to reproduce the issue (maybe related to using private browsing). Observations:

There is a cookie without first party domain and userContextId 5. This has the attribute "public" set to false. But getIdentityFromId returns only identities which have public set to true: https://dxr.mozilla.org/mozilla-central/rev/5e17f9181c6cb0968966280d1c1d96e725702af1/toolkit/components/contextualidentity/ContextualIdentityService.jsm#259

So the return value is undefined in SiteDataManager.jsm calls |undefined.public|.

Comment 2

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Attachment: Bug 1337964 - cookies of background thumbnail service container shouldn't break cookie manager.

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

Comment 3

[:Gijs (he/him)]

Comment on attachment 8835458 [details]
Bug 1337964 - cookies of background thumbnail service container shouldn't break cookie manager.

https://reviewboard.mozilla.org/r/111218/#review112368

::: browser/components/preferences/SiteDataManager.jsm:251
(Diff revision 1)
> +  /* "Private" means "internal" here. E.g. the thumbnail service runs in its
> +     own inaccessible container with its own cookies. */

Then please rename the method. It's dumb to have a public method whose name makes no sense.

X-ref bug 1319929 which has a different patch to a similar function that's in cookies.js, rather than SiteDataManager.jsm .

Please sort out with baku what the right name is, maybe put the method somewhere more sensible (like ContextualIdentityService?) and make sure the implementation is correct. The comment in :baku's patch on 1319929 suggests that we'll also return null if the identity is gone but the cookie still exists, which is going to be a problem because then we will assume here that the cookie is private and then not delete it, which is wrong.

Comment 4

[Andrea Marchesini [:baku]]

I would rename:
1. ContextualIdentityServices.getIdentities to getPublicIdentities
2. ContextualIdentityServices.getIdentityFromId to getPublicIdentityFromId

plus:

3. SiteDataManager.jsm isPrivateCookie(cookie) can simply do:
  return !!ContextualIdentityService.getPublicIdentityFromId(userContextId);

Comment 5

[Andrea Marchesini [:baku]]

Attachment: public.patch

I actually wrote this patch for bug 1319929 and it fixes this bug as well.

Comment 6

[:Gijs (he/him)]

Comment on attachment 8836143 [details] [diff] [review]
public.patch

Review of attachment 8836143 [details] [diff] [review]:
-----------------------------------------------------------------

I think this still has the same problem I've been asking about for a while - what happens when public identities are deleted and we ask about those from 'stale' data from other sources? It looks to me like this patch too will treat those as items being associated with the 'private' identity.

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Comment on attachment 8836143 [details] [diff] [review]
public.patch

Review of attachment 8836143 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/preferences/SiteDataManager.jsm
@@ +249,5 @@
>    },
>  
>    isPrivateCookie(cookie) {
>      let { userContextId } = cookie.originAttributes;
> +    return !!ContextualIdentityService.getPublicIdentityFromId(userContextId);

That's one negation more than before, so the cookies for existing userContextIds (1-4) won't be shown, the ones for the internal thumbnail service (5), the default context (0; not defined in _defaultIdentities in ContextualIdentityService.jsm) and the ones of none-existing contexts will be shown.
As far as I understand it, the cookies for 0 and 1-4 (and of course contexts added by the user) should be shown, for 5 and none-existing ones not.

Comment 8

[Andrea Marchesini [:baku]]

Attachment: log.patch

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Comment on attachment 8836717 [details] [diff] [review]
log.patch

Review of attachment 8836717 [details] [diff] [review]:
-----------------------------------------------------------------

This patch lacks all the changes to ContextualIdentityService.jsm.

::: browser/components/preferences/SiteDataManager.jsm
@@ +249,5 @@
>    },
>  
>    isPrivateCookie(cookie) {
>      let { userContextId } = cookie.originAttributes;
> +    return !ContextualIdentityService.getPublicIdentityFromId(userContextId);

switching from !! to ! returns the expected result for publicly listed containers (false), but not the default user context. So how about this check:
> // 0 is the id of the default user context
> userContextId != 0 && !ContextualIdentityService.getPublicIdentityFromId(userContextId)

Comment 10

[Andrea Marchesini [:baku]]

Attachment: log.patch

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Comment on attachment 8837148 [details] [diff] [review]
log.patch

Review of attachment 8837148 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/preferences/SiteDataManager.jsm
@@ +249,5 @@
>    },
>  
>    isPrivateCookie(cookie) {
>      let { userContextId } = cookie.originAttributes;
> +    return !userContextId || !ContextualIdentityService.getPublicIdentityFromId(userContextId);

For the default user context with the the id, !0 will return true - but it's not a private one, so we need false.

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Also, the || will cause this line to always return if the user context id is not in the publicly listed ones.

Comment 13

[Andrea Marchesini [:baku]]

Attachment: log.patch

Comment 14

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Comment on attachment 8837449 [details] [diff] [review]
log.patch

Review of attachment 8837449 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you, redirecting to Gijs who is an official reviewer.

Comment 15

[:Gijs (he/him)]

Comment on attachment 8837449 [details] [diff] [review]
log.patch

Review of attachment 8837449 [details] [diff] [review]:
-----------------------------------------------------------------

Ugh. We need to just fix this at this point, so I guess r=me, but we really also need a followup filed to fix the problem with deleted identities. We shouldn't just keep those cookies in perpetuity and never show them in the cookie or site manager, and potentially reuse them if we ever reuse identity numbers. :-\

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

(In reply to :Gijs from comment #15)
> Ugh. We need to just fix this at this point, so I guess r=me, but we really
> also need a followup filed to fix the problem with deleted identities. We
> shouldn't just keep those cookies in perpetuity and never show them in the
> cookie or site manager, and potentially reuse them if we ever reuse identity
> numbers. :-\
Kamil filed bug 1338735 for that.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/70af77d96837
SiteDataManager should check correctly if a cookie belongs to a private identity or not, r=aryx, r=gijs

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/70af77d96837

Comment 20

[Kamil Jozwiak [:kjozwiak]]

Went through verification in bug#1319929comment#14.