557047 - Replace mailnews specific ifdef (MOZ_MAIL_NEWS) in cookie code with tests for a protocol flag

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[Serge Gautherie (:sgautherie)]

And update comm-central in sync, of course.

/configure.in
{
6319 if test -n "$MOZ_PLACES"; then
6321     if test -z "$MOZ_MAIL_NEWS"; then
6322       MOZ_MORK=
6323     fi
6325 fi
}
The current m-c defaults are MOZ_PLACES=1 and MOZ_MORK=1: this seems a little odd wrt this code.
The values and the code should be reworked to be more explicit.

/extensions/cookie/nsCookiePermission.cpp
{
93 #ifdef MOZ_MAIL_NEWS
94 // returns PR_TRUE if URI appears to be the URI of a mailnews protocol
95 // XXXbz this should be a protocol flag, not a scheme list, dammit!
96 static PRBool
97 IsFromMailNews(nsIURI *aURI)
98 {
107 }
108 #endif

197 nsCookiePermission::CanAccess(nsIURI         *aURI,
200 {
201 #ifdef MOZ_MAIL_NEWS
204   if (IsFromMailNews(aURI)) {
208 #endif // MOZ_MAIL_NEWS
}
At least, rename it to "DENY_MAILNEWS_COOKIES" or the like.
Ideally, bz seems to suggest that each protocol should have a flag to allow/deny cookies ... which should let us get rid of these #ifdef :-)

Comment 1

[Mark Banner (:standard8)]

(In reply to comment #0)
> /configure.in

Dupe of bug 556253 which I just haven't checked in yet.

> /extensions/cookie/nsCookiePermission.cpp
...
> At least, rename it to "DENY_MAILNEWS_COOKIES" or the like.
> Ideally, bz seems to suggest that each protocol should have a flag to
> allow/deny cookies ... which should let us get rid of these #ifdef :-)

IMHO renaming is a waste of time and effort and it would be better to consider a long term solution for cookies (for which there are various bugs filed that would cover it iirc).

Comment 2

[Mark Banner (:standard8)]

Attachment: WIP mozilla-central patch

The basic idea here is to provide an opt-out flag for the URI to not allow it to access cookies. I'll attach the comm-central patch in a moment.

The patches work together for Thunderbird and pass tests, but I still need to fix (separate out) the unit test for TestCookie.cpp so that it keeps passing for Firefox, as Firefox doesn't have the mailnews protocols defined.

Comment 3

[Mark Banner (:standard8)]

Attachment: WIP comm-central patch

Comment 4

[Mark Banner (:standard8)]

Attachment: Proposed fix (mozilla-central)

So the mailnews cookie tests will be covered by the comm-central patch, hence we shouldn't really need them in the mozilla-central one. I'm going to push this to try and see if it comes back green.

Comment 5

[Mozilla RelEng Bot]

Try run for 019ee71450ce is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=019ee71450ce
Results (out of 4 total builds):
    exception: 3
    failure: 1
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-019ee71450ce

Comment 6

[Mark Banner (:standard8)]

Attachment: Proposed fix v2 (mozilla-central)

The previous patch had forgotten to take account of the PRBool rename.

Comment 7

[Mozilla RelEng Bot]

Try run for d8022c1e9bf7 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=d8022c1e9bf7
Results (out of 65 total builds):
    success: 54
    warnings: 4
    failure: 7
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-d8022c1e9bf7

Comment 8

[Mark Banner (:standard8)]

Attachment: Proposed fix (comm-central)

This is the comm-central part of the fix, with unit test.

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 567880 [details] [diff] [review]
Proposed fix v2 (mozilla-central)

Try server passed, the only failures where random orange. This gets the mailnews specific cookie stuff out of gecko and allows protocols to forbid cookies individually.

Comment 10

[Mark Banner (:standard8)]

Note for the build to work for Thunderbird in comm-central, the patch from bug 686278 must be applied first.

Comment 11

[David :Bienvenu]

Comment on attachment 568406 [details] [diff] [review]
Proposed fix (comm-central)

for some reason, my build failed in ldap with these two patches applied. I'll try a clobber build next.

Comment 12

[David :Bienvenu]

Comment on attachment 568406 [details] [diff] [review]
Proposed fix (comm-central)

ugh, apologies, but I've tried this several times, and my Windows build fails here:

nsLDAPService.cpp
c:/builds/tbirdhq/objdir-tb/ldap/xpcom/src/../../../../ldap/xpcom/src/nsLDAPURL.
cpp(48) : fatal error C1083: Cannot open include file: 'nsMsgUtils.h': No such f
ile or directory
c:\builds\tbirdhq\config\rules.mk:1308:0: command 'c:/mozilla-build/python/pytho
n2.6.exe -O c:/builds/tbirdhq/mozilla/build/cl.py cl -FonsLDAPURL.obj -c -D_HAS_

If I back out the patches, the build is fine. Clobbering didn't help.

Comment 13

[David :Bienvenu]

oh, I should say, I'm doing a pymake; not sure if that matters or not.

Comment 14

[Mark Banner (:standard8)]

Comment on attachment 568406 [details] [diff] [review]
Proposed fix (comm-central)

Did you apply the patch from the blocking bug?

Comment 15

[David :Bienvenu]

ah, sorry, I read that as having to apply the moz-central patch. With the other patch applied, this does build. The protocol tests (imap, mailbox, nntp) are failing, however. Perhaps I need to do a clobber build.

Comment 16

[David :Bienvenu]

clobber build didn't help - still seeing the protocol tests failing:

TEST-UNEXPECTED-FAIL | c:/builds/tbirdhq/objdir-tb/mozilla/_tests/xpcshell/mailn
ews/local/test/unit/test_mailboxProtocol.js | 32928 == 160 - See following stack
:
JS frame :: c:\builds\tbirdhq\mozilla\testing\xpcshell\head.js :: do_throw :: li
ne 453
JS frame :: c:\builds\tbirdhq\mozilla\testing\xpcshell\head.js :: _do_check_eq :
: line 547
JS frame :: c:\builds\tbirdhq\mozilla\testing\xpcshell\head.js :: do_check_eq ::
 line 568
JS frame :: c:/builds/tbirdhq/objdir-tb/mozilla/_tests/xpcshell/mailnews/local/t
est/unit/test_mailboxProtocol.js :: run_test :: line 30
JS frame :: c:\builds\tbirdhq\mozilla\testing\xpcshell\head.js :: _execute_test

I'll try without the patches, I guess.

Comment 17

[Mark Banner (:standard8)]

Oh, they are because we're testing the default protocol flags for those protocols and I've now changed the default flags. I'll fix in a bit once my compile's finished.

Comment 18

[Mark Banner (:standard8)]

Attachment: Proposed fix v2 (comm-central)

Fixed the unit tests.

Comment 19

[David :Bienvenu]

Comment on attachment 568900 [details] [diff] [review]
Proposed fix v2 (comm-central)

thx, that works. Since this is just test code that's being moved, I won't complain about the egregious code duplication :-)

Comment 20

[Mark Banner (:standard8)]

Comment on attachment 567880 [details] [diff] [review]
Proposed fix v2 (mozilla-central)

Not heard anything from dwitte :-( So trying some other cookie peers...

Comment 21

[Shawn Wilsher :sdwilsh]

Comment on attachment 567880 [details] [diff] [review]
Proposed fix v2 (mozilla-central)

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

r=sdwilsh

Comment 22

[Mark Banner (:standard8)]

Checked in both patches:

https://hg.mozilla.org/mozilla-central/rev/cb70391c86d9
http://hg.mozilla.org/comm-central/rev/38490f18f960

Comment 23

[Armen [:armenzg]]

This changeset has been pointed out by the email regression detector in dev.tree-management.

Could you please have a look at it?

Talos Regression :( Dromaeo (SunSpider) decrease 4.03% on Win7 Firefox-Non-PGO
Talos Regression :( Dromaeo (DOM) decrease 3.61% on Linux x64 Firefox-Non-PGO

This could have been blamed incorrectly. If so, if you mention it on the thread we can work on narrowing this down to other changes.