1239671 - Session Restore overrides persistent cookies (e.g. logged out from bugzilla on every restart)

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Honza Bambas (:mayhemer)]

Current nightly.  This started to manifest a week or so ago.  Every time I restart Firefox (after a regular day by day update restart or just after a single minute) I'm logged off from my bugzilla account.  Have to go through the 2FA again.  Looking then to the active sessions list in bugzilla's preferences I can see all the session are held (from the same IP) as expected.

I tried with Edge and it kept me logged in between browser sessions.

I also tried with the current Aurora and it also kept me logged in.

Hence I file under cookies as a Nightly bug.


Cookie settings:
- accept, no exceptions
- 3dr party: always
- keep until expiration

So, pretty standard as checked with a clean profile settings.

Comment 1

[Honza Bambas (:mayhemer)]

Any reaction on this?

Comment 2

[Katelyn Gadd (:kael)]

I'm losing my cookies on every exit/window close in Developer Edition as of a few days before (2016-02-04). The most obvious way to reproduce is Twitter (I get logged out every time) but it's also throwing away cookies for things like my bank website.

When I check the cookies database in sqlitespy there are cookies there so I'm not sure if it's throwing away specific cookies or just failing to read the database (db corruption like the localstorage bug?)

Comment 3

[Honza Bambas (:mayhemer)]

I've isolated the problem to session restore.  Looks like something stored (probably cookies) in sessionstore.js overrides the last obtained cookies after bugzilla login.  Needs to be confirmed.

Comment 4

[Honza Bambas (:mayhemer)]

Confirmed:
- after logging in, I see a certain value (say X) of Bugzilla_logincookie being sent to b.m.o
- I shut down
- check sessionstore.js for Bugzilla_logincookie, I can find a different (say Y) value for this cookie being stored
- start Fx again
- I can see that value Y is sent to to bugzilla now - the value from sessionstore.js overrides the previously obtained cookie


Going to investigate further.

Comment 5

[Honza Bambas (:mayhemer)]

This isn't a recent regression.  Checking the same profile with Firefox 41 I can get the same behavior.  Seems like something was broken in the session storage files.

Comment 6

[Honza Bambas (:mayhemer)]

Seems like I once logged in to bugzilla for only a session (unchecked the "Remember" box).  An old session-only cookie got into the session store cookie storage and since then it's reloading and overwrites the cookies stored in the cookie service.  When I log in again and leave the Remember box checked, the new cookie is not session-only and hence doesn't rewrite the one stored in session store.

This is incredibly stupid.

I think the following needs to be fixed:
- when there is a persistent (non-session-only) cookie in the cookie service, we must not overwrite it with session store's session only cookies
- when there is a session only cookie in the session store and a new value for the cookie is being set as persistent, the session stores cookie has to be deleted

Comment 7

[Honza Bambas (:mayhemer)]

Another question is, how is it possible we so much persist session-only cookies?

The most ironic on this is that the "Remember" check box actually behaves completely opposite way!  When I uncheck it, the session-only cookies is stored in session store and next time reused - so it actually remembers me.  When I check "Remember", the new cookies is not stored and previous session-only one is left...  So next time, I'm forgotten.  Completely crazy :)

Comment 8

[Honza Bambas (:mayhemer)]

Attachment: v1

- don't rewrite network cookie service cookies on session restore
- delete session-only cookies from session restore when a new cookie (the same host|path|name) is now a persistent cookie

The question about keeping session-only cookie across sessions this way still applies.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab6463e7e2c0

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8722107 [details] [diff] [review]
v1

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

::: browser/components/sessionstore/SessionCookies.jsm
@@ +116,5 @@
>     */
>    restore(cookies) {
>      for (let cookie of cookies) {
>        let expiry = "expiry" in cookie ? cookie.expiry : MAX_EXPIRY;
> +      if (!Services.cookies.cookieExists({host: cookie.host, path: cookie.path || "", name: cookie.name || ""})) {

Can you please make this fit on a line?

Comment 11

[Honza Bambas (:mayhemer)]

Attachment: v1.0.1

like this?

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8722681 [details] [diff] [review]
v1.0.1

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

::: browser/components/sessionstore/SessionCookies.jsm
@@ +118,5 @@
>      for (let cookie of cookies) {
>        let expiry = "expiry" in cookie ? cookie.expiry : MAX_EXPIRY;
> +      if (!Services.cookies.cookieExists({
> +        host: cookie.host, path: cookie.path || "", name: cookie.name || ""
> +      })) {

How about:

let cookieObj = {
  host: cookie.host,
  path: cookie.path || "",
  name: cookie.name || ""
};

if (!Services.cookies.cookieExists(cookieObj)) { ...

Comment 13

[Honza Bambas (:mayhemer)]

Attachment: v1.1

OK, like this? :)

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8722708 [details] [diff] [review]
v1.1

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

Great, thanks!

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/9cc28ef28c0f

Comment 16

[(no longer active)]

(In reply to Honza Bambas (:mayhemer) from comment #7)
> Another question is, how is it possible we so much persist session-only
> cookies?
> 
> The most ironic on this is that the "Remember" check box actually behaves
> completely opposite way!  When I uncheck it, the session-only cookies is
> stored in session store and next time reused - so it actually remembers me. 
> When I check "Remember", the new cookies is not stored and previous
> session-only one is left...  So next time, I'm forgotten.  Completely crazy
> :)

Sorry I am not familiar with what sessionstore does with cookies, but based on your investigation I agree that this is completely silly.  Thanks for fixing it!

Comment 17

[:Gijs (he/him)]

(In reply to Honza Bambas (:mayhemer) from comment #7)
> Another question is, how is it possible we so much persist session-only
> cookies?

bug 530594

Comment 18

[Honza Bambas (:mayhemer)]

(In reply to :Gijs Kruitbosch from comment #17)
> (In reply to Honza Bambas (:mayhemer) from comment #7)
> > Another question is, how is it possible we so much persist session-only
> > cookies?
> 
> bug 530594

Wow, very old problem.  Thanks for the reference.

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9cc28ef28c0f

Comment 20

[Katelyn Gadd (:kael)]

This should be fixed in 47.0a2 (2016-04-09), right?

I'm still having this problem every time I restart FF. If it helps, it seems like my pinned Twitter tab had a zillion cookies stored in sessionstore.js (???) and may have been restoring them.

Comment 21

[Andrew McCreight [:mccr8]]

dolske and Jesse reported a similar issue on Twitter: https://twitter.com/dolske/status/717912689456971776

Maybe file a new bug?