Last Comment Bug 557047 - Replace mailnews specific ifdef (MOZ_MAIL_NEWS) in cookie code with tests for a protocol flag
: Replace mailnews specific ifdef (MOZ_MAIL_NEWS) in cookie code with tests for...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: Cookies (show other bugs)
: Trunk
: All All
: -- minor (vote)
: mozilla11
Assigned To: Mark Banner (:standard8)
:
:
Mentors:
http://mxr.mozilla.org/comm-central/s...
Depends on: 556253 686278
Blocks:
  Show dependency treegraph
 
Reported: 2010-04-03 19:08 PDT by Serge Gautherie (:sgautherie)
Modified: 2011-12-06 18:45 PST (History)
9 users (show)
standard8: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP mozilla-central patch (2.89 KB, patch)
2011-08-26 04:40 PDT, Mark Banner (:standard8)
no flags Details | Diff | Splinter Review
WIP comm-central patch (3.25 KB, patch)
2011-08-26 04:41 PDT, Mark Banner (:standard8)
no flags Details | Diff | Splinter Review
Proposed fix (mozilla-central) (3.25 KB, patch)
2011-10-18 13:05 PDT, Mark Banner (:standard8)
no flags Details | Diff | Splinter Review
Proposed fix v2 (mozilla-central) (5.17 KB, patch)
2011-10-18 14:15 PDT, Mark Banner (:standard8)
sdwilsh: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
Proposed fix (comm-central) (17.09 KB, patch)
2011-10-20 08:51 PDT, Mark Banner (:standard8)
no flags Details | Diff | Splinter Review
Proposed fix v2 (comm-central) (18.85 KB, patch)
2011-10-22 12:44 PDT, Mark Banner (:standard8)
mozilla: review+
Details | Diff | Splinter Review

Description Serge Gautherie (:sgautherie) 2010-04-03 19:08:59 PDT
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) 2010-04-04 13:20:10 PDT
(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) 2011-08-26 04:40:31 PDT
Created attachment 556001 [details] [diff] [review]
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) 2011-08-26 04:41:17 PDT
Created attachment 556002 [details] [diff] [review]
WIP comm-central patch
Comment 4 Mark Banner (:standard8) 2011-10-18 13:05:14 PDT
Created attachment 567849 [details] [diff] [review]
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 2011-10-18 14:01:56 PDT
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/bugzilla@standard8.plus.com-019ee71450ce
Comment 6 Mark Banner (:standard8) 2011-10-18 14:15:49 PDT
Created attachment 567880 [details] [diff] [review]
Proposed fix v2 (mozilla-central)

The previous patch had forgotten to take account of the PRBool rename.
Comment 7 Mozilla RelEng Bot 2011-10-19 01:00:43 PDT
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/bugzilla@standard8.plus.com-d8022c1e9bf7
Comment 8 Mark Banner (:standard8) 2011-10-20 08:51:33 PDT
Created attachment 568406 [details] [diff] [review]
Proposed fix (comm-central)

This is the comm-central part of the fix, with unit test.
Comment 9 Mark Banner (:standard8) 2011-10-20 08:55:34 PDT
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) 2011-10-21 03:41:08 PDT
Note for the build to work for Thunderbird in comm-central, the patch from bug 686278 must be applied first.
Comment 11 David :Bienvenu 2011-10-21 12:43:10 PDT
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 2011-10-21 15:56:28 PDT
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 2011-10-21 15:58:12 PDT
oh, I should say, I'm doing a pymake; not sure if that matters or not.
Comment 14 Mark Banner (:standard8) 2011-10-21 23:45:00 PDT
Comment on attachment 568406 [details] [diff] [review]
Proposed fix (comm-central)

Did you apply the patch from the blocking bug?
Comment 15 David :Bienvenu 2011-10-22 09:10:18 PDT
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 2011-10-22 10:51:19 PDT
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) 2011-10-22 11:49:01 PDT
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) 2011-10-22 12:44:46 PDT
Created attachment 568900 [details] [diff] [review]
Proposed fix v2 (comm-central)

Fixed the unit tests.
Comment 19 David :Bienvenu 2011-10-23 07:21:07 PDT
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) 2011-12-02 02:42:24 PST
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 2011-12-04 10:12:41 PST
Comment on attachment 567880 [details] [diff] [review]
Proposed fix v2 (mozilla-central)

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

r=sdwilsh
Comment 23 Armen Zambrano [:armenzg] (EDT/UTC-4) 2011-12-06 18:45:51 PST
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.

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