Create module that parses the p3p compact policies and makes its results available to others. This is needed by the cookie module in implementing p3p-based cookie management.

See bug 98882 for the other half of the story.

Save the attachment as privacy.tar.gz.

FYI: The attached tar ball does not contain changes to cookies,content module. Will post a patch soon once I resolve the conflicts.

Comment on attachment 54987 [details] [diff] [review] patch v1.0 [ cookie,content,plugin changes ] r=morse

cc'ing dp for r on the tarball patch cc'ing alecf for sr on both

Comment on attachment 54987 [details] [diff] [review] patch v1.0 [ cookie,content,plugin changes ] I don't understand why we load the P3P service and then never use it... doesn't that unnecessarily load the dll early?

>I don't understand why we load the P3P service and then never use it... It's only a trigger to load the P3P dll. That's, we load the p3p dll when p3p gets enabled via pref. >doesn't that unnecessarily load the dll early? No, it enables p3p service to listen to response headers before the cookie can ask for p3p's consent.

I guess it just seems hacky to me - I mean, what if your code to trigger the dll load doesn't get called? The way we normally handle this kind of system is via categories: the party that generates the events (in this case, http) enumerates members of a category (for instance, p3p), and instantiates them. This causes the dll to get loaded.. Maybe instead you should be using the http startup category stuff? that way p3p is loaded as soon as http is loaded? see nsHttpHandler.cpp for more info.

Comment on attachment 54987 [details] [diff] [review] patch v1.0 [ cookie,content,plugin changes ] I'm going to revoke my sr= until we get this issue resolved... (to make sure we've completely hashed this out before any code lands)

Yes, GetService(nsIP3PService) will load the dll. Per my discussion with Harish - P3P dll wont register HTTP startup category (wont be loaded on startup) - P3P will get loaded from cookie code when the pref for P3P is enabled If P3P registers for startup category, it will always get loaded no matter if the pref is ON or OFF. So we dont want that. Unfortunately we had to do the latter because we could be getting P3P header with no SetCookie header then the page could have a SetCookie from js - the SetCookie from js needs to obey the P3P header. For this to work, the moment P3P is enabled from perf, P3P needs to start watching the headers. If there was a way to get to the headers when Cookies calls nsIP3PService::GetConsent() when a JS setcookie happened, then we wont need it. Cookie will cause the p3p service dll load when it does GetService(). I believe there is no Channel when a js SetCookie() happens and hence no way to get the headers in the js SetCookie() case. Summary: P3P will never be loaded until the P3P pref is enabled. If the pref is enabled, it will get loaded on startup :-( I am told the pref is OFF by default.

Yes, the p3p pref is off by default. I'm glad that these points are being recorded in the bug report. For example, I had forgotten why harish's code needed to be listening to headers before I explicitly called him from the cookie module. dp's comment about js cookies reminded me of the reason.

Comment on attachment 55113 [details] [diff] [review] patch v1.1. yay.. talked with harish on the phone and this seems overall like the right approach. however... I just realized that this also introduces a build-time dependency on the p3p module... which really works against our embedding effort! I thought the dependency was already there... but the addition to makefile.win points out that it isn't. I'm going to sr= this for now, but I'm going to be revisiting this at a later point.

Comment on attachment 55113 [details] [diff] [review] patch v1.1. r=morse

Alec, does your sr apply to the attachment labelled "CP zipped" as well? The total checkin for this report consists of both the patch and the zip file. dp, have you reviewed the zip file?

no, I haven't looked. is this a tar.gz or a zip? It's listed as type "application/octet-stream" so I can't really tell one way or another

r=dp (Yeah I reviewed all of it)

It's bad enough that extension (which by definition should be optional) are required, but now we're coupling cookie to p3p at compile time? I think it is time to stop this tendency and then reverse it. Why can't we use XPCOM to couple cookie to p3p at runtime, optionally? /be

Comment on attachment 54935 [details] CP zipped [ untar under extensions ] r=dp Comment: Routines that parse the header and find out the policy etc can be improved.

I dont what you are refering to brendan : cookies uses nsIP3PService.h How can xpcom eliminate it ? XPCOM already eliminated the run-time dependency. The only way I see is folding P3P into cookies. But then, that doesn't make sense when we do full policy implementation.

It's a zip.

Oops, ignore previous comment. It was an answer to a question that alecf asked, but I just realized that he asked it in a private e-mail and not in this bug report.

OK, so ignore my very last comment (about ignoring other comment). Alecf's question was in this bug report after all. Sorry :-(

you can move the interface out of p3p into cookies, although that won't really make me happy. morse: it's a tgz which is not a zip. so says winzip [i updated the mime type to reflect this] Or is it absolutely impossible to create a new interface in cookies which p3p implements which is more generic? I don't think i mind p3p requiring cookies (maybe in some twisted world i might, but in general i just don't want p3p), but i do object to cookies requiring p3p.

dp, this is the same deal as remote LDAP prefs (where we don't want to make prefs depend at build-time on ldap, whether via a .h file or an XPCOM-generated .h from a .idl file, doesn't matter). The dependent module should purvey a "provider" or "callback" interface (XPCOM, of course) to the dependency to implement and set at runtime, if the dependency can discover the dependent module via the registry. /be

Note: The compact policy (CP) implementation does not completely follow December 2000 spec. For example, the implementation should make sure that the CP contains atleast one ACCESS element and one RETENTION element etc. Will work on it soon.

You should be working from the September 28, 2001 draft of the P3P spec. It has a number of clarifications and corrections from the December 2000 draft. This version is posted on the Web at http://www.w3.org/TR/P3P/ While the September 28 draft is listed as just a "working draft", I expect that it will be very close to the final draft.

Martin: My implementation is based on Sept. 2000 spec. :-)

ok, so the more I think about this, the more it bothers me. Mostly it bothers me in that I will have to go clean this up only a week or two after it lands. For embedding, we cannot have the cookies module depending on p3p. What this comes down to in the real world is that we cannot have "p3p" in the REQUIRES line for cookies. Here's a suggestion: make an interface, nsICookiePolicy, which resides in the cookie directory. p3p will implement this interface. Then simply have a well-known contract ID that is declared in the cookies module. Cookies will try to instantiate this generic Cookie policy service in the same way that it does in your patch.. basically all I'm suggesting is that the interface and contractID exist in the cookies module, and have a cookies-oriented name.

Alecf: Do I have your sr= for the tar ball?

Comment on attachment 55113 [details] [diff] [review] patch v1.1. actually given the issues that have come up, I'll sr= the initial tarball, but not the patch. I want to see this written in a way that does not require "p3p" to be added to the REQUIRES line.

Comment on attachment 54935 [details] CP zipped [ untar under extensions ] That said, I have a few comments on the tarball: 1) use of static nsHashtable - the C++ portability guidelines say "no static objects" 2) use of templates in FindCompactPolicy - I believe there is a non-template way to declare a reading iterator 3) in MapTokenToConsent, its not clear to me if aConsent is an "inout" or an "out" - I would have guessed it was "out" until I saw the default: handler in the switch statement, which looks at the current value. Can you document that? 4) can you use "const" on the iterators when you're not modifying them, such as in MapTokenToConsent? This will help distinguish between "in" and "inout" parameters (this may also apply to other functions) 5) instead of casting the result of .Get(), there is a NSPR macro for converting back and forth between pointers and integers 6) in a C++ context, using nsIObserver and nsIPrefBranchInternal::AddObserver as a way to listen for pref changes is the preferred method - nsIPref is deprecated and I've been asking people writing new code to use that interface.

>5) instead of casting the result of .Get(), there is a NSPR macro for >converting back and forth between pointers and integers It's in xpcom/base/nscore.h, actually: a pair of macros, NS_PTR_TO_INT32 and NS_INT32_TO_PTR. /be

harish, if you are checking in the tarball, then also check in all the cookie-module changes except for the actual call to the p3p code. That will remove the requirement of p3p on the cookie module, so alecf will be able to give you an sr now. And it will make it much simpler to do add the p3p call back in later.

I'll support that as well...:) the key is that we need to be able to continue to build the cookies module without ever entering the p3p directory.

Things seemed to have stalled here. Seems like harish has updated his patch. And alecf said he would approve it if the actual call from cookies to p3p is removed for the moment, thereby avoiding the p3p dependency on the cookie module. And that's where things stopped. So I'm attaching a revised version of harish's patch which does just that. My revised patch along with harish's tarball and what we are now asking approvals on. Now for reviews: harishd, please r my revised patch dp, please review harish's latest tarball alecf, does harish's revised tarball satisfy the requirements in your latest comment? If so, please sr the tarball alecf, please sr my revised patch

Comment on attachment 57055 [details] [diff] [review] cookie-module patch but with dependency on p3p module removed sr=alecf on this part.. will get to the p3p files now..

Ok, went over privacy.tar.gz. In the token hashtable you should use NEVER_OWN for the ownership on that nsCStringKey - since the table is static strings, there's no need for the extra allocation to hold a copy of the key. The only curiosities that I see now are why mCompactPolicies and mPolicyTable are pointers to objects? i.e. why allocate them seperately with operator new? you can do something like: class foo { nsHashTable mPolicyTable; } and then it will be allocated in the same space as "foo" I can understand if you're not sure these member objects will be created, but it looks quite certain that they will?

cookie-module patch checked in (does not call compact policy parser yet) still to do: - cookie module to call compact policy parser - compact policy parser tarball to be reviewed and checked in

--> 0.9.7

Splitting this up into two separate bugs so it can be properly linked to the navteam's dependency-blocking metabug (bug 112941). So now this bug applies only to harish's compact policy tarball which needs to get reviewed and checked in. New bug is bug 113143. It applies to the cookie module calling the code in the compact policy tarball.

Raising priority to P1.

dp: Need your r=.

what are the chances this will make 098? looks like we just need a r = from dp, right? dp - can you help?

no, I already reviewed the latest privacy.tar.gz, we need a new version which addresses my comments.

Out of time. Mass move to 0.9.9

Alecf: could you please sr=? Thanx.

ok, sorry to keep going around on this, but here's what's left: - I didn't mean to use NEVER_OWN _all_ the time - just in the case where you're dealing with static strings. In OnHeaderAvailable() you're getting a string that's probably owned by nsP3PService, and you have no guarantee that the string is going to stick around, so you have to own it. - in nsIP3PService's getConsent, you don't need [const] before aURI, all strings are [const] by implication of being an "in" parameter - I finally found the right iterator stuff.. you should be using nsACString::const_iterator, and not nsReadingIterator<char> - they're the same type, but this guarantees that you always use the right iterator, and you'll save the string mongers the overhead of fixing your code later. I promise I'll be faster with the final review.. we're close! :)

Comment on attachment 68187 [details] Patch v1.4 whoo hoo! sr=alecf Just keep in mind that we won't be adding a dependency where cookie depends on p3p... so as I suggested in the other bug, you could call nsIP3PService this something like nsICookieConsent and put it in cookie/public.. and then instantiate it via the category manager, or via some well-known ContractID

This has landed, but it is not enabled by default.

Please give instructions for enabling this so I can try it out.

I think on Windows just cd to mozilla/extensions/p3p and do nmake -f makefile.win depend all (You might need to do cvs up -dAP mozilla/extensions/p3p first, dunno if it is pulled by default.) Harish said it should build on Linux as well but I have not tested. There is no Mac project file yet (I am working on it).

Now I'm confused. I thought the stuff in extensions/p3p had to do with bug 62399. This bug is different and involves the parsing of the compact policy. It it also in extensions/p3p? It doesn't appear to be according to the tarball attached.

The reason harishd attached the patch as a tarball is because there was old code in p3p while he was working on completely new code in 'privacy' dir on his local disc. Before actually checking in, he removed the code in the existing p3p dir, and checked this new code in there. Everything is the same as in the tarball, just mentally replace 'privacy' dir with 'p3p'. We want to keep P3P stuff in p3p, and not create a new dir for it.

OK, I think I understand. The extensions/p3p directory did contain the stuff for bug 62399 up until yesterday. But as of today all that stuff was purged and the compact policy parser was put there instead. Is that correct?

mid-air collision. Looks like we were saying the same thing. Thanks for the clarificatin.

Comment on attachment 69725 [details] [diff] [review] patch v1.5 [ enable p3p on windows and unix ] r=heikki A nit in the makefile.win - would be nice if you used tab since all the other dirs are preceded by a tab.

Comment on attachment 69725 [details] [diff] [review] patch v1.5 [ enable p3p on windows and unix ] r=cls pending staff@mozilla.org approval.

Comment on attachment 69725 [details] [diff] [review] patch v1.5 [ enable p3p on windows and unix ] sr=alecf, what cls said.

Windows - build p3p upon request Unix - build if --enable-extensions=all

Why are we hedging our bets here? Can't we just build p3p without using the ifdef?

steve: I tried to get approval from staff@mozilla.org, to build p3p by default, in vain.

At least two tinderboxes will build all extensions, to prevent bit rot. Shrike and myotonic (the latter on SeaMonkey-Ports, the former on SeaMonkey) are the boxes. /be

+# if ($main::options{p3p}) +# { +# CreateJarFromManifest(":mozilla:extensions:p3p:resources:jar.mn", $chrome_dir, \%jars); +# } Why is this commented out, and not just removed?

Simon: Harish mentioned that he would be adding P3P resources to the build soon, and suggested merely commenting it out for now. :)

Simon: I was planning on adding resources but I'm not so sure now. I'll remove it, if you insist, and add it when needed.

You can leave it in if you wish, just comment that it may be used in future. sr=sfraser on the changes.

Comment on attachment 70163 [details] [diff] [review] patch v1.6 r=heikki I would recommend changing the define to MOZ_P3P on Windows though, because I believe our convention is: MOZ_? enable something DISABLE_? disable smg

Comment on attachment 70163 [details] [diff] [review] patch v1.6 sr=alecf

oops, with heikki's comments - prefix with MOZ_*, as in MOZ_ENABLE_P3P

What *is* our convention? Or what should it become during post-1.0 cleanup, and as we land stuff like this for 1.0? INCLUDE_XUL is yet another variation. /be

We really don't have a convention. We prefix some vars with MOZ_, others with NO_ and INCLUDE_XUL just really came out of left field. I'd vote for MOZ_ENABLE_ & MOZ_DISBABLE_ .

Is this going to change the leak stats or perf numbers on the tinderboxes where it's enabled? Last I checked (which was, I admit, almost a year ago), p3p leaked quite a bit.

The code that there now is brand new ( old code got removed ). I would prefer the numbers to be reported so that I can fix leaks if there are any.

I'm not sure how much I like the idea of turning this code on in order to detect leaks for you. Have you done A-B testing with Purify or our home-grown leak tools? If yes, and you haven't found any leaks, I'm a lot happier than if not and you want to let our leak tinderboxes find them for you. That's like checking in to find out if your code compiles on the Mac!

the tinderboxen will not use p3p, because it requires a pref to be set. I've reviewed the code and I'm pretty confident in it's memory management. (of course I could have missed the leak of some large root object.. :)) Anyway, as this won't be affecting tinderbox bloat data, I dont think that should be a criteria for deciding if we're going to check this in.

>Have you done A-B testing with Purify or our home-grown leak tools? Ok I shouldn't have mentioned about the numbers, because as Alecf pointed out P3P would be off by default and hence wouldn't affect the numbers. The main reason for enabling P3P is to avoid bit rotting [ which has already started happening ].

Comment on attachment 70163 [details] [diff] [review] patch v1.6 a=asa (on behalf of drivers) for checkin to the 1.0 trunk

changes landed on the trunk. To build P3P: WINDOWS ------- set MOZ_ENABLE_P3P = 1 UNIX ---- Add this line to mozconfig ac_add_options --enable-extensions=all MAC ---- mozilla\build\mac\build_scripts\MozillaBuildFlags.txt(64):p3p 1 Note: Please open new bugs for Compact policy related issues. Marking this bug fixed.

Verified.