210491 - [p3p] shouldn't be observing nsIHttpNotify notifications

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[[e-mail]]

I've recently been looking at integrating cookies into necko, and I noticed that
nsP3PService implements nsIHttpNotify, and registers itself as an observer for
incoming HTTP headers.

This may have some future goal in mind, but as it stands, I see no reason for
this - cookies will call up the P3PService as required, and give it an
nsIHttpChannel to work with. So right now we're just wasting cycles doing the
p3p header parsing twice. (Cookies also has some inefficiencies here, because
it'll call up the P3PService twice in some cases, for the same URI - but I'll
fix that at some point. Along these same lines, it might be nice for the
P3PService to cache its last result, and just use that if the URI's are the same
- or at least, the channels).

Anyway, we should be able to remove the observer impl, since there are no other
consumers of nsICookieConsent in the tree (including commercial).

(Thinking about it, the reason for the observing may have been that cookies
didn't always have an nsIHttpChannel to pass in - however, darin recently fixed
that. So we should have a channel to give it, in all the cases where it matters.)

Patch to remove the observer impl coming up.

Comment 1

[[e-mail]]

Attachment: patch v1

removes nsIHttpNotify & nsIObserver impls, and associated methods. (p3p doesn't
need to listen to the p3p-enabled pref now, since cookies makes that decision
for it.)

this patch saves 4.4k codesize on linux (opt stripped).

Comment 2

[[e-mail]]

Comment on attachment 126361 [details] [diff] [review]
patch v1

seeking reviews for this cleanup.

Comment 3

[Darin Fisher]

hmm... what if a server response includes a P3P header without any Set-Cookie
headers?  is it okay for us to ignore such P3P headers?

Comment 4

[harishd]

>is it okay for us to ignore such P3P headers?
I don't think so. I'm sure there was a reason ( trying to recall ) why we
were're listening to headers. Dwitte, please make sure that you're not breaking
anything.

Comment 5

[[e-mail]]

nsICookieConsent appears to be completely isolated from the rest of the p3p impl
(which apparently parses an XML nsIDOMDocument to get the info, when you use the
"View Privacy Info" option for a given page. So it looks like we will ignore any
p3p headers not specified as metatags).

>>is it okay for us to ignore such P3P headers?
>I don't think so. I'm sure there was a reason ( trying to recall ) why we
>were're listening to headers. Dwitte, please make sure that you're not breaking
>anything.

I don't believe I'm breaking anything, but that's also an offhand assessment.
(Perhaps the reason it existed was because cookies didn't always have a channel
to hand to nsP3PService - but this has recently changed). Maybe peterv can
recall something?

Comment 6

[harishd]

>Maybe peterv can recall something?

I wrote that code and I should remember ;-). Let me think about this a bit more
and will update the bug with the information.

Comment 7

[harishd]

Comment on attachment 126361 [details] [diff] [review]
patch v1

If the cookies code always hands over a channel to nsP3PService then things
should be fine ( AFAICT ).

r=harishd

Comment 8

[Darin Fisher]

Comment on attachment 126361 [details] [diff] [review]
patch v1

sr=darin

Comment 9

[[e-mail]]

Checked in. Thanks for the reviews!

Actually, it gets even better. Cookies does (and always has) called up the
p3pservice _only_ if its nsIHttpChannel is non-null - which was easily possible
in the past (before darin fixed up the consumers some). So even if the
p3pservice had a useful p3p header via OnExamineResponse(), cookies would bail
just because it didn't have a channel.

So I completely fail to see why we ever needed this, actually. <shrug> :)