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

RESOLVED FIXED in Firefox 47

Status

()

--
major
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mayhemer, Assigned: mayhemer)

Tracking

Trunk
Firefox 47
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox44 wontfix, firefox45 affected, firefox46 affected, firefox47 fixed, firefox-esr38 ?, firefox-esr45 affected)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Any reaction on this?

Comment 2

3 years ago
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?)
See Also: → bug 1249718
(Assignee)

Comment 3

3 years ago
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.
Component: Networking: Cookies → Session Restore
Product: Core → Firefox
(Assignee)

Comment 4

3 years ago
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.
Assignee: nobody → honzab.moz
Summary: Logged out from bugzilla on every Firefox restart → Session Restore overrides cookies (e.g. logged out from bugzilla on every restart)
(Assignee)

Comment 5

3 years ago
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.
(Assignee)

Comment 6

3 years ago
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
(Assignee)

Comment 7

3 years ago
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 :)
Flags: needinfo?(ehsan)
(Assignee)

Comment 8

3 years ago
Created attachment 8722107 [details] [diff] [review]
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
Attachment #8722107 - Flags: review?(dtownsend)
(Assignee)

Updated

3 years ago
Summary: Session Restore overrides cookies (e.g. logged out from bugzilla on every restart) → Session Restore overrides persistent cookies (e.g. logged out from bugzilla on every restart)
(Assignee)

Updated

3 years ago
Status: NEW → ASSIGNED
Attachment #8722107 - Flags: review?(dtownsend) → review?(ttaubert)
Duplicate of this bug: 1221480
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?
Attachment #8722107 - Flags: review?(ttaubert) → feedback+
(Assignee)

Comment 11

3 years ago
Created attachment 8722681 [details] [diff] [review]
v1.0.1

like this?
Attachment #8722107 - Attachment is obsolete: true
Attachment #8722681 - Flags: review?(ttaubert)
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)) { ...
Has Regression Range: --- → no
status-firefox44: --- → affected
status-firefox45: --- → affected
status-firefox47: --- → affected
status-firefox-esr38: --- → ?
status-firefox-esr45: --- → affected
(Assignee)

Comment 13

3 years ago
Created attachment 8722708 [details] [diff] [review]
v1.1

OK, like this? :)
Attachment #8722681 - Attachment is obsolete: true
Attachment #8722681 - Flags: review?(ttaubert)
Attachment #8722708 - Flags: review?(ttaubert)
Comment on attachment 8722708 [details] [diff] [review]
v1.1

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

Great, thanks!
Attachment #8722708 - Flags: review?(ttaubert) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed

Comment 16

3 years ago
(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!
Flags: needinfo?(ehsan)

Comment 17

3 years ago
(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
(Assignee)

Comment 18

3 years ago
(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.
status-firefox44: affected → wontfix
Flags: in-testsuite?

Comment 19

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9cc28ef28c0f
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47

Comment 20

3 years ago
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.
dolske and Jesse reported a similar issue on Twitter: https://twitter.com/dolske/status/717912689456971776

Maybe file a new bug?
(Assignee)

Updated

2 years ago
No longer blocks: 1264192
Depends on: 1353533
You need to log in before you can comment on or make changes to this bug.