690992 - App tabs break deleting cookies on close (FF8+)

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Jackson Sul]

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

[Jackson Sul]

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]]

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]]

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 bugmail, email directly)]

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 bugmail, email directly)]

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 bugmail, email directly)]

Attachment: 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 bugmail, email directly)]

Attachment: 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)]

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 bugmail, email directly)]

pushed to fx-team

Comment 10

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

https://hg.mozilla.org/mozilla-central/rev/bc6854d8fe7d

Comment 11

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

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

[LegNeato]

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 bugmail, email directly)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4a3931156e57

Comment 14

[Virgil Dicu [:virgil] [QA]]

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 bugmail, email directly)]

(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]]

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.