Closed Bug 104894 Opened 23 years ago Closed 23 years ago

P3P: Parsing of Compact Policies

Categories

(Core :: Networking: Cookies, defect, P1)

x86
Windows NT
defect

Tracking

()

VERIFIED FIXED
mozilla0.9.9

People

(Reporter: morse, Assigned: harishd)

References

Details

Attachments

(11 files, 2 obsolete files)

6.74 KB, application/x-gzip
Details
7.00 KB, application/x-gzip
Details
13.91 KB, patch
harishd
: review+
Details | Diff | Splinter Review
7.18 KB, application/x-gzip
Details
7.27 KB, application/x-gzip
Details
1.25 KB, patch
hjtoi-bugzilla
: review+
Details | Diff | Splinter Review
1.09 KB, patch
hjtoi-bugzilla
: review+
Details | Diff | Splinter Review
19 bytes, text/plain
Details
145.08 KB, text/plain
Details
102.82 KB, text/plain
Details
1.48 KB, patch
Details | Diff | Splinter Review
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.
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla0.9.6
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
Attachment #54987 - Flags: review+
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?
Attachment #54987 - Flags: superreview+
>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)
Attachment #54987 - Flags: superreview+
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.
Attached patch patch v1.1. (obsolete) — Splinter Review
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.
Attachment #55113 - Flags: superreview+
Attachment #54987 - Attachment is obsolete: true
Comment on attachment 55113 [details] [diff] [review]
patch v1.1.

r=morse
Attachment #55113 - Flags: review+
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
Attachment #54935 - Attachment mime type: application/octet-stream → application/x-gzip
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
Attachment #54935 - Flags: review+
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.
Attachment #55113 - Flags: superreview+
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.
Attachment #54935 - Flags: needs-work+
>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
Attachment #57055 - Flags: review+
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..
Attachment #57055 - Flags: superreview+
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
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Blocks: 112941
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.
Blocks: 113143
No longer blocks: 112941
Raising priority to P1.
Priority: P2 → P1
dp: Need your r=.
Target Milestone: mozilla0.9.7 → mozilla0.9.8
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
Target Milestone: mozilla0.9.8 → mozilla0.9.9
Keywords: nsbeta1
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! :)
Attached file Patch v1.4
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
Attachment #68187 - Flags: superreview+
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.
Attachment #69725 - Flags: review+
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.
Attachment #69725 - Flags: superreview+
Attached patch patch v1.6 Splinter Review
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
Attachment #70163 - Flags: review+
Comment on attachment 70163 [details] [diff] [review]
patch v1.6 

sr=alecf
Attachment #70163 - Flags: superreview+
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
Attachment #70163 - Flags: approval+
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.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
QA Contact: tever → junruh
Verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: