1319929 - TypeError: ContextualIdentityService.getIdentityFromId(...) is undefined

Status: RESOLVED DUPLICATE of bug 1337964

(Core :: DOM: Security, defect, P1)

Description

[Kamil Jozwiak [:kjozwiak]]

Attachment: issue.png

When you're removing containers or clearing cookies, you'll sometimes get into a situation where the cookie manager stops working under about:preferences#privacy and will appear completely blank. You'll also receive a "TypeError: ContextualIdentityService.getIdentityFromId(...) is undefined" error message under the browser console.

Platforms Used:
================

* macOS 10.12.1 x64 - reproduced on both fx53 and fx52
* Win 10 x64 - reproduced on both fx53 and fx52
* Ubuntu 16.04 x64 - reproduced on both fx53 and fx52

Browser Console Error:
======================

TypeError: ContextualIdentityService.getIdentityFromId(...) is undefined cookies.js:86:15
gCookiesWindow._isPrivateCookie chrome://browser/content/preferences/cookies.js:86:15
gCookiesWindow._loadCookies chrome://browser/content/preferences/cookies.js:496:13
gCookiesWindow._populateList chrome://browser/content/preferences/cookies.js:50:5
gCookiesWindow.init chrome://browser/content/preferences/cookies.js:33:5
onload chrome://browser/content/preferences/cookies.xul:1:1

STR:
====

* install and launch the latest version of fx
* open a new personal container via File -> New Container Tab
* load gmail.com within the personal container
* open a new tab and remove the personal container via about:preferences#containers
* ensure that the personal container is the only tab that's opened
* restart fx and click on "Restore Previous Session"
* open a new tab and visit the cookie manager via about:preferences#privacy

Comment 1

[Kamil Jozwiak [:kjozwiak]]

STR:

* open a freshly installed m-c
* open a personal container via "File -> New Container Tab"
* load facebook.com inside the personal container and close the remaining tabs
* open about:preferences#containers in a new tab and remove the personal container
* once the container has been removed, click on "Privacy" on the left hand side
* click on "remove individual cookies"

Short video of the issue being reproduced:
* https://youtu.be/kOHydArPJAE

Comment 2

[Andrea Marchesini [:baku]]

I cannot reproduce it with the latest nightly. Do you mind to check if it's already fixed?

Comment 3

[Kamil Jozwiak [:kjozwiak]]

As we discussed in the all hands, I went through the STR from comment#1 and could still reproduce the issue using the latest debug builds [1]. Using a non-debug build on my personal Windows desktop, I managed to reproduce this on a very old profile. The concerning part is that the issue was persistent even after restarting the browser several times. Usually when I reproduce this bug and restart the browser, the cookie manager is restored to it's correct behaviour.

[1] Debug builds being used:

* fx53.0a1, buildid: 20161214214334, changeset: b1ab720c6d3e (Ubuntu 16.04.1 LTS)
* fx53.0a1, buildid: 20161214214711, changeset: b1ab720c6d3e (Win 10 Pro x64)
* fx53.0a1, buildid: 20161214174132, changeset: 18b5a7a5d833 (macOS 10.12.2 x64)

Comment 4

[Andrea Marchesini [:baku]]

I'm not able to reproduce it. But maybe we can find the reason of this bug in the log. Kamil, can you please upload the log of FF when this issue appears?

Comment 5

[Kamil Jozwiak [:kjozwiak]]

I went through the STR using a debug build and attached the output from the terminal [1]. The original JS error occurs on line 151 [2]. There's also a bunch of assertions that occurred this time around but I'm not sure if they're related to this particular issue as they don't happen all the time.

Baku, let me know if there's another log that I can provide. I'm not sure if this is the log that you wanted.

[1] https://gist.github.com/kjozwiak/a2d82e2ffe36ab0a6ab4a61a75590a6c#file-terminallog-txt
[2] https://gist.github.com/kjozwiak/a2d82e2ffe36ab0a6ab4a61a75590a6c#file-terminallog-txt-L151

Comment 6

[Andrea Marchesini [:baku]]

Attachment: cookie.patch

When a container is deleted, nothing prevents other components to ask for that identity in the meantime.

Comment 7

[Andrea Marchesini [:baku]]

Comment on attachment 8820387 [details] [diff] [review]
cookie.patch

I know what the _real_ problem is. When a container tab is opened and the user wants to delete such container, we close that tab. Then we propagate a "clear-origin-attributes-data".

Unfortunately, it's possible that we have an active http connection and this sets a cookie _after_ the dispatching of "clear-origin-attributes-data".

When this happens, we have a cookie with a UCI, without having that container ID.

Comment 8

[Andrea Marchesini [:baku]]

Comment on attachment 8820387 [details] [diff] [review]
cookie.patch

I haven't found any better way to deal with this problem.

Comment 9

[:Gijs (he/him)]

Comment on attachment 8820387 [details] [diff] [review]
cookie.patch

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

I think this produces the wrong result for the name of the function (is this cookie private) in the most common case, as we don't yet support customizable private containers.

More generally, we should kill open http requests, service workers, etc. when a container goes away. That feels like a larger problem, and this feels very wallpapery. Is there a way to avoid that?

::: browser/components/preferences/cookies.js
@@ +85,5 @@
>        }
> +      let identity = ContextualIdentityService.getIdentityFromId(userContextId);
> +      // If the identity doesn't exist, probably it's because it has been
> +      // deleted but this cookie is still around.
> +      return !identity || !identity.public;

Why should the identity not existing imply the cookie is private? Shouldn't it be marked public?

Comment 10

[Andrea Marchesini [:baku]]

it seems that this issue happens only with e10s so definitely it's a timing issue: the tab is not completely closed when removeTab() returns.

Comment 11

[Tanvi Vyas[:tanvi]]

baku, are you still working on this or shoudl we unassign?  In your opinion, how important is this?

Comment 12

[Kamil Jozwiak [:kjozwiak]]

(In reply to Tanvi Vyas - not reading bugmail, PM me [:tanvi] from comment #11)
> baku, are you still working on this or shoudl we unassign?  In your opinion,
> how important is this?

I personally think we should fix this whenever possible. Unfortunately this bug is in the release version of fx as well, which is affecting the test pilot as well. I've managed to reproduce this bug a few times while going through the test pilot [1][2]. It's pretty bad regression as the user will lose functionality, and restarting fx doesn't always fix the problem.

From the above "See Also" from Gijs, it looks like people are running into this as well.

[1] https://github.com/mozilla/testpilot-containers/issues/172
[2] https://github.com/mozilla/testpilot-containers/issues/103

Comment 13

[Andrea Marchesini [:baku]]

I have submitted a patch for bug 1337964.

Comment 14

[Andrea Marchesini [:baku]]

Kamil, are you able to reproduce this issue after bug 1337964?

Comment 15

[Kamil Jozwiak [:kjozwiak]]

(In reply to Andrea Marchesini [:baku] from comment #14)
> Kamil, are you able to reproduce this issue after bug 1337964?

Nope, I can't reproduce this anymore after bug#1337964 landed. I went through the following test cases:

Reproduced the original issue using STR from comment#1 using the following build:
* fx54.0a1, buildid: 20170214030231, changeset: 195049fabb7a
** https://archive.mozilla.org/pub/firefox/nightly/2017/02/2017-02-14-03-02-31-mozilla-central/

Went through verification using the STR from comment#1 using the following build:
* fx55.0a1, buildid: 20170321030211, changeset: 5fe5dcf1c10a
* fx54.0a2, buildid: 20170321004003, changeset: 0496e366a8f4

Platforms Used:

* macOS 10.12.3 x64 - PASSED
* Win 10 Pro x64 VM - PASSED
* Ubuntu 16.04.2 LTS x64 VM - PASSED