431125 - Various Cookie tests fail when running against Thunderbird

Status: RESOLVED FIXED

(Thunderbird :: General, defect, P2)

Description

[Mark Banner (:standard8)]

The following tests fail when running against Thunderbird:

test_cookie_header.js:
*** CHECK FAILED: C1=V1 == C2=V2; C1=V1

test_cookies.js:
WARNING: NS_ENSURE_TRUE(aChannel) failed: file /Users/moztest/mozilla/hg/mozilla/mailnews/base/src/nsMsgContentPolicy.cpp, line 627
WARNING: NS_ENSURE_TRUE(docShellTreeItem) failed: file /Users/moztest/mozilla/hg/mozilla/mailnews/base/src/nsMsgContentPolicy.cpp, line 632
*** CHECK FAILED: 1 == 2

TestCookies.cpp:
*** Beginning mailnews tests...
*** tests 0 1 2 FAILED!

I believe all of these are probably due to the way Thunderbird handles its cookies.

I'm filing this to get it tracked. Not sure if this is a core or a Thunderbird specific problem yet. Will post more info when I have it.

Comment 1

[Mark Banner (:standard8)]

I can get test_cookie_header.js (run as part of check in network/test) and test_cookies.js (run as part of extensions/cookie/test) to pass if I change Thunderbird's cookie policy to enable all cookies:

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mailnews/base/src/nsMsgContentPolicy.cpp&rev=1.52&mark=626#621

Change that line to ACCESS_DEFAULT. I guess we limit to RSS cookies for a reason. I'm not sure if a pref would be best here or some other way of overriding this for the tests?

I've still got to look at TestCookie.cpp.

Comment 2

[Mark Banner (:standard8)]

(In reply to comment #1)
> I've still got to look at TestCookie.cpp.

I've not tracked the actual source of the TestCookies.cpp failure yet, but I bet its something to do with the fact that Thunderbird overrides the Cookie Permission manager (http://mxr.mozilla.org/seamonkey/source/extensions/cookie/nsCookiePermission.cpp) with its own (http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mailnews/base/src/nsMsgContentPolicy.cpp&rev=1.52&mark=609-668#609).

Even more fun, nsCookiePermission.cpp has code wrapped with MOZ_MAIL_NEWS defines to handle rejection of cookies on mailnews protocols (i.e. imap/mailbox etc).

This could be an interesting one to fix.

Comment 3

[Mark Banner (:standard8)]

Bug 468700 has just exposed Thunderbird's bad cookie handling even more, test_bug468700.js is failing:

NEXT ERROR TEST-UNEXPECTED-FAIL | ../../../_tests/xpcshell-simple/test_cookies/unit/test_bug468700.js | test failed, see log
../../../_tests/xpcshell-simple/test_cookies/unit/test_bug468700.js.log:
>>>>>>>
*** test pending
*** exiting
NEXT ERROR *** TEST-UNEXPECTED-FAIL | ../../../_tests/xpcshell-simple/test_cookies/unit/test_bug468700.js | 2 == 1
JS frame :: /buildbot/linux-comm-central-check/build/mozilla/testing/xpcshell/head.js :: do_throw :: line 109
JS frame :: /buildbot/linux-comm-central-check/build/mozilla/testing/xpcshell/head.js :: do_check_eq :: line 128
JS frame :: ../../../_tests/xpcshell-simple/test_cookies/unit/test_bug468700.js :: run_test :: line 28
JS frame :: /buildbot/linux-comm-central-check/build/mozilla/testing/xpcshell/tail.js :: _execute_test :: line 41
JS frame :: /buildbot/linux-comm-central-check/build/mozilla/testing/xpcshell/execute_test.js :: <TOP_LEVEL> :: line 38
2147500036
*** FAIL ***

<<<<<<<

The test added there starts off by checking part of the lifetime policy is working correctly - Thunderbird has no concept of this.


The issue is the cookie handling policy - Thunderbird overrides the core policy code to allow only RSS cookies (see comment 2 and bug 275131). It doesn't understand about lifetimes or asking to accept cookies etc.

We could potentially remove the Thunderbird specific code, and set network.cookie.disableCookieForMailNews to true, however I believe that RSS cookies would still be disabled as the extensions/cookie code checks for the uri with the cookie originating from a load context of type APP_TYPE_MAIL and denies cookies:

http://hg.mozilla.org/releases/mozilla-1.9.1/file/d00b0d0d4b64/extensions/cookie/nsCookiePermission.cpp#l209

Special-casing RSS would be difficult in the extensions/cookies code as it is a complex process to work out if its an RSS article or not, needing access to mailnews code:

http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgUtils.cpp#946

FWIW I think SeaMonkey will have the same problem with RSS + cookies in mailnews - they won't work currently.

For Thunderbird, we'd potentially have to pick up some of the cookie UI, which may not be too bad as its constrained to preferences, but its not really necessary afaict.

I'm not sure if we should try and sort this for TB 3.0. I'd certainly like to get opinions on what we can do to resolve this.

In the short term Thunderbird can disable the test in the same way as the other cookie tests, although we ought to fix the issues - or have them scheduled/a way forward for them to be fixed.

Really the extensions/cookie code should be extensible and not require specific mailnews ifdefs in its code. I'm not sure what it would look like and its certainly not likely to happen in the 1.9.1 timeframe.

Comment 4

[Serge Gautherie (:sgautherie)]

(In reply to comment #3)
> Bug 468700 has just exposed Thunderbird's bad cookie handling even more,
> test_bug468700.js is failing:
> 
> FWIW I think SeaMonkey will have the same problem with RSS + cookies in
> mailnews - they won't work currently.

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090124 SeaMonkey/2.0a3pre] (experimental/_m-c_, home, optim default) (W2Ksp4)
(http://hg.mozilla.org/mozilla-central/rev/096ab3b6abe0
 +http://hg.mozilla.org/comm-central/rev/176d86062c81)

Fwiw, current SM/1.9.2 passes this test.

Comment 5

[Mark Banner (:standard8)]

(In reply to comment #4)
> > FWIW I think SeaMonkey will have the same problem with RSS + cookies in
> > mailnews - they won't work currently.

> Fwiw, current SM/1.9.2 passes this test.

Sorry, maybe I was clear. AFAICT SeaMonkey won't accept cookies on RSS feeds due to the APP_MAIL_TYPE check in the extensions/cookie code (SeaMonkey doesn't have a cookie manager override).

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

It's hard to decide what to do about this without having a better idea of what the real-world implications of the various options are.  In particular, it would be helpful to nail down a holistic view of where in messaging clients users generally expect cookie-gated features to work.

I assume this is not a regression.  Is that correct?

Comment 7

[Mark Banner (:standard8)]

(In reply to comment #6)
> I assume this is not a regression.  Is that correct?

Without doing explicit tests as far as I can tell our cookie handling is the same as in Thunderbird 2.

The TUnit tests which are currently failing on trunk, and the ones we have disabled, just show up the "broken" nature of what we do have.

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

My suggestion is that this should be blocking- until such time as someone outlines a real-world use case that's currently broken and is important enough to block.

Comment 9

[Mark Banner (:standard8)]

I've just been talking to dmose on irc about this. We've decided we won't block on it as there doesn't seem to be a major issue here; though we would like to fix it for the unit tests etc.

For now though, we'll be disabling the unit test to allow the trunk tree to be green and run its full unit tests.

Comment 10

[Mark Banner (:standard8)]

Attachment: Disable new cookie test

As previously mentioned, we'll disable the new cookie test until such time as we fix this bug, so at least we can have a green TUnit which tests all the test we can pass.

Comment 11

[Mark Banner (:standard8)]

Bug 492279 attachment 382275 [details] [diff] [review] backed out the patch for disabling the cookie tests - hence the cookie tests now all pass :-)

Resolving fixed.

Comment 12

[Serge Gautherie (:sgautherie)]

(In reply to comment #5)
> Sorry, maybe I was clear. AFAICT SeaMonkey won't accept cookies on RSS feeds
> due to the APP_MAIL_TYPE check in the extensions/cookie code (SeaMonkey doesn't
> have a cookie manager override).

See bug 501925 ;-)