Closed
Bug 1239671
Opened 9 years ago
Closed 9 years ago
Session Restore overrides persistent cookies (e.g. logged out from bugzilla on every restart)
Categories
(Firefox :: Session Restore, defect)
Firefox
Session Restore
Tracking
()
RESOLVED
FIXED
Firefox 47
People
(Reporter: mayhemer, Assigned: mayhemer)
References
Details
Attachments
(1 file, 2 obsolete files)
1.96 KB,
patch
|
ttaubert
:
review+
|
Details | Diff | Splinter Review |
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•9 years ago
|
||
Any reaction on this?
Comment 2•9 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?)
Assignee | ||
Comment 3•9 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•9 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•9 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•9 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•9 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•9 years ago
|
||
- 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•9 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•9 years ago
|
Status: NEW → ASSIGNED
Updated•9 years ago
|
Attachment #8722107 -
Flags: review?(dtownsend) → review?(ttaubert)
Comment 10•9 years ago
|
||
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•9 years ago
|
||
like this?
Attachment #8722107 -
Attachment is obsolete: true
Attachment #8722681 -
Flags: review?(ttaubert)
Comment 12•9 years ago
|
||
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)) { ...
Updated•9 years ago
|
Has Regression Range: --- → no
status-firefox44:
--- → affected
status-firefox45:
--- → affected
status-firefox47:
--- → affected
status-firefox-esr38:
--- → ?
status-firefox-esr45:
--- → affected
Assignee | ||
Comment 13•9 years ago
|
||
OK, like this? :)
Attachment #8722681 -
Attachment is obsolete: true
Attachment #8722681 -
Flags: review?(ttaubert)
Attachment #8722708 -
Flags: review?(ttaubert)
Comment 14•9 years ago
|
||
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•9 years ago
|
Keywords: checkin-needed
Comment 15•9 years ago
|
||
Keywords: checkin-needed
Comment 16•9 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•9 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•9 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.
Updated•9 years ago
|
Flags: in-testsuite?
Comment 19•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Comment 20•9 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.
Comment 21•9 years ago
|
||
dolske and Jesse reported a similar issue on Twitter: https://twitter.com/dolske/status/717912689456971776
Maybe file a new bug?
You need to log in
before you can comment on or make changes to this bug.
Description
•