Last Comment Bug 690992 - App tabs break deleting cookies on close (FF8+)
: App tabs break deleting cookies on close (FF8+)
Status: VERIFIED FIXED
[fixed-in-fx-team][qa!]
: privacy, regression
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: 8 Branch
: All All
: -- normal (vote)
: Firefox 10
Assigned To: Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
:
Mentors:
Depends on:
Blocks: 668865
  Show dependency treegraph
 
Reported: 2011-09-30 19:58 PDT by Aibara Iduas
Modified: 2011-11-24 08:07 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
Patch A+C v0.1 (1.96 KB, patch)
2011-11-02 14:27 PDT, Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
no flags Details | Diff | Review
Patch C v0.1 (3.16 KB, patch)
2011-11-02 16:24 PDT, Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
dietrich: review+
christian: approval‑mozilla‑aurora+
Details | Diff | Review

Description Aibara Iduas 2011-09-30 19:58:19 PDT
User Agent: Mozilla/5.0 (X11; Linux i686; rv:8.0) Gecko/20100101 Firefox/8.0
Build ID: 20110929094716

Steps to reproduce:

I set cookies to be kept until I close Firefox (FF8 on Ubuntu 10.10). 
I went to websites with cookies.
With an app tab set, I closed Firefox.
I opened Firefox again.
I found that all cookies were still present.
I went to new websites with cookies.
I again closed Firefox.
I opened Firefox a third time. 
I found the cookies created in the very first session were gone, but those created in the second were still present.


Actual results:

The cookies created in the last session are still present. Cookies that were initially created in the session before the last are actually deleted.

This only seems to happen with an app tab open (content doesn't matter, it could even be blank). I also tested this with a new profile with no extensions, so that shouldn't matter. If I have no app tabs set, all cookies are properly deleted each time Firefox is closed. 


Expected results:

All cookies should be deleted every time Firefox is closed.
Comment 1 Aibara Iduas 2011-10-23 00:02:44 PDT
I just tested the newest beta build, this time on a fresh Ubuntu 11.10 install. The problem is still present.

I suppose that the cookies from app tabs might be supposed to be kept? Or are they just immediately recreated when you restart? Not sure, but regardless all the rest should be deleted upon exit. Firefox 7 and older versions do not have this problem.
Comment 2 Virgil Dicu [:virgil] [QA] 2011-10-31 03:29:52 PDT
Mozilla/5.0 (Windows NT 5.1; rv:10.0a1) Gecko/20111030 Firefox/10.0a1
Mozilla/5.0 (X11; Linux x86_64; rv:10.0a1) Gecko/20111030 Firefox/10.0a1

Confirmed. The cookies from the previous session are still displayed after Firefox restart with an app tab. Another restart will erase them. 

Doesn't seem to occur for me on Firefox 7. To investigate for regression window.
Comment 3 Virgil Dicu [:virgil] [QA] 2011-10-31 05:28:54 PDT
Last good nightly: 2011-07-28
First bad nightly: 2011-07-29

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2011-07-28&enddate=2011-07-29
Comment 4 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-11-01 16:57:29 PDT
I just found this independently while looking at code. Bug 668865 is the change that made this happen.

Aibara, I appreciate you filing this a month ago. I'm sorry for the delay in reading this (it got lost in the pile of vacation bugmail).

I may be fixing this for free thanks to bug 698565, so stay tuned.

If we want a quick aurora/beta fix, simply removing "_host" and "_scheme" from INTERNAL_KEYS at https://hg.mozilla.org/mozilla-central/file/978002c0b0ad/browser/components/sessionstore/src/nsSessionStore.js#l122 should fix the problem and be a safe fix.
Comment 5 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-11-01 17:49:46 PDT
More specifically, this is happening because we get no hosts from _extractCookieHosts and so we end up building a regular expressions /.*()/ to match against hosts when splitting the app tab cookies out. every host is going to match that so :( We can build a more complete fix, but at this point I think the best short term action would be to back out bug 668865.
Comment 6 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-11-02 14:27:27 PDT
Created attachment 571459 [details] [diff] [review]
Patch A+C v0.1

There were 3 options essentially.
A: simply include _host & _scheme in the data written to disk
B: backout bug 668865
C: add fallback in _extractHostsForCookie (if _host/_scheme don't exist, figure them out) & fix the regex check

A mostly works except for the 1 time upgrade (when quitting & starting, not through restart process). This doesn't fix the case where there is only 1 app tab & it's an about: url though (contrived, but not impossible)
B is ok
C is riskiest, but most complete.

This patch does A, but also takes the regex fix from C. This saves face for most cases and in the one time upgrade (quit & start), we will just throw away many cookies until the session is restored (essentially opposite behavior of this bug). This errs on the side of privacy. On future startups we'll be back to normal behavior.
Comment 7 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-11-02 16:24:06 PDT
Created attachment 571499 [details] [diff] [review]
Patch C v0.1

Here is C. The tradeoff between these is data on disk vs xpcom. A+C would write more data to disk all the time. C will result in us generating (potentially a lot of) nsIURIs during 1 potential startup path (apptabs + not restoring)
Comment 8 Dietrich Ayala (:dietrich) 2011-11-02 16:52:56 PDT
Comment on attachment 571499 [details] [diff] [review]
Patch C v0.1

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

r=me, thanks for figuring this out!
Comment 9 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-11-03 13:48:45 PDT
pushed to fx-team
Comment 10 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-11-03 18:45:12 PDT
https://hg.mozilla.org/mozilla-central/rev/bc6854d8fe7d
Comment 11 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-11-07 14:16:05 PST
Comment on attachment 571499 [details] [diff] [review]
Patch C v0.1

Since we're tracking this for Firefox 9, asking approval for aurora now.
Comment 12 christian 2011-11-07 14:37:32 PST
Comment on attachment 571499 [details] [diff] [review]
Patch C v0.1

[triage comment]

Approved for aurora. Please land today.
Comment 13 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-11-07 15:09:11 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/4a3931156e57
Comment 14 Virgil Dicu [:virgil] [QA] 2011-11-17 08:01:43 PST
Mozilla/5.0 (X11; Linux x86_64; rv:9.0) Gecko/20100101 Firefox/9.0

Cookies received before selecting the "keep cookies until I close Firefox" option are still displayed after restarting. Is this expected?
Comment 15 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-11-19 08:44:15 PST
(In reply to Virgil Dicu [QA] from comment #14)
> Mozilla/5.0 (X11; Linux x86_64; rv:9.0) Gecko/20100101 Firefox/9.0
> 
> Cookies received before selecting the "keep cookies until I close Firefox"
> option are still displayed after restarting. Is this expected?

Yea, those prefs are in conflict. See bug 530594.
Comment 16 Virgil Dicu [:virgil] [QA] 2011-11-24 08:07:14 PST
Mozilla/5.0 (X11; Linux x86_64; rv:9.0) Gecko/20100101 Firefox/9.0
Mozilla/5.0 (Windows NT 6.1; rv:9.0) Gecko/20100101 Firefox/9.0
Mozilla/5.0 (Windows NT 5.1; rv:9.0) Gecko/20100101 Firefox/9.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0) Gecko/20100101 Firefox/9.0

Verified in Firefox9Beta3, 10 and 11 on Mac OS 10.6, Windows XP, Windows 7 and Ubuntu 11.10.

Steps used:
1. Start Firefox with a clean profile. 
2. From Edit-Preferences-Privacy set "Use custom settings from history" and keep cookies until close Firefox. (remove all existing cookies)
3. Pin an app tab.
4. Visit any site which stores cookies (e.g. www.tv5.org) in a normal tab.
5. Check that there are cookies saved from that specific site.
6. Restart Firefox.

Upon restart all cookies are now deleted.

Note You need to log in before you can comment on or make changes to this bug.