191839 - Content Policy API (nsIContentPolicy) sucks rocks

Status: RESOLVED FIXED

(Core :: Graphics: Image Blocking, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Let me explain.  ;)  Currently, ShouldLoad() takes an nsISupports* "context". 
This is only used by the image content policy that I can tell -- it QIs this to
nsIContent, gets its nsIDocument (incorrectly), gets the base URI from the document.

So what it _really_ wants to take is an nsIURI or worst-case an nsIDocument...
or something.

Would you mind thinking about this a bit?  I know you have related bugs already,
which is why I'm imposing on you; if you don't want to deal with this just let
me know and I will try to find someone else (or do it myself).

Comment 1

[Tim]

Looking at the callers, everyone sends an element (except docshell, which would
be nice to change to pass the document/subdocument element for consistency). 
For this reason, I'd like to change the context argument to the element in
question (nsIContent*, nsIHTMLDOMElement* (sp), or something similar).  It would
be nice to add the originating URI as well (possibly null in the case of
::DOCUMENT, or perhaps the reffer URI if that's available).  I'll look through
the users some more to see how feasible this is.  I'd also like feedback (better
ideas, situations my ideas fail to cover or make awkward to cover) in the mean
time, since it will be a short while until I have access to a dev environment again.

If you know someone using this API now (that isn't in the main tree--which is
all I've searched right now) or about to use it, possibly CC them.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Right now callers pass in an element because that's what the implementation expects.

What we should do is figure out what's really needed as the "context", change
the API to take that, and fix callers as needed.

Comment 3

[Tim]

I should have clarified before I submitted... I meant that I agree with passing
the element on a conceptual level, since various properties of the element may
play a role in whether it is accepted (random examples: height and width
properties of an image, declared content type of an EMBED or OBJECT element,
etc.).  I am continuing to investigate the callees.

Oh, and s/reffer/referrer/ in my previous comment (oops =).

Comment 4

[Boris Zbarsky [:bzbarsky]]

Ah, OK.  Sure, that makes sense.

In that case, make it an nsIContent, please (since things linking to
stylesheets, eg, may not be elements).

Comment 5

[Axel Hecht]

bz asked the XSLT folks to check content policy for XSLT stylesheet loads, so
from the point of the potential caller, we don't have nsIContent contexts.
XSLT stylesheets are compiled into a non-XPCOM datastructure, thus any context of
ours is useless to this. We could provide a nsIURI, though. A 
nsIInterfaceRequestor would be quite a burdon on the caller, though pretty 
flexible.

Comment 6

[Tim]

I don't know what ContentPolicy would do about an XSLT stylesheet except judge
it by URI prior to its being loaded and/or executed.  Probably I'm not thinking
creatively enough.

Looking back, my thoughts are as follows:
A) We want a URI (canonical or easy to make canonical) to the document
originating the request. (this would be optional, mainly in the case of initial
requests)

B) We want a URI to the content to be downloaded (preferably canonical; if not
canonical, enough information needs to be available to make it canonical).  This
will allow things like host-based and path-based policies.

C) We want a content node (in the document referred to by A), where possible,
which is causing the request.

D) In the "would be nice" category, an optional best-guess at the target's MIME
type (useful in OBJECT, where it may be specified).

I agree that an interface requestor would be the most flexibility, but I can't
think of a specific case where it would be useful to someone (it doesn't help
XSLT, since it can only get XPCOM interfaces).  If nobody ends up needing it, it
would be fairly wasteful of effort.  If someone can come up with a detailed (but
not necessarily currently possible) situation where an nsIContent would not be
suitable, please speak up.

Comment 7

[Boris Zbarsky [:bzbarsky]]

There is no such thing as a non-canonical URI when using nsIURI object -- nsIURI
implementations canonicalize the URI.

The rest of what you say makes sense to me.

There is also the question of how ShouldLoad and ShouldProcess should differ, if
at all.

Comment 8

[Tim]

Dwitte and I are discussing hooking cookies into content policy to reduce the
rather redundant cookie-specific permission code.

My main thought is to add an nsISupports at the end of the arguments (because
content policy's primary customer--content blockers--deserves a nsIContent =).

Also, cookie wants to do what image-prefs used to do (potentially show a
confirmation dialog), which makes me feel icky: once you get a few prompters in
the content policy chain, user experience goes down the tubes due to lots of popups.

Ignoring all my rambling about cookie, we want to be prepared for there to be
potentially many vetoers in the content policy chain.  Dwitte had a good
suggestion that we're currently investigating: let each vetoer have a content
type mask (a mask of the constants currently in nsIContentPolicy--the values can
be messed around with to facilitate this). Since CategoryManager doesn't deserve
to be bothered with this kind of change, I figure a readonly attribute
"applicableTypeMask" would be in order.  A registered vetoer would only be
called if the particular type being queried was in the vetoer's mask.

I'm going to spin the popup issue off into another bug (blocking this one)
because it warrants some thought.  It does need to be fixed because it would
allow prompting vetoers to prompt even if a later entity in the chain gave a
negative response.

Comment 9

[[e-mail]]

> Dwitte and I are discussing hooking cookies into content policy to reduce the
> rather redundant cookie-specific permission code.

I wouldn't call it redundant - it does what needs to be done. the reason I think
conpol would be conceptually nicer, is that it's not possible to "add"
functionality into the nsICookiePermission implementation without replacing it,
so it's not extension-friendly. it'd be nice if folks could make .xpi extensions
that just plug in a new cookie conpol module and block cookies that taste like
carrots and have three z's in their name.

of course all this comes at a cost - lots of headaches :)

the rest I agree completely with. I think they are general problems that would
be fun to solve anyway - conpol "probably should" be able to handle these
things. so it's not like all this pain is just for cookies.

Comment 10

[Tim]

Attachment: Snapshot of nsIContentPolicy thoughts

Okay, here is my current understanding of where we stand (I left the two
conflicting possibilities for prompting in with explanatory documentation; see
bug 208867 for where that will be hashed out). I haven't spent many cycles
thinking about what Process could use differently from Load, and I'd like
suggestions.

If there are some n.p.m newsgroups that would like a say in this sort of thing,
bring them into the circle =).

I tossed in a COOKIE constant, though that could just as easily fall under
OTHER.	Also, is there a reason to use PRInt32, or would moving to PRUint32 be
acceptable (not important, but just for clarity that sign is meaningless)?

Comment 11

[Tim]

Attachment: Same thing, no allowance for prompting

Dougt hints in bug 208801 that he doesn't want me to hold things up by
addressing the prompting thing, so I've outlawed it in the contract. Here is a
possible candidate (I'm not going to turn it into a far-less-readable patch
until the intermediate version has been smiled upon).  I still want some input
from an embedder's perspective and any suggestions on the needs of
shouldProcess.

Comment 12

[Tim]

Attachment: Results of more discussion

Discussion is continuing. The ability to prompt is back, but is still in flux
(as to how it will happen). I also fixed some misc problems I noticed I left in
earlier. XXX marks questions I have (since this is the first time I've hacked
IDL files).

I moved some parameters around so the required ones are together. Optional
parameters are clearly marked as such in the documentation.

Comment 13

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 125373 [details]
Results of more discussion

>  /**
>   * UNDER DISCUSSION
>   * A mask of the above content types which this implementation handles
>   */
>  readonly attribute PRInt32 applicableTypeMask;

Will this be called at every load, or only on init? Init would ofcourse be
faster, but that means you can't add other types when running. I think that
should be documented.

>  const PRIntn REJECT = 0;
>  const PRIntn ACCEPT = 1;
>  const PRIntn PROMPT = 2;

Which one will 'win'? If one user of conpol says to reject, will others still
be called? And for accept?
Currently, you can veto the load to not happen. I think that makes some sense.
It is about blocking after all.

An additional return param, something like TYPE_DISABLED would help fixing bug
201885. This value can be returned when images are disabled, so that the
alt-text can be displayed. You only want this when all images are disabled, not
when you blocked one image because it is an add or something.

Comment 14

[Tim]

> >  /**
> >   * UNDER DISCUSSION
> >   * A mask of the above content types which this implementation handles
> >   */
> >  readonly attribute PRInt32 applicableTypeMask;
> 
> Will this be called at every load, or only on init? Init would ofcourse be
> faster, but that means you can't add other types when running. I think that
> should be documented.

Right. I will document it once we decide.  The reason is that if we query it
every call, then there is no gain, since the way things are now, the shouldXXX
function filters on type (with a switch).  If we query it every call, we still
get the function call overhead we originally get.  I suggest we cache it on init
and document that vetoers who want to change at runtime should just set the mask
to ~0.

> >  const PRIntn REJECT = 0;
> >  const PRIntn ACCEPT = 1;
> >  const PRIntn PROMPT = 2;

> Which one will 'win'? If one user of conpol says to reject, will others still
> be called? And for accept?
> Currently, you can veto the load to not happen. I think that makes some sense.
> It is about blocking after all.

I tried to word the @return statements to indicate this, since it may depend on
the function (I'll probably move them to the definitions themselves since there
are no methods that differ on that). ACCEPT merely means "not rejected", PROMPT
means "need more info", and REJECT means "reject".  The system is defined in
terms of requests and rejection (meaning that if a request is not rejected by
any vetoer, it is accepted).

> An additional return param, something like TYPE_DISABLED would help fixing
> bug 201885. This value can be returned when images are disabled, so that the
> alt-text can be displayed. You only want this when all images are disabled,
> not when you blocked one image because it is an add or something.

I think that's beyond the scope of ContentPolicy, but I'll agree that it
warrants some consideration. However, would that block the type for the
remainder of the run?  I don't think that's what is wanted (since it should be
possible to toggle it back and forth at runtime).

An easier way to accomplish the same thing would be to throw a separate image
listener in at the beginning that does nothing but return
gPrefBranch.getBoolPref("images.allow", &foo), foo ? ACCEPT : REJECT (I really
don't want to introduce call-order-altering priorities to the mix, but I will if
embedders suggest it's wanted).

The catch which made that last statement longer than it should have been is
important--there is currently no way for vetoers to control where they fall in
the list. If it is possible for the conpol manager to rearrange category
manager's category lists, I'd still want to wait on a yea or nay from dougt on
whether it's worth it (the only way it would affect nsIContentPolicy is by the
addition of a read-once-on-init attribute, priority. (I'm not going to detail
the semantics of priority until it's decided we'd want it).

Comment 15

[Michiel van Leeuwen (email: mvl+moz@)]

In my suggestion TYPE_DISABLED would be the same as REJECT, but the caller now
knows that this type is disabled, and can display the alt text for example.
It doesn't need te be cached or anything, or be called as the first in the chain.

Comment 16

[Tim]

I understand where Michiel was coming from now, and we discussed his suggestion
some.  Since this is fairly specific for image blocking, I think it's better to
use a custom object and the |extra| parameter to pass values back to certain
parties querying content policy.

The specific request was to control the display (or not) of ALT text in place of
rejected images. I think that is too specific for nsIContentPolicy, which is why
it should be purely between the image loader and the image blocking vetoer via.
the extra parameter. If cookie wanted to do the same thing while still accepting
the cookie through the extra parameter, it can come up with its own way of
aggregating them both into the parameter (such as with an XPCOM array).

If anyone wants to suggest another situation-specific extension to
nsIContentPolicy, please demonstrate why it would be better that way than to use
the extra parameter.

Comment 17

[Tim]

Attachment: Minor fixes

Fix some copy-paste errors (promptLoad/promptProcess should not have a prompt
parameter) and a small thinko (promptFoo would require a parent window to
launch dialogs).

Also added random observation about modality in promptFoo (not a question to
reviewers, just an observation and a flag for future reference).

Feedback is appreciated.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Tim, the image loader is core gecko.  It MUST NOT have any dependencies on any
content policy implementations (since none of them are in the core).  Further,
if some embeddor creates a content policy for images, they MUST NOT need to know
implementation details of the core image loader to get proper alt text display.
 It should be obvious to them from the content policy API what they should be
returning in which case.

I don't think that giving the caller a hint as to _why_ the load is blocked is
"too specific for content policy".  This is in no way specific to images; the
same applies to all inline objects in web pages... (I still don't see why we
enforce this separation between images and, eg, flash objects....)

Comment 19

[Boris Zbarsky [:bzbarsky]]

In fact, the only content policy caller I can think of offhand that's not in
core gecko is XSLT... and no core gecko caller can make use of the "extra" param
in a useful way.

Other than that, I have nit comments: bitfields need to be unsigned, and the
type constants need a lot more documentation (what is a CONTROL_TAG?).

Comment 20

[Tim]

I'm hoping to carry on the discussion of revealing details about content policy
decisions in bug 201885.

Comment 21

[Tim]

Okay. Discussion will continue in here.

If we're going to officially support a mechanism for giving the reason, what
kind of distinctions are we going to want to make?  Image blocking wants to know
whether blocking was due to a type-wide block versus a specific decision made
about the image in question.  I'm not sure what other types would care whether
they were globally or specifically blocked.

But ignoring that, let's forge ahead and assert that we want to know why some
content was rejected.  What do we use for the possible rejection reasons? 
Should it be extensible, or should it be restricted to a fixed set of responses?

Comment 22

[Tim]

I'm thinking it's probably simpler all around if we use the return value to
communicate the rejection reason (mvl's suggestion earlier). I'm thinking we can
have REJECT_REQUEST (for indicating the specific request was rejected on its own
merits), REJECT_TYPE (for indicating the request was rejected based only on its
type).

I'm considering adding REJECT_OTHER for allowing external entities to indicate
to their own handlers when details are provided by the extra parameter
(presumably from their own nsIContentPolicy implementations). Core gecko
handlers will treat REJECT_OTHER the same as REJECT_REQUEST.

Are there other rejection situations we want to be identifiable to core gecko
(or that make sense)? Also feel free to comment on the names =).

My next task will be to take inventory of the types content policy currently
handles and to see if it could be improved (in addition to documenting them).

Comment 23

[Boris Zbarsky [:bzbarsky]]

REJECT_REQUEST/TYPE/OTHER sounds good... Maybe we could add REJECT_SERVER if the
target server is explicitly prohibited from loading the type being asked about?

To facilitate ease of return-value checking, maybe we could make all REJECT_*
types have some sign (eg negative), make ACCEPT have the opposite sign, and make
PROMPT be 0?  Not sure how good an idea this is....

Comment 24

[Tim]

Just documenting, for personal use, current occurrences of content policy.  Note
I'll be gone for most of tomorrow--not that it means anything.

Callers:
http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsObjectFrame.cpp#1313
http://lxr.mozilla.org/seamonkey/source/layout/base/src/nsPresContext.cpp#1400
http://lxr.mozilla.org/seamonkey/source/content/html/style/src/nsCSSLoader.cpp#957
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsImageLoadingContent.cpp#545
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsScriptLoader.cpp#431
http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDocShell.cpp#4950
http://lxr.mozilla.org/seamonkey/source/content/base/public/nsContentPolicyUtils.h
http://lxr.mozilla.org/seamonkey/source/xpfe/global/resources/content/bindings/tabbrowser.xml#572

Implementations:
http://lxr.mozilla.org/seamonkey/source/content/base/src/nsContentPolicy.cpp
http://lxr.mozilla.org/seamonkey/source/embedding/browser/webBrowser/nsWebBrowserContentPolicy.cpp
http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsObjectFrame.cpp

Comment 25

[Tim]

CCing people from bug 69453 and bug 81260 who might care about this. Sorry if I
get someone who doesn't care or miss someone who does care.

If only internal content policy implementations care about the window (or if
anyone is going to end up going through docshell or nsIDocument--i.e., unfrozen
interfaces), they can QI the DOMNode's ownerDocument. I will be "strongly
recommending" that all callers of content policy include something for the
nsIDOMNode parameter if at all possible (the nsIDOMDocument if nothing else is
relevant).

Per-window image loading preferences can be implemented in
nsWebBrowserContentPolicy given the arguments currently offered.

Unrelated comment: I'm considering ripping out RAW_URL (use OTHER) and changing
CONTROL_TAG to REFRESH (since it only seems to apply to meta-refresh tags,
according to the one #if-0-ed out use of it).

Note that REFRESH (which I intend to make DocShell query from ::RefreshURI) will
also apply to Refresh HTTP headers, since that makes sense to me from a policy
point of view.

Comment 26

[Tim]

Attachment: More updates

Some comment updates, added the various REJECT classifications.
I converted flags to Uint32s (should they perhaps be Uint16s?).
I documented all the types: I may have been too specific or too vague, or
missed some important cases either way.
I tweaked some of the method comments, too. Changes all around, and XXXs too.

I also added a miscellaneous note at the end that deserves to go somewhere.

A random thought about one intended use of the old nsIDOMWindow parameter:
image loading makes sense to be a per-tab thing if so desired (a comment in one
of the bugs I read wanted the parameter to allow a per-window image loading
setting). Forcing implementors to make the tab-or-window choice themselves
allows the interface to provide for either mode.

Comment 27

[Benjamin Smedberg]

SirL asked me to comment here regarding I wish that I had: that content policy
providers could redirect a "denied" URI to something else. For example, a
child-content-filtering mechanism would want not just to "deny" a request, but
put up a chrome page saying "this website was blocked, do such-and-such to
unblock it".

To accomplish this, I would add an "out nsIURI newURI"; to the methods in
question, which would only be used in the case of a "false/do not process" return.

--BDS

Comment 28

[Darin Fisher]

hmm... would other content policy implementations have an opportunity to block
that replaced URL?  i was also interested in using content policy to change the
URL being loaded, but i worry about the security implications of changing the
origin of the page out from under the hood.  what if some other module thinks
we're loading a page from site X, but really we're loading it from site Y?  or
would you restrict this redirection to chrome:// URLs only?

Comment 29

[Tim]

Attachment: Another round of changes

Minor changes. Mostly comment tweaks.  Currently playing around with what
constants we need to provide.  Feedback welcome.

Oh, it was more or less decided that I'm not going to handle URL redirection
unless it gets some serious defense.

Comment 30

[Tim]

It is decided that cookies are too much of an edge case and don't quite fit the
content policy purpose. Cookies will move to something like nsICookiePolicy
which will closely resemble this interface. Thus, ignore mentions of cookies in
the IDL.

I'm working on implementing this to make sure it's feasible, and to work out
what I should and should not guarantee to interface users (I'd like to know
where an appropriate place would be for documentation specifically about our
callers should go--what they guarantee, etc.--so that callees of the core
implementation can find the details in one place).

Comment 31

[Tim]

If anyone has a suggestion of how to get at the frame/object/iframe element from
nsDocShell::InternalLoad, feel free to speak up...  I've found that I have
access to the frame's nsIScriptGlobalObject, which is implemented (in one case)
by nsGlobalWindowImpl, which in turn has GetFrameElementInternal(). Obviously,
this kind of path would be horribly evil/broken (assuming a certain impl), but I
haven't yet found another way to get access to that info without tweaking
interfaces or moving the content policy check.

If the question doesn't make it obvious, I'm new to the nsDocShell/friends scene =).

Comment 32

[Boris Zbarsky [:bzbarsky]]

You should be able to QI the nsIScriptGlobalObject to nsIDOMWindowInternal and
then use the .frameElement property on that, no?

Comment 33

[Axel Hecht]

Would it be possible to implement same-origin checks as a policy?
A single point of entry to "all the checks" you need would be such a good thing.
Or should the script security manager have entry points that include content
policy?

Comment 34

[Tim]

Attachment: Mostly final version

This is more or less where I feel it should be.

My main thought about letting this encompass security checking is that it would
require more data (through parameters) than is currently given.

Comment 35

[Johnny Stenback (:jst)]

Comment on attachment 127796 [details]
Mostly final version

inline const char *
nsCPResponseName(PRInt16 response)
...

inline const char *
nsCPContentTypeName(PRUint32 contentType)
...

Those function names make it look like they're classes and not functions. How
about naming them NS_CP...Name() in stead?

Comment 36

[Tim]

Attachment: First draft patch

A lot of what is here is what I intend to keep there. I intend to add a few
nsresult error values for content policy use. I also intend to hunt down a few
more people who need to call content policy.

I'm trying to get out a rough draft to receive any "obvious misunderstanding"
insights. There are probably debugging statements sitting in various places.
I've made a number of changes since I last ran this, so there may be flaws that
will be detected when I run it again (not tonight; I'm heading to bed), but I
do know it compiles (on linux).

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 128752 [details] [diff] [review]
First draft patch

Just commenting on the interface, since the goal is to finalize and freeze
that:

>+ * WARNING: do not issue user prompts on the event thread from shouldXYZ
>+ *   (this means AT ALL for mozilla consumers).

What exactly is "the event thread"?  Perhaps we should just say that content
policy calls may not block the caller (and list posing a modal dialog as an
example of something that blocks the caller)?

>+//XXX Does a new UUID need to be generated for changes to an unfrozen interface?

It's preferable, yes.

>+  /**
>+   * Indicates an executable script (such as JavaScript).
>+   *
>+   * shouldLoad will not get this.

Why not?  Is that really a limitation you want to encode into the API or just
an implementation detail?

>the
>+   * type attribute AS PROVIDED (meaning it is inherently tainted).

It's not any more tainted than anything else (see below).  I would say that
this is the "type of script as indicated by the markup" or something, maybe?

>+  /**
>+   * Indicates an image.
>+   *
>+   * shouldLoad will get this for IMG elements.
>+   *
>+   * shouldProcess will get this for OBJECT, EMBED, IFRAME, and FRAME
>+   * elements after their types are determined to be image (as well as for IMG
>+   * elements).

Again, these are implementation details, not something you should be exposing
in the interface, imo.

>+  /**
>+   * Indicates a stylesheet.
>+   *
>+   * shouldLoad will not get this.

Why not?  And as it happens, that's not what your nsCSSLoader code does...

>+  /**
>+   * Indicates a generic object. XXX reevaluate
...
>+   * shouldProcess will never get this.

You know what I want to say.  ;)

>+  /**
>+   * Indicates a document at the top-level (i.e., in a browser).
>+   *
>+   * shouldLoad will get this for page loads into tabs/windows.
>+   *
>+   * shouldProcess will get this for X/HTML documents in the same situation.
>+   */

And here.

>+  /**
>+   * Indicates a timed refresh.
>+   *
>+   * shouldLoad will never get this.

Ok, this actually makes sense.	Perhaps write a bit about how shouldLoad never
gets this, because the refresh itself is not a resource to load, it's something
to process.

The document being refreshed to will trigger ShouldLoad/ShouldProcess calls of
its own, of course, right?

>+   * Returned from shouldFoo if the load or process request is rejected 

"from shouldLoad or shouldProcess"; "shouldFoo" is not a good idea.  ;)

>+   * Returned from shouldFoo if the load/process is rejected based solely 

same.

>+   * Returned from shouldFoo if the load/process is rejected based on the server

same.

>+   * it is hosted on or requested from (contentLocation or requestOrigin).


What should a policy that says "reject all images from www.mozilla.org" return?
 REJECT_REQUEST?

Perhaps the descriptions should be fleshed out a bit more?

>+   * The note from REJECT_TYPE applies here.

Just copy the note.  There is no such thing as too much documentation in this
case.

>+   * ShouldLoad will be called BEFORE the content specified in the parameters
>+   * is loaded to determine whether it will be loaded at all.

Maybe "will be called before loading of the content at aContentLocation is
started to determine whether to start the load at all"?

In any case, make the params be named aWhatever; that's the convention.

>+   * @param contentType      the type of content being tested (from the above
>+   *                         constants)

"the type of content being tested.  This will be one of the TYPE_* constants."

And rename the constants to TYPE_whatever.

At least that makes things a lot more readable to me....

>+   * @param requestOrigin    OPTIONAL. the location of the document/etc. that
>+   *                         initiated this load request; can be null if
>+   *                         inapplicable

This needs to be much more specific.  What is document/etc?  Perhaps "the
location of the resource that initiated that load request"?

In the case of documents, specify that this is the actual URI of the document,
base settings and document.domain settings notwithstanding.  Or is it?

>+   * @param requestingContent OPTIONAL. the DOM node that initiated the
>+   *                          request; can be null if inapplicable

aRequestingNode, please.

>+   * @param mimeTypeGuess    OPTIONAL. a guess for the requested content's
>+   *                         MIME type, based on information available to
>+   *                         the request initiator (the type attribute of an
>+   *                         OBJECT element, for example); very likely to be
>+   *                         inaccurate. TREAT AS TAINTED

Just take out the "TREAT AS TAINTED" part.

>+   *                         pass extra data to callees. Internal Mozilla
>+   *                         consumers may not make use of this parameter.

What does that mean?  Are internal Mozilla consumers things like gecko, or
things like the image permission manager?  Methinks you just shouldn't say
anything about this parameter....

>   /**
>    * Should the contents of the element in question be processed?

Doesn't make much sense in the absense of an element... How about "Should the
resource be processed?"

>+   * ShouldProcess will be called when enough of the content specified in the
>+   * parameters has loaded to determine whether it will be processed/
>+   * displayed/etc.

How about "ShouldProcess will be called once all the information passed to it
has been determined about the resource, typically after part of the resource
has been loaded"?

>+   * @param contentLocation OPTIONAL; the location of the content being
>+   *             requested: MAY be a post-redirection URI (will be, where
>+   *             possible).

I'm not sure we need to explicitly state that the "post-redirection" thing...
but I guess it's ok.

>+   * @param mimeType the MIME type of the requested content (e.g., image/png)
>+   *                 WILL NOT BE TAINTED

This is just the MIME type returned by necko (or possibly imagelib)?  How is
that any less tainted than the MIME type in the markup?  Both come from the
same server....

How about "the MIME type of the resource, as reported by the networking
library, if available"?  Is the MIME type allowed to be empty when not relevant
(eg for TYPE_REFRESH)?

>+   * @see #shouldLoad

Why?

>+%{C++

If we're planning on freezing this interface, we should not be having %{C++
sections in it.  Move these to some header if really needed; imo the REJECTED
check can easily be val != nsIContentPolicy::ACCEPT, etc, and these macros are
not really needed.

>+///////////////////////
>+// Random comment (which may end up somewhere else--in a different file, 
or
>+// whatever

Imo this comment should just go away.  ;)

Is there any way this patch could be landed in stages (API change first, then
updating all sorts of callers to call it) so that the perf impact of the
separate parts could be tracked?

Comment 38

[Tim]

Attachment: Updated nsIContentPolicy.idl based on comment 37

I've addressed all comments except for the C++ blocks. I defend their
existence, because they are inherently tied to the interface (and will only
change when the interface itself changes--but the interface can't change, so
neither will the C++ code).

As for the NS_CP_REJECTED and NS_CP_ACCEPTED macros, I felt they were more
readable (and shorter to type ;) than blah [!=]= nsIContentPolicy::ACCEPTED
(and I stand behind keeping them there).

> Is there any way this patch could be landed in stages (API change first, then

> updating all sorts of callers to call it) so that the perf impact of the
> separate parts could be tracked?
I'm happy to do that. However, I'll be producing all-inclusive patches until I
think it has reached review-request quality.  After that, I'll break them up
into changing existing callers (and api) and adding new callers.  If it's
desired, I can try to write a version where new callers call the old API (to
guage the impact of that).

Comment 39

[Boris Zbarsky [:bzbarsky]]

Just having separate patches for updating existing callers and adding new
callers is fine.

As for the C++ bugs, this is a scriptable API.  If there is something that you
can't do with the API without those C++ blocks, that's not acceptable; if there
is nothing like that, they have no reason for existence.

If the API is hard to use without those blocks, then it should perhaps be made
easier in general.

More to the point, frozen interfaces should not have C++ blocks in the IDL.  Period.

Comment 40

[Mike Shaver (:shaver emeritus)]

If you want to provide C++ helpers, I think that's fine -- we do it for nsCOMPtr
and do_CreateInstance and other cases that are made awkward not by deficiency in
any specific XPCOM API, but rather by the verbosity and error-prone nature of
the C++ XPCOM binding in general.

For those cases, though, you should provide a C++ header file, like
"nsContentPolicyHelper.h" that can be included in addition to -- or in place of,
if it transitively includes nsIContentPolicy.h itself -- the generated interface
header.

(And while I don't think the original API "sucked rocks", per se, I do think
this looks a lot nicer.  I wish I'd had the time to polish it to this degree
originally, and saved people the grief.  At one point we wanted to try to use
the API for context-less network-level denials, which I'm glad to see is no
longer on the table.)

Comment 41

[Boris Zbarsky [:bzbarsky]]

Well, perhaps "sucks rocks" was a bit of an exaggeration; I just really didn't
like the aContext thing.  ;)

Comment 42

[Tim]

Attachment: Most recent modifications

This is sort-of a supplement to the other patch (probably with some overlap). I
just diffed the most recent changes I've made, since they're new and warrant
looking over (they're in files I have very little experience in).

An example of probably wrong behavior I just noticed from skimming my patch is
that I should not return failure from nsBulletFrame::Init, but should instead
just prevent the image from loading.

I also made a couple of manual modifications to the patch itself, so pieces
probably won't apply. It's not meant to apply. (I have verified that my tree
compiles and runs before posting this patch.)

Comment 43

[Tim]

Attachment: Current status

I will be away for at least one week and up to an indefinite amount of time
(away from the Internet), so here's a snapshot of my current progress (still
incomplete).

Comment 44

[Tim]

Attachment: for review - base changes and updates to existing callers

This is a review copy (diff -w) of my hopefully-final set of changes.

Comment 45

[Tim]

Attachment: for review - new callers

This is a review copy (diff -w) of my hopefully-final set of changes

Comment 46

[Tim]

Attachment: for review - new callers (the real one)

This is a review copy (diff -w) of my hopefully-final set of changes.
Sorry, hit submit before changing the file name previously.

Comment 47

[Tim]

Attachment: for application - base changes and changes to existing callers

Same as before, this time in a form that can be applied.

Comment 48

[Tim]

Attachment: for application - new callers

.

Comment 49

[Tim]

Comment on attachment 133624 [details] [diff] [review]
for review - base changes and updates to existing callers

I'll outline and cc a list of requested reviewers in a moment.

Comment 50

[Tim]

My requests:
numbers are <# in base>/<# in new callers>

bz - sr

dougt - new content policy interface (5/0 changes: rewritten IDL, c++ helper
file, new nsresults) - bz and/or jst may be interested in this, too

adam lock - embedding/.../nsWebBrowserContentPolicy.cpp (1/0 change)

bryner or dbaron - layout/base, layout/html, layout/xul (4/15 changes)

jst or bz - content/base (13/0 changes)

dbaron - CSS Loader (11/0 changes)

dwitte or mvl - image manager (4/0 changes)

jst or heikki - XMLHttpRequest (0/5 changes)

neil or jag - xpfe - tabbrowser.xml (2/0 changes)

I am hoping to get the core changes ("base") in before the code freeze on
Tuesday, or failing that, in before beta if possible.

The "new caller" changes are less urgent, but they are responsible for fixing
some of the bugs blocked by this bug.

Comment 51

[Tim]

oops, I messed up--dbaron doesn't own cssloader;

bz or sicking or peterv - cssloader (see above for numbers)

Comment 52

[Tim]

Revised list, after having this explained better to me:

bz/dbaron - sr
bz/dbaron - content/base, cssloader, layout/base, layout/html, layout/xul

dougt - nsIContentPolicy and helpers, nsWebBrowserContentPolicy,
nsWebBrowserContentPolicy

dwitte - image manager
jst/heikki - XMLHttpRequest
neil - tabbrowser.xml

Un-CCing the other people I needlessly CCed.

Comment 53

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 133626 [details] [diff] [review]
for review - new callers (the real one)

>+// vim: ft=cpp tw=78 sw=2 et ts=2

For consistency with uses already in the tree, if you add vim modelines, please
add them as the second line of the file, between the emacs modeline and the
license.  (This makes it easier to check that the two agree.)

Comment 54

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(And other than that, I'm probably not a very good reviewer for this.)

Comment 55

[Boris Zbarsky [:bzbarsky]]

My apologies for taking so long on this... :(  I will try to make sure I have
the review done well before 1.7a opens.

Comment 56

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 133624 [details] [diff] [review]
for review - base changes and updates to existing callers

>Index: content/base/public/nsContentErrors.h
>+/** Error codes for content policy blocking */
>+/** Success variations of content policy blocking */

Why do we need both, exactly?

>Index: content/base/public/nsContentPolicyUtils.h
>+// Offer convenient translations of constants -> const char*

Shouldn't these only be built if PR_LOGGING is enabled?

>+ * "<Unknown Type>" if an unknown content type value is given.
>+NS_CP_ContentTypeName(PRUint32 contentType)
>+    return "<Unknown Response>";

Code and comment should match up.

>+NS_CheckContentLoadPolicy( PRUint32          contentType,

Why the extra space after '('?	Get rid of that.

I'm not sure inlining all these nice long functions is such a great idea
(particularly NS_CP_GetDocShellFromDOMNode).

>+NS_CP_GetDocShellFromDOMNode(nsIDOMNode *aNode)
>+        rv = doc->GetScriptGlobalObject(getter_AddRefs(scriptGlobal));

That won't work anymore (api changed).

>Index: content/base/public/nsIContentPolicy.idl
>+  const PRUint32 TYPE_SCRIPT      = 2;

This is IDL. Please use "unsigned long".  Same for the other consts.

>+  const PRInt16 REJECT_REQUEST = -1;

This is IDL.  Please use "short".  Same for the other consts.

>+   * @param aMimeTypeGuess    OPTIONAL. a guess for the requested 
>+   *                          attribute, for example); very likely to be
>+   *                          inaccurate.

Perhaps, "Does not reliably reflect the actual MIME type of the requested
content" is better than "very likely to be inaccurate"?

>+   * @param aExtra            an OPTIONAL argument, pass-through for callers to
>+   *                          pass extra data to callees.

Add something here about what content policies may or may not implement this or
something?  It should be made crystal-clear that Gecko itself will never use
this, if that's the case....

>+  PRInt16 shouldLoad(in PRUint32    aContentType,

unsigned long

>+   * @param aExtra            an OPTIONAL argument, pass-through for 

Same as for shouldLoad.

>+  PRInt16 shouldProcess(in PRUint32    aContentType,

And here.

>Index: content/base/src/nsContentPolicy.cpp
>  * This constructor does far too much.  I wish there was a way to get
>  * an Init method called by the service manager after the factory

File a followup bug on this?  I'm pretty sure it's possible to do that....

> nsContentPolicy::nsContentPolicy()
> 	nsCOMPtr<nsISupportsCString> string = do_QueryInterface(item, &rv);
>-	if (NS_FAILED(rv))
>+        if (NS_FAILED(rv)) {

What's with the indentation here?  More of that in that method too.

>+#if defined(DEBUG_shaver) || defined(DEBUG_riceman)
> 	fprintf(stderr, "POLICY: loading %s\n", contractid.get());
> #endif

How about just using the NSPR logging for that?

> nsContentPolicy::~nsContentPolicy()
> {
>+    mPolicies.Clear();

Not needed.  ~nsCOMArray handles this.

>+#define WARN_IF_URI_UNINITIALIZED(uri,name)                         \

Why not condition this define on DEBUG, so you don't have to check DEBUG when
you use this define?

>+            NS_WARNING(name " is uninitialized, fix caller");       \

parens around "name"? 

>+nsContentPolicy::CheckPolicy(CPMethod          policyMethod,
>+        if (!policy) { //shouldn't happen
>+            NS_WARNING("Somehow a null policy got into the list");

Make that an NS_ERROR.

>Index: content/base/src/nsImageLoadingContent.cpp

Ditch the random "add a line of whitespace, remove a line of whitespace"
changes.

>+shouldProcess(nsImageLoadingContent *aContent, nsIDocument *aDoc,

ShouldProcess, please.

>+  if (aDoc && NS_FAILED(aDoc->GetDocumentURL(getter_AddRefs(originURI)))) 

This will no longer compile either...

>+  nsCOMPtr<nsIDOMNode> node( do_QueryInterface(aContent) );

Ditch the extra spaces.

>+  // Give Content Policy (ShouldProcess) a chance to veto the load

I would rather like it if this part were done separately from the API change;
then we can add additional callsites like this one one at a time, watching Tp
as we go.

>     oldObserver->mNext = nsnull;  // so we don't destroy them all
>+
>     delete oldObserver;

Don't do that.

>+  nsCOMPtr<nsIURI> docURI;
>+  nsresult rv = aDocument->GetDocumentURL(getter_AddRefs(docURI));

This won't compile either.

>Index: content/base/src/nsImageLoadingContent.h
>+                           PRBool evenIfSizeAvailable = PR_FALSE);

Let's not make that an optional arg, ok?  Just update the callers to pass the
necessary value.

>Index: content/base/src/nsScriptLoader.cpp
>+shouldProcess(nsIURI *scriptURI, nsIURI *docURI, nsIDOMNode *node)

ShouldProcess

>+  NS_NAMED_LITERAL_CSTRING(type, "application/x-javascript");

Why do you need a named string?  Just use NS_LITERAL_CSTRING in the call.

>+          // XXXtw - rework this so we ask ahead of time (before pageload);
>+          // otherwise, we can't easily render earlier noscripts

"Not Happening" (would require reparsing the document multiple times, which is
just not feasible).  I wouldn't even bother putting this comment in here... or
checking for this case.

>+      // we haven't revoked the load, be sure it goes through ShouldProcess

Shouldn't that happen once the script is actually loaded and about to be
compiled?  Again, I think this should be a separate patch.

>@@ -490,18 +534,29 @@ nsScriptLoader::ProcessScriptElement(nsI

Same for the changes to this function (separate patch).

>Index: content/html/style/src/nsCSSLoader.cpp
>+ * Asks content policy whether processing the (inline or transcluded) sheet
>+ * described by the arguments.

That's not a sentence, and please don't use "transcluded".  How about "asks
content policy whether to process the sheet described by the arguments"?

Again, this should be a separate patch.

>+  if (aLoadData->mOwningElement) {
>+    domNode = do_QueryInterface(aLoadData->mOwningElement);
>+  }

do_QI is null-safe.

>+  // the URI of the sheet that loaded us, or of the document itself
>+  nsCOMPtr<nsIURI> requestingURI;
>+  if (aLoadData->mParentData && aLoadData->mParentData->mURI) {
>+    // we are a child sheet: get the URL of our parent

We can be a child sheet even without aParentData, actually... this should
actually look at the parent sheet, if any, of the child if it wants to be
"correct".

>+                                    NS_LITERAL_CSTRING("text/css"), // mime type

That comment is really not neeeded, imo.

>+ * @param aNode the node owning the sheet.  This is the element or document
>+ *                owning the stylesheet (possibly indirectly, for child sheets)

>@@ -1642,19 +1698,27 @@ CSSLoaderImpl::LoadStyleLink(nsIContent*
>+  nsCOMPtr<nsIDOMNode> node;
>+  if (!aElement) {
>+    node = do_QueryInterface(mDocument);
>+  } else {
>+    node = do_QueryInterface(aElement);
>+  }

nsCOMPtr<nsIDOMNode> node(do_QueryInterface(aElement));
if (!node) {
  node = do_QueryInterface(mDocument);
}

>@@ -1718,19 +1782,41 @@ CSSLoaderImpl::LoadChildSheet(nsICSSStyl
>+  nsCOMPtr<nsIDOMStyleSheet> nextParentSheet( do_QueryInterface(aParentSheet) );

Skip the extra spaces.	Also, no need to do any of this if the parentSheet has
a null document, since there will not be a node then (eg for UA sheets).

>+    nsCOMPtr<nsIDOMStyleSheet> curSheet( nextParentSheet );

Why do you need curSheet?  Why not just use topSheet?

>+  } while(nextParentSheet);

Space before '(' please.

>+      owningNode = do_QueryInterface(mDocument, &rv);

You never check this rv, so why bother with it?

>+ * 6) Parsing of the sheet: ParseSheet(), where CheckProcessAllowed() gets a
>+ *    chance to kill the sheet (which prevents children from loading)

What's the point of that parenthetical comment?

>Index: docshell/base/nsDocShell.cpp
>+getDOMNodeFromScriptGlobal(nsIScriptGlobalObject *aScriptGlobal)

This should probably make it clearer that it's getting the frame node in which
the corresponding document is loaded....  In any case, name the function with a
starting capital letter.

>+            (void) CallQueryInterface(elt, &node);

No need for the "(void)" there.

>+    // Check with content policy (only for meta refreshes)

Why only for meta refreshes?  Why not for HTTP-based ones?

>+        NS_NAMED_LITERAL_CSTRING(mimeType, "");

Again, no need for a named literal...

There seem to be a lot of unrelated curly-brace changes in this patch.	Take
them out, please.

>Index: embedding/browser/webBrowser/nsWebBrowserContentPolicy.cpp
>+static inline nsresult
>+performPolicyCheck(PRUint32    contentType,

Why inline, again?  And why leading lowercase letter?  Change both, please.

>Index: extensions/cookie/nsImgManager.cpp
>+getRootDocShell(nsIDOMNode *node, nsIDocShell **result)

Leading uppercase please.

Assert that node is non-null up front.

>+  rv = docshellTreeItem->GetRootTreeItem(getter_AddRefs(rootItem));

Document why you're not using GetSameTypeRootTreeItem (you actually want the
app docshell).

Imo, this part of the code should really move out into a mailnews-specific
content policy impl anyway....

>Index: layout/base/src/nsPresContext.cpp
>-    nsImageLoader *newLoader = new nsImageLoader();

Um... why did you remove all the code that actually does an image load?

>Index: xpfe/global/resources/content/bindings/tabbrowser.xml
>+              if (contentPolicy.shouldLoad(IMAGE, uri, origURI, docNode, type,

Why docNode?  This should use the original node that triggered the load, no? 
That's event.target....  In any case, you don't need to explicitly QI to DOM
interfaces in JS -- classinfo handles it.

Please attach a minimal patch that just changes the API and existing consumers.
 Let's get that reviewed and in (it should be a little shorter than this one,
hopefully).  Once that's done, we can add new callers one at a time.

Comment 57

[Tim]

>>Index: content/base/public/nsContentErrors.h
>>+/** Error codes for content policy blocking */
>>+/** Success variations of content policy blocking */
>
>Why do we need both, exactly?
Because blocking content may not be exceptional behavior.

>>Index: content/base/public/nsContentPolicyUtils.h
>>+// Offer convenient translations of constants -> const char*
>
>Shouldn't these only be built if PR_LOGGING is enabled?
Makes sense.

>>+ * "<Unknown Type>" if an unknown content type value is given.
>>+NS_CP_ContentTypeName(PRUint32 contentType)
>>+    return "<Unknown Response>";
>
>Code and comment should match up.
Perhaps this is just my commenting style seeming odd: the leading /* does match
up, but my comment continuation lines are indented to match up *s (in javadoc
style).

>>+NS_CheckContentLoadPolicy( PRUint32          contentType,
>
>Why the extra space after '('?	Get rid of that.
Check.

>I'm not sure inlining all these nice long functions is such a great idea
>(particularly NS_CP_GetDocShellFromDOMNode).
It's not nice, but it was a stopgap measure (to avoid duplicating that code)
since I wasn't able to add a new translation unit and make it link properly (due
to inexperience).
I haven't checked, but it's also possible that the compiler wouldn't inline it
due to its complexity (in which case I'm simply using the linker as a crutch to
accomplish what I failed to).

>>+NS_CP_GetDocShellFromDOMNode(nsIDOMNode *aNode)
>>+        rv = doc->GetScriptGlobalObject(getter_AddRefs(scriptGlobal));
>
>That won't work anymore (api changed).
Will fix.

>>Index: content/base/public/nsIContentPolicy.idl
>>+  const PRUint32 TYPE_SCRIPT      = 2;
>
>This is IDL. Please use "unsigned long".  Same for the other consts.
Check.

>>+   * @param aMimeTypeGuess    OPTIONAL. a guess for the requested 
>>+   *                          attribute, for example); very likely to be
>>+   *                          inaccurate.
>
>Perhaps, "Does not reliably reflect the actual MIME type of the requested
>content" is better than "very likely to be inaccurate"?
Okay.

>>+   * @param aExtra            an OPTIONAL argument, pass-through for callers to
>>+   *                          pass extra data to callees.
>
>Add something here about what content policies may or may not implement this or
>something?  It should be made crystal-clear that Gecko itself will never use
>this, if that's the case....
Check.

>> nsContentPolicy::nsContentPolicy()
>> 	nsCOMPtr<nsISupportsCString> string = do_QueryInterface(item, &rv);
>>-	if (NS_FAILED(rv))
>>+        if (NS_FAILED(rv)) {
>
>What's with the indentation here?  More of that in that method too.
Note my use of -w in the diff.

>>+#if defined(DEBUG_shaver) || defined(DEBUG_riceman)
>> 	fprintf(stderr, "POLICY: loading %s\n", contractid.get());
>> #endif
>
>How about just using the NSPR logging for that?
Can do.

>> nsContentPolicy::~nsContentPolicy()
>> {
>>+    mPolicies.Clear();
>
>Not needed.  ~nsCOMArray handles this.
Check.

>>+#define WARN_IF_URI_UNINITIALIZED(uri,name)                         \
>
>Why not condition this define on DEBUG, so you don't have to check DEBUG when
>you use this define?
Makes sense.

>>+            NS_WARNING(name " is uninitialized, fix caller");       \
>
>parens around "name"? 
Oops.

>>+nsContentPolicy::CheckPolicy(CPMethod          policyMethod,
>>+        if (!policy) { //shouldn't happen
>>+            NS_WARNING("Somehow a null policy got into the list");
>
>Make that an NS_ERROR.
Okay. I forgot NS_ERROR was debug-only (and a non-crasher-unless-you-ask-nicely).

>>Index: content/base/src/nsImageLoadingContent.cpp
>
>Ditch the random "add a line of whitespace, remove a line of whitespace"
>changes.
Will do. They appear to have been unintentional (from code I added and
mass-removed).

>>+shouldProcess(nsImageLoadingContent *aContent, nsIDocument *aDoc,
>
>ShouldProcess, please.
Here and elsewhere, I was using a leading lowercase to visually reaffirm private
status.  I can ditch it if you still disagree.

>>+  // Give Content Policy (ShouldProcess) a chance to veto the load
>
>I would rather like it if this part were done separately from the API change;
>then we can add additional callsites like this one one at a time, watching Tp
>as we go.
Acknowledged.

>>Index: content/base/src/nsImageLoadingContent.h
>>+                           PRBool evenIfSizeAvailable = PR_FALSE);
>
>Let's not make that an optional arg, ok?  Just update the callers to pass the
>necessary value.
I did this to avoid nudging unrelated code, but will do.

>>+  NS_NAMED_LITERAL_CSTRING(type, "application/x-javascript");
>
>Why do you need a named string?  Just use NS_LITERAL_CSTRING in the call.
To be 80-column-friendly.

>>+          // XXXtw - rework this so we ask ahead of time (before pageload);
>>+          // otherwise, we can't easily render earlier noscripts
>
>"Not Happening" (would require reparsing the document multiple times, which is
>just not feasible).  I wouldn't even bother putting this comment in here... or
>checking for this case.
Acknowledged.

>>+      // we haven't revoked the load, be sure it goes through ShouldProcess
>
>Shouldn't that happen once the script is actually loaded and about to be
>compiled?  Again, I think this should be a separate patch.
I don't remember why I did this; it was probably due to me not knowing (or
looking up) where to hook into loading.

>>@@ -490,18 +534,29 @@ nsScriptLoader::ProcessScriptElement(nsI
>
>Same for the changes to this function (separate patch).
Okay.

>>Index: content/html/style/src/nsCSSLoader.cpp
>>+ * Asks content policy whether processing the (inline or transcluded) sheet
>>+ * described by the arguments.
>
>That's not a sentence, and please don't use "transcluded".  How about "asks
>content policy whether to process the sheet described by the arguments"?
Oops. Sounds good.

>Again, this should be a separate patch.
Okay.

>>+  // the URI of the sheet that loaded us, or of the document itself
>>+  nsCOMPtr<nsIURI> requestingURI;
>>+  if (aLoadData->mParentData && aLoadData->mParentData->mURI) {
>>+    // we are a child sheet: get the URL of our parent
>
>We can be a child sheet even without aParentData, actually... this should
>actually look at the parent sheet, if any, of the child if it wants to be
>"correct".
Okay.

>>+    nsCOMPtr<nsIDOMStyleSheet> curSheet( nextParentSheet );
>
>Why do you need curSheet?  Why not just use topSheet?
Oops. Agreed.

>>+      owningNode = do_QueryInterface(mDocument, &rv);
>
>You never check this rv, so why bother with it?
Looks like I took out whatever I almost did and left that artifact. Will remove.

>>+ * 6) Parsing of the sheet: ParseSheet(), where CheckProcessAllowed() gets a
>>+ *    chance to kill the sheet (which prevents children from loading)
>
>What's the point of that parenthetical comment?
Nothing, apparently. Removed.

>
>>Index: docshell/base/nsDocShell.cpp
>>+getDOMNodeFromScriptGlobal(nsIScriptGlobalObject *aScriptGlobal)
>
>This should probably make it clearer that it's getting the frame node in which
>the corresponding document is loaded....
Okay.

>  In any case, name the function with a
>starting capital letter.
This was the privacy indicator, as before.

>>+            (void) CallQueryInterface(elt, &node);
>
>No need for the "(void)" there.
I think I was trying to remind the reader that I was ignoring any error results,
but agreed.

>
>>+    // Check with content policy (only for meta refreshes)
>
>Why only for meta refreshes?  Why not for HTTP-based ones?
I didn't mean to imply an intent to ignore HTTP refreshes (but I seem to have
overlooked them).  I'll look to see how I distinguish HTTP refreshes from UI
refreshes in that function (I may be misunderstanding who the actual callers of
that function are).

>>+        NS_NAMED_LITERAL_CSTRING(mimeType, "");
>
>Again, no need for a named literal...
Check.

>
>There seem to be a lot of unrelated curly-brace changes in this patch.	Take
>them out, please.
Those were at someone else's request (to make if bodies blocked in functions I
touched). I'll remove them.

>>Index: embedding/browser/webBrowser/nsWebBrowserContentPolicy.cpp
>>+static inline nsresult
>>+performPolicyCheck(PRUint32    contentType,
>
>Why inline, again?  And why leading lowercase letter?  Change both, please.
Inline because there were only two callers and it struck me as a potential
bottleneck; lower-case for the reason mentioned earlier. Will change.

>>+  rv = docshellTreeItem->GetRootTreeItem(getter_AddRefs(rootItem));
>
>Document why you're not using GetSameTypeRootTreeItem (you actually want the
>app docshell).
I didn't know about it. I'm not sure if I should use that or not, given this
introduction. I'll look into it.

>Imo, this part of the code should really move out into a mailnews-specific
>content policy impl anyway....
I'll leave it in for the sake of backwards-compat (and not having to add more
new files), though I'm open to moving it out.

>>Index: layout/base/src/nsPresContext.cpp
>>-    nsImageLoader *newLoader = new nsImageLoader();
>
>Um... why did you remove all the code that actually does an image load?
Oops.

>>Index: xpfe/global/resources/content/bindings/tabbrowser.xml
>>+              if (contentPolicy.shouldLoad(IMAGE, uri, origURI, docNode, type,
>
>Why docNode?  This should use the original node that triggered the load, no? 
>That's event.target....
Good question. I don't remember. I'll use event.target.

>  In any case, you don't need to explicitly QI to DOM
>interfaces in JS -- classinfo handles it.
Okay.

>Please attach a minimal patch that just changes the API and existing consumers.
> Let's get that reviewed and in (it should be a little shorter than this one,
>hopefully).  Once that's done, we can add new callers one at a time.
I will expect to get this done within the next week or so. Thanks for the review!

When I didn't quote a bit, I agreed with it.

Comment 58

[Boris Zbarsky [:bzbarsky]]

> Because blocking content may not be exceptional behavior.

In fact, never is.  There should just be a success code here, imo; anyone who
needs to turn it into an exception can....

> Perhaps this is just my commenting style seeming odd:

Actually I was saying that the stuff in "<>" should match up between comment and
code; it does not.

> It's not nice, but it was a stopgap measure (to avoid duplicating that code)

Why not make them static instead?  Or better yet, putting them in CPP file. 
Just create a whatever.cpp in that dir, add it to the Makefile in the obvious
place, and move them there; that's all you should need to do.  CVS diff won't
pick it up, but you can attach it to this bug in addition to the patch.

Definitely ditch the leading-lowercase stuff....

>>Why do you need a named string?  Just use NS_LITERAL_CSTRING in the call.
>To be 80-column-friendly.

Outdent some stuff instead or something... it's not worth creating a named
literal here, really.

> I didn't mean to imply an intent to ignore HTTP refreshes 

Actually, it looks like both HTTP and non-HTTP refreshes will have that arg set
to true.  In other words, that arg will never be false that I can see.

It should probably be eliminated; file a followup bug on that, ok?  cc me.

>>Document why you're not using GetSameTypeRootTreeItem (you actually want the
>>app docshell).
>I didn't know about it. I'm not sure if I should use that or not, given this
>introduction. I'll look into it.

Perhaps I was unclear.  ;)  You do want to use the function you are using, and
the reason you want to do that is that you want the app docshell -- the one that
loL for your window.  That's the comment you should add as to why that code is
NOT using GetSameTypeRootTreeItem.

>>Imo, this part of the code should really move out into a mailnews-specific
>>content policy impl anyway....
>I'll leave it in for the sake of backwards-compat

Sure.  I think it should move out, but not as part of this patch.

Sorry it took so long to get to this, by the way.  I'm still hopeful for getting
it all in by 1.7a or 1.7b at the latest.

Comment 59

[Tim]

Imo, it does make sense for blocked content to cause exceptions in some cases
(in loading callbacks, the image/document/subdocument was in fact not loaded).
I'd like the exceptions raised in these cases to be usefully fine-grained. I've
got at least one bug open complaining that some function is returning a generic
exception instead of a specific exception (because no applicable specific
exception exists yet; the reason for the bug was that a warning was being
emitted for cases that didn't warrant [that kind of] a warning, and spurious
warnings are distracting).

I'm willing to strive to hit 1.7a (21 January sounds achievable for me for all
changes, assuming I don't end up causing myself lots of review round-trips).

Comment 60

[Boris Zbarsky [:bzbarsky]]

Then we should eliminate the success codes and have callers who care check for
the corresponding error codes explicitly and not treat them as exceptions...

Do we even use the success codes anywhere?  If so, why?  Wouldn't it be better
to communicate a decision via an out param?

Basically, right now the choice of whether an exception is raised or not is
somehow semi-arbitrary and it's not clear who makes that decision....

Comment 61

[[e-mail]]

Comment on attachment 133627 [details] [diff] [review]
for application - base changes and changes to existing callers

>             if (tabBrowser.mTabbedMode) {
>               // We need to update a tab.
>               for (i = 0; i < this.childNodes.length; i++) {
>-                if (this.childNodes[i].contentDocument == event.target.ownerDocument) {
>-                  if (!contentPolicy.shouldLoad(Components.interfaces.nsIContentPolicy.IMAGE,
>-                                                uri, event.target, this.childNodes[i].contentWindow))
>-                    return;
>-
>+                if (this.childNodes[i].contentDocument == targetDoc) {
>                   var listener = tabBrowser.mTabListeners[i];
>                   listener.mIcon = href;
>                   break;
>                 }
>               }
>-
>               notifyListeners = (this.childNodes[i] == tabBrowser.mCurrentBrowser);
>             }
>-            else if (!contentPolicy.shouldLoad(Components.interfaces.nsIContentPolicy.IMAGE,
>-                                               uri, event.target, tabBrowser.mCurrentBrowser.contentWindow))
>-              return;
> 
>             if (notifyListeners && tabBrowser.mProgressListeners) {
>               for (i = 0; i < tabBrowser.mProgressListeners.length; i++) {
>                 var p = tabBrowser.mProgressListeners[i];
>                 if (p)
>                   p.onLinkIconAvailable(href);
>               }
>             }
>           ]]>

While writing a patch to bug 225854 (to which I have forgotton to request
reviews, oops) I noticed that until you activate tabbed mode the tabbrowser
accepts link icons from iframes, a problem which your patch exacerbates.

Comment 62

[Michiel van Leeuwen (email: mvl+moz@)]

Comment on attachment 133627 [details] [diff] [review]
for application - base changes and changes to existing callers

>Index: extensions/cookie/nsImgManager.cpp

>-  if (!aContentLoc || !aContext)
>+  if (!aContentLocation || !aRequestingNode) {
>     return rv;
>+  }

Isn't the use of braces in case like this a per-module style thing? You add
them in a few more places around in this file, making the patch harder to read.
I didn't read other files btw, so can't talk about that.

Comment 63

[Brendan Eich [:brendan]]

> Isn't the use of braces in case like this a per-module style thing?

Indeed, so (patch submitters) please don't add 'em to files that don't use such
a style.

/be

Comment 64

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

+  if (NS_SUCCEEDED(rv) && NS_CP_REJECTED(decision)) {
+    return NS_ERROR_CONTENT_BLOCKED;
+  }

This seems a bit dangerous. We don't want to give access if something unexected
like OOM or uninitialized variable or so happens, right? Wouldn't the test be
better written as |NS_FAILED(rv) || NS_CP_REJECTED(decision)|?

Comment 65

[Boris Zbarsky [:bzbarsky]]

Tim, what's the state here?  A bunch of work is being held off pending
resolution of this bug....  If we could get a minimal change in that just makes
the API changes, most of this work could move forward....

Comment 66

[Tim]

I will once again have some free time in a week or so; I'll get this done then
(and hopefully be more active again, in general).

Comment 67

[Tim]

Attachment: patch v1.5

(made-up version number)
I think I have addressed all review comments (except for the
exception/non-exception status; I still think having both is the best solution
here).	One area I'd like assistance (or careful checking) in is the
tabbrowser.xml changes, since that is an area where I am shaky (especially in
the security aspect).

I have not yet fixed the problem of some utility methods existing in
nsIContentPolicyUtils.h which should be in a .cpp file, but that should be a
relatively minor thing to fix (which I didn't want to hold this up for).

New stuff in this patch:
tabbrowser.xml in FireFox (toolkit/) is patched similarly to SeaMonkey's.
SeaMonkey's tabbrowser.xml gained FireFox's fix for neil's observation in
comment 61.

I took out gratuitous style changes where they were obvious (i.e., where I
noticed them).	I took out all changes that were purely additive (e.g., new
callsites).

I left in at least one dump() in the js because I was/am having problems in
that area, but pretend it's gone. =)

Unless someone requests it, I will avoid posting the "full" patch (which
includes things like the new callsites) until the basic/api patch lands (for
the sake of reducing bugspam).

I have not yet tested the FireFox-only changes (toolkit/.../tabbrowser.xml
changes only).

Comment 68

[Tim]

Attachment: patch v1.5.1

sigh.  I managed to attach the wrong diff; here is the real one.

Oh, and so this gets a proper reply:
RE: comment 64, I agree, though I opted to pass along original exceptions
instead of content policy exceptions in the case of exceptional failure.

Comment 69

[Brendan Eich [:brendan]]

Need firefox test coverage, natch.

/be

Comment 70

[Boris Zbarsky [:bzbarsky]]

I'll hopefully take a look this coming Saturday

Comment 71

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 143252 [details] [diff] [review]
patch v1.5.1

>Index: content/base/public/nsContentPolicyUtils.h

>+NS_CP_GetDocShellFromDOMNode(nsIDOMNode *aNode)

>+    nsIScriptGlobalObject *scriptGlobal;

You should init this to nsnull.

>+        rv = aNode->GetOwnerDocument(getter_AddRefs(ownerDoc));
>+        // and turn it into an nsIDocument
>+        if (NS_SUCCEEDED(rv) && ownerDoc) {

No need to store or check rv here.  Just use ownerDoc.	In fact, no need to
check anything, since do_QI is null-safe.  Just GetOwnerDocument and do_QI

>+    if (NS_FAILED(rv) || !scriptGlobal) {
>+        return nsnull;
>+    }

Just check scriptGlobal here (since you inited it to null up front, that's ok).
 rv can be removed from this function altogether.

>Index: content/base/public/nsIContentPolicy.idl

>+   * @param aMimeTypeGuess    OPTIONAL. a guess for the requested content's
....
>+   *                          the request initiator (e.g., an OBJECT's type
>+   *                          attribute, for example); does not reliably

Remove "for example"; it's redundant with "e.g."

>Index: content/base/src/nsContentPolicy.cpp

> +#define CP_ACCEPT      nsIContentPolicy::ACCEPT

It looks like you only use this in one place.  Why bother with the define?  All
it does is obscure what's really going on.

>Index: content/base/src/nsImageLoadingContent.cpp

The changes to this file need to be merged to the changes from bug 57607.  The
content policy access now happens in nsContentUtils::CanLoadImage.  Feel free
to change the signature of that method as convenient and to change the two
callers (nsImageLoadingContent and nsCSSValue::Image constructor)
accordingly...

I'll take a look at the merged version, ok?  

>Index: content/base/src/nsScriptLoader.cpp

>+      nsCOMPtr<nsIDOMNode> node(do_QueryInterface(aElement));

No need for that QI.  aElement is an nsIDOMHTMLScriptElement, which inherits
from nsIDOMNode.  So you can just pass aElement as an nsIDOMNode.

> +      if (NS_FAILED(rv) || NS_CP_REJECTED(shouldLoad)) {

This changes the logic, no?  The code used to have the equivalent of 
|if (NS_SUCCEEDED(rv) && NS_CP_REJECTED(shouldLoad))|.	Please put it back to
doing that (and remove the rv check in the nested if).

>Index: content/html/style/src/nsCSSLoader.cpp

>+ * @param aNode the node owning the sheet.  This is the element or document
>+ *                owning the stylesheet (possibly indirectly, for child 

Fix indent, please?  Line up "owning" with "the".

>@@ -1733,19 +1733,49 @@ CSSLoaderImpl::LoadChildSheet(nsICSSStyl

>+  nsCOMPtr<nsIStyleSheet> parentStyleSheet = do_QueryInterface(aParentSheet);

Again, no need to QI.  aParentSheet is an nsICSSStyleSheet, which inherits from
nsIStyleSheet.

>+    do {
>+      topSheet = nextParentSheet;

|topSheet.swap(nextParentSheet)| may be just a tiny little tad faster... (not
like it really matters given all the work we're doing here).

>+    rv = topSheet->GetOwnerNode(getter_AddRefs(owningNode));
>+    if (NS_FAILED(rv)) {
>+      owningNode = nsnull;

No need for that.  If the getter set the out param to garbage, the nsCOMPtr
will crash here anyway.  So you have to assume the getter is semi-sane.

>+  if (!owningNode && mDocument) {

do_QI is null-safe; no need for the mDocument check here.

>Index: docshell/base/nsDocShell.cpp

>+ * @return the DOM Document (as a DOMNode) framing aScriptGlobal, or nsnull on
>+ *    failure

That's not what the function is returning... It's returning the <frame> or
<iframe> element that contains the scriptglobal, not a DOM document.  Please
adjust the comment accordingly.

>+static inline already_AddRefed<nsIDOMNode>

>+            CallQueryInterface(elt, &node);

nsIDOMElement inherits from nsIDOMNode.  You could probably return a weak
nsIDOMNode pointer from this function and just return elt here.

>@@ -4893,63 +4916,84 @@ nsDocShell::InternalLoad(nsIURI * aURI,
>-    
>+

Ditch that whtiespace change, please.

>-        rv = aURI->SchemeIs("wyciwyg", &isWyciwyg);   
>-        if ((isWyciwyg && NS_SUCCEEDED(rv)) || NS_FAILED(rv)) 
>+        rv = aURI->SchemeIs("wyciwyg", &isWyciwyg);
>+        if ((isWyciwyg && NS_SUCCEEDED(rv)) || NS_FAILED(rv))

And that one.

>+    nsCOMPtr<nsIDOMNode> requestingNode =
>+        GetDOMNodeFromScriptGlobal(mScriptGlobal);

I think you should only do this in the IsFrame() branch below.	Otherwise the
main document will count as a subdocument, no?	After all, it's being loaded
from a XUL frame...  Note that the IsFrame() check handles this case by looking
at docshell types.

>+    } else if (NS_FAILED(rv)) {
>+        return rv;
>     }

Absolutely not.  See the "Only abort the load ..." comment above this code.
Content policy failure should not be fatal here.

>Index: embedding/browser/webBrowser/nsWebBrowserContentPolicy.cpp

>+#include "nsIDOMNode.h"
>+#include "nsIDOMDocument.h"
>+#include "nsIDocument.h"
>+#include "nsContentPolicyUtils.h"

I don't see you using nsIDocument or nsIDOMDocument anywhere in this code.  Are
you?  If not, please don't include the headers.

>+// Most of the checks will go through both ShouldLoad and ShouldProcess,
>+// so we perform our checks in both

I'm not sure what this comment means, and it seems superfluous....

>+nsWebBrowserContentPolicy::ShouldProcess(PRUint32          contentType,

>+    return NS_ERROR_NOT_IMPLEMENTED;

Just return NS_OK for now, ok?	Saves callers some grief...

>Index: extensions/cookie/nsImgManager.cpp

Again, this needs merging (this time to bug 236919). Please do that and I'll
take a look?

>Index: layout/base/src/nsPresContext.cpp

These changes need merging too.  The code being changed is gone (the image
loads now go through nsContentUtils).  You probably don't need changes to this
file anymore.

>Index: layout/html/base/src/nsObjectFrame.cpp
>+                                   nsDependentCString(aMimetype ? aMimetype
>+                                                                : ""),

Could you file a followup bug on plugins that aMimetype here should never be
null and cc me on it?  The code that passes in a null type looks wrong to me...
 Leave this as-is for now; the other bug will remove the null check.

>+    } else if (NS_FAILED(rv)) {
>+      return rv;

Content policy failure should not be fatal here.

>Index: toolkit/content/widgets/tabbrowser.xml
>+            var safeGetProperty = function(obj, propname) {

function safeGetProperty(obj, propname) {
  return Components.lookupMethod(obj, propname).call(obj);
}

without this var cruft should work, no?

>+              dump("/" + propname + "\n"); //XXX debug

Please take that out.

>+            var uri = ioService.newURI(href, docCharsest, baseURI);

You misspelled "docCharset"

>+            //XXXtw let the exception continue or turn it into a return?

An exception means that you're not allowed to do the load.  So you absoltely
want to return if there is an exception.  Please remove this comment.

>+            // Security says okay, now ask content policy
>+            var contentPolicy =
>+              Components.classes['@mozilla.org/layout/content-policy;1']
>+                        .getService();
>+            if (!contentPolicy)
>+              return; // Refuse to load if we can't do a security check.

I know that's what the code used to do.... it's totally bogus.	getService will
throw if it can't return the service.  So the !contentPolicy check is
pointless.  Do a try/catch around the getService() and return in the catch.

(!contentPolicy.shouldLoad(Components.interfaces.nsIContentPolicy.TYPE_IMAGE,
>+                                                uri, origURI, targetDoc,

Shouldn't you use event.target for the node?

Also, shouldLoad no longer returns a boolean.  You want to compare the return
value to Components.interfaces.nsIContentPolicy.ACCEPT, no?

>+            else if (this.contentDocument != safeGetProperty(event.originalTarget,
>+                                                             "ownerDocument") 

Why is this check being added?

>+                     !contentPolicy.shouldLoad(Components.interfaces.nsIContentPolicy.TYPE_IMAGE

Again, this no longer returns a boolean.

I don't see changes to xpfe/global/resources/content/bindings/tabbrowser.xml in
this diff.  Did you forget to attach those?

I also don't see changes to nsMsgContentPolicy.  Again, did you forget to
attach those?

Comment 72

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 143252 [details] [diff] [review]
patch v1.5.1

By the way, as far as other reviews go, I would say that neil taking a look at
the tabbrowser changes and dougt giving the new interface a quick skim should
be perfectly sufficient; I can r+sr the rest of it.

Comment 73

[Tim]

(In reply to comment #71)
> >Index: content/base/src/nsScriptLoader.cpp
> 
> > +      if (NS_FAILED(rv) || NS_CP_REJECTED(shouldLoad)) {
> 
> This changes the logic, no?  The code used to have the equivalent of 
> |if (NS_SUCCEEDED(rv) && NS_CP_REJECTED(shouldLoad))|.	Please put it back to
> doing that (and remove the rv check in the nested if).
See comment 64 (though I'll make your suggested change).

> 
> >Index: embedding/browser/webBrowser/nsWebBrowserContentPolicy.cpp
> 
> >+// Most of the checks will go through both ShouldLoad and ShouldProcess,
> >+// so we perform our checks in both
> 
> I'm not sure what this comment means, and it seems superfluous....
Oops; forgot to ditch that (it's appropriate in the full change).

> 
> >+nsWebBrowserContentPolicy::ShouldProcess(PRUint32          contentType,
> 
> >+    return NS_ERROR_NOT_IMPLEMENTED;
> 
> Just return NS_OK for now, ok?	Saves callers some grief...
Will do (and I'll set the out param, since I'm returning success).

> 
> >Index: toolkit/content/widgets/tabbrowser.xml
> >+            var safeGetProperty = function(obj, propname) {
> 
> function safeGetProperty(obj, propname) {
>   return Components.lookupMethod(obj, propname).call(obj);
> }
> 
> without this var cruft should work, no?
Can I do that in XBL without adding a new <whatever> element?

> 
> >+            var uri = ioService.newURI(href, docCharsest, baseURI);
> 
> You misspelled "docCharset"
oops

> 
> >+            //XXXtw let the exception continue or turn it into a return?
> 
> An exception means that you're not allowed to do the load.  So you absoltely
> want to return if there is an exception.  Please remove this comment.
The question meant whether I should simply return versus allowing an exception
to pass to the caller, not whether I should return or continue executing my
function.

> 
> (!contentPolicy.shouldLoad(Components.interfaces.nsIContentPolicy.TYPE_IMAGE,
> >+                                                uri, origURI, targetDoc,
> 
> Shouldn't you use event.target for the node?
probably =)

> 
> Also, shouldLoad no longer returns a boolean.  You want to compare the return
> value to Components.interfaces.nsIContentPolicy.ACCEPT, no?
oops; you are correct.

> 
> >+            else if (this.contentDocument !=
safeGetProperty(event.originalTarget,
> >+                                                             "ownerDocument") 
> 
> Why is this check being added?
see comment 61; I merged in that change from toolkit/, which seems like it fixes
the problem mentioned in comment 61.

> 
> >+                    
!contentPolicy.shouldLoad(Components.interfaces.nsIContentPolicy.TYPE_IMAGE
> 
> Again, this no longer returns a boolean.
> 
> I don't see changes to xpfe/global/resources/content/bindings/tabbrowser.xml in
> this diff.  Did you forget to attach those?
> 
> I also don't see changes to nsMsgContentPolicy.  Again, did you forget to
> attach those?
> 

Thanks for the review! (I'll try to get the changes done tonight or tomorrow night.)

Comment 74

[Boris Zbarsky [:bzbarsky]]

> See comment 64 (though I'll make your suggested change).

Ah, ok.  Yeah, if sicking prefers us to bail on failure we should do that.

> Can I do that in XBL without adding a new <whatever> element?

You should be able to.... if you can't, stick with what you have now. In either
case, test it to make sure it works.  Mail me offline if you need a testcase....

> The question meant whether I should simply return versus allowing an exception
> to pass to the caller

Ah, ok.  Just returning is fine.

> see comment 61;

Right.  This bug has gotten _way_ too long.   ;)

Comment 75

[[e-mail]]

(In reply to comment #73)
>(In reply to comment #71)
>>(From update of attachment 143252 [details] [diff] [review])
>>>Index: toolkit/content/widgets/tabbrowser.xml
>>>+            var safeGetProperty = function(obj, propname) {
>>function safeGetProperty(obj, propname) {
>>  return Components.lookupMethod(obj, propname).call(obj);
>>}
>> 
>>without this var cruft should work, no?
>Can I do that in XBL without adding a new <whatever> element?
function f(...) should be roughly equivalent to const f = function(...)

Comment 76

[Brendan Eich [:brendan]]

> function f(...) should be roughly equivalent to const f = function(...)

It's more like var, except the function declaration form (on the left; the form
on the right is a function expression) remembers its name, and it replaces a
property of the same name (in ECMA; SpiderMonkey has warnings and errors for
various cases such as attempting to redefine a const f via function f).  With
var f = ..., the pre-existing property named f will not be replaced, and if
read-only, will not be set to the initializer value.

/be

Comment 77

[[e-mail]]

Ah, I see what you mean... you can't do var closed = true; because closed only
has a getter, although you can do function closed() {}; closed = true;

Comment 78

[Tim]

I didn't make Sunday night =(; I will have some free time tomorrow evening and
Wednesday; my target: before Thursday

Comment 79

[Boris Zbarsky [:bzbarsky]]

Ping?  It would be great to have this done, reviewed, and ready to go when 1.7
branches and the tree reopens....

Comment 80

[Tim]

Pong.  I'll work on it after exams this week.

Comment 81

[Michiel van Leeuwen (email: mvl+moz@)]

I'm working on merging the patch with the current tree, and updating to review
comments. More soon. (famous last words...)

Comment 82

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: Patch v1.6

Updated patch to the current trunk and to as much review comments as i could.

Not done:

>>+static inline already_AddRefed<nsIDOMNode>
>>+	       CallQueryInterface(elt, &node);
> nsIDOMElement inherits from nsIDOMNode.  You could probably return a weak
> nsIDOMNode pointer from this function and just return elt here.
Because i don't know how to do it. 

> (!contentPolicy.shouldLoad(Components.interfaces.nsIContentPolicy.TYPE_IMAGE,

> >+						    uri, origURI, targetDoc,
> 
> Shouldn't you use event.target for the node?
Because i'm not sure how and why. targetDoc is the ownerDocument from
event.target. Why don't we want to use that? after all, it should be
aRequestingNode.

I hope I'm not missing anything

Comment 83

[[e-mail]]

Comment on attachment 147918 [details] [diff] [review]
Patch v1.6

>+static inline already_AddRefed<nsIDocShell>
>+GetRootDocShell(nsIDOMNode *node)
I was wondering how this is better than static void inline
GetRootDocShell(nsIDOMNode* aNode, nsIDocShell** aDocShell) but I'm not sure
this was worth breaking out, although at least it is slightly longer than
GetDOMNodeFromScriptGlobal below.

>+static inline already_AddRefed<nsIDOMNode>
>+GetDOMNodeFromScriptGlobal(nsIScriptGlobalObject *aScriptGlobal)
>+{
>+    nsCOMPtr<nsIDOMWindowInternal> intWin(do_QueryInterface(aScriptGlobal));
>+    nsIDOMNode *node = nsnull;
>+    if (intWin) {
>+        nsCOMPtr<nsIDOMElement> elt;
>+        nsresult rv = intWin->GetFrameElement(getter_AddRefs(elt));
>+        if (NS_SUCCEEDED(rv) && elt) {
>+            CallQueryInterface(elt, &node);
>+        }
>+    }
>+    return node;
>+}
I think you'd be better of manually inlining this as follows, instead of:

>+    nsCOMPtr<nsIDOMNode> requestingNode;
> 
>-    (void) NS_CheckContentLoadPolicy((IsFrame() ? nsIContentPolicy::SUBDOCUMENT
>-                                                : nsIContentPolicy::DOCUMENT),
>-                                     aURI,
>-                                     nsnull,
>-                                     domWindow,
>-                                     &bShouldLoad);
>-    if (!bShouldLoad) {
>-      // XXX: There must be a better return code...
>-      return NS_ERROR_FAILURE;
>+#ifdef DEBUG_riceman
>+    if(IsFrame() && !requestingNode) {
>+        NS_WARNING("A frame but no DOM node!?");
>+    }
>+#endif
>+
>+    PRInt16 shouldLoad = nsIContentPolicy::ACCEPT;
>+    PRUint32 contentType;
>+    if (IsFrame()) {
>+        requestingNode = GetDOMNodeFromScriptGlobal(mScriptGlobal);
>+        contentType = nsIContentPolicy::TYPE_SUBDOCUMENT;
>+    } else {
>+        contentType = nsIContentPolicy::TYPE_DOCUMENT;
>+    }
nsCOMPtr<nsIDOMElement> requestingElement;
PRInt16 shouldLoad = nsIContentPolicy::ACCEPT;
PRUint32 contentType;
if (IsFrame()) {
  nsCOMPtr<nsIDOMWindowInternal> intWin(do_QueryInterface(mScriptGlobal));
  if (intWin)
    intWin->GetFrameElement(getter_AddRefs(requestingElement));
#ifdef DEBUG_riceman
  NS_ASSERTION(requestingElement, "A frame but no DOM element!?");
#endif
  contentType = nsIContentPolicy::TYPE_SUBDOCUMENT;
} else {
  contentType = nsIContentPolicy::TYPE_DOCUMENT;
}

The nsIDOMElement will cast itself into an nsIDOMNode for the subsequent
NS_CheckContentLoadPolicy call.

Comment 84

[Boris Zbarsky [:bzbarsky]]

mvl, it looks like you left out the diff for nsImgManager.h... also, could you
diff the files in the same order as the last patch Tim posted?  That would make
it much easier to review the changes...

Comment 85

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: Patch v1.6.1

I didn't change nsImgManager.h, because all what changed in there was a vi mode
line.
Attaching a patch with that change back in. I hand-edited the patch to the
original order of the files. Not sure why cvs diff messed up, as i gave at
least the dirs in the original order. Why extension/cookie ended up as first, i
don't know.
This patch is otherwise the same. I didn't update anything yet.

Comment 86

[Boris Zbarsky [:bzbarsky]]

So do you want me to look at that as-is, or wait till you make the updates?

Comment 87

[Michiel van Leeuwen (email: mvl+moz@)]

The only things that needs changes is what neil said, and what i didn't know how
to do it. Just consider that fixed.

Comment 88

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 147999 [details] [diff] [review]
Patch v1.6.1

>Index: content/html/style/src/nsCSSLoader.cpp

>+  if (NS_SUCCEEDED(rv) && NS_CP_REJECTED(shouldLoad)) {

I've been convinced that this would be better as

  if (NS_FAILED(rv) || NS_CP_REJECTED(shouldLoad)) {

for safety's sake... make that change?

>Index: docshell/base/nsDocShell.cpp

>@@ -4954,30 +4977,49 @@ nsDocShell::InternalLoad(nsIURI * aURI,
>+#ifdef DEBUG_riceman
>+    if(IsFrame() && !requestingNode) {
>+        NS_WARNING("A frame but no DOM node!?");
>+    }
>+#endif

This doesn't make sense anymore, since you don't set requestingNode until
lower... this should live in the IsFrame() branch (and hence not need to check
IsFrame()).  Sorta like what Neil suggested.

>+    if (NS_SUCCEEDED(rv) && NS_CP_REJECTED(shouldLoad)) {

Again, if (NS_FAILED(rv) || NS_CP_REJECTED(shouldLoad)) {

>+        if (shouldLoad == nsIContentPolicy::REJECT_TYPE) {

And this should be 

  if (NS_SUCCEEDED(rv) && shouldLoad == nsIContentPolicy::REJECT_TYPE)

>Index: extensions/cookie/nsImgManager.cpp
>+NS_IMETHODIMP nsImgManager::ShouldProcess(PRUint32          aContentType,

>+   return NS_ERROR_NOT_IMPLEMENTED;

Fix indent, and return NS_OK please (after setting the out param to accept).

>Index: layout/html/base/src/nsObjectFrame.cpp

>+                                   nsDependentCString(aMimetype ? aMimetype : ""),

This needs a bug filed, right?

>+    if (NS_SUCCEEDED(rv) && NS_CP_REJECTED(shouldLoad)) {
>+      return NS_CONTENT_BLOCKED_SHOW_ALT;

Again, if NS_FAILED(rv) || NS_CP_REJECTED(shouldLoad).

>Index: toolkit/content/widgets/tabbrowser.xml

>+            const ioService =
>+              Components.classes["@mozilla.org/network/io-service;1"]
>+                        .getService();

getService(Components.interfaces.nsIIOService), right?

> (contentPolicy.shouldLoad(Components.interfaces.nsIContentPolicy.TYPE_IMAGE,
>+                                               uri, origURI, targetDoc

event.target for the node, like I said...

>+            else if (this.contentDocument != safeGetProperty(event.originalTarget,
>+                                                             "ownerDocument") ||

Again, why is this check being added?

>Index: xpfe/global/resources/content/bindings/tabbrowser.xml

Same comments.	I love identical code... ;)

>Index: mailnews/base/src/nsMsgContentPolicy.cpp

>+  if (aContentType == nsIContentPolicy::TYPE_OBJECT)
>   {
>       // only allow the plugin to load if the allow plugins pref has been set
>-      *shouldLoad = mAllowPlugins;
>+      *aDecision = mAllowPlugins ? nsIContentPolicy::ACCEPT :
>+                                   nsIContentPolicy::REJECT_REQUEST;

This should be REJECT_TYPE, right?

>+nsMsgContentPolicy::ShouldProcess(PRUint32          aContentType,
>   return NS_ERROR_NOT_IMPLEMENTED;

Again, this should set the out param to accept and return NS_OK.

>Index: content/base/src/nsContentUtils.cpp
> nsContentUtils::CanLoadImage(nsIURI* aURI, nsISupports* aContext,

>+                                          NS_LITERAL_CSTRING(""), //mime guess

EmptyCString(), please.  I guess the docshell code should do that too; it does
NS_LITERAL_CSTRING("") as well.

Fix up these nits and the ones left over from last time, and I'll give this a
last look?

And thanks for picking this up!

Comment 89

[[e-mail]]

(In reply to comment #88)
>(From update of attachment 147999 [details] [diff] [review])
>>+            else if (this.contentDocument !=
safeGetProperty(event.originalTarget,
>>+                                                             "ownerDocument") ||
>Again, why is this check being added?
>>Index: xpfe/global/resources/content/bindings/tabbrowser.xml
>Same comments.	I love identical code... ;)
Except it isn't, thus the added code in the ff version.

Comment 90

[Boris Zbarsky [:bzbarsky]]

Neil, care to expand on that?  The added code is in "non-tabbed" mode, so why is
it bothering to check the document?  Is it checking for subframes?

Comment 91

[Boris Zbarsky [:bzbarsky]]

Neil sez it is in fact checking for subframes.  So I'm fine with adding it.

Comment 92

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: Patch v1.7

Updated to the review comments.

On i side node, i'm wondering about the tabbrowser changes. Those are needed
for the site icons. But those icons need to go through the image loading stuff
in content or layout somewhere. I don't know that code, but shouldn't it get
hit by the content policy checks that already are there?
Or is it because it is xul, and not html, and we only have checks in html?

Comment 93

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 148056 [details] [diff] [review]
Patch v1.7

>Index: toolkit/content/widgets/tabbrowser.xml

>+                                              uri, origURI, targetDoc,

event.target, no?  (you got the other three spots, missed this one).

Fix that, r+sr=bzbarsky on the various back end stuff, sr=bzbarsky on the front
end.

Neil, would you mind reviewing the XBL changes?

> But those icons need to go through the image loading stuff

Well... There's also this icon cache (or used to be) which did weird things
like loading the data directly...

> Or is it because it is xul, and not html, and we only have checks in html?

That too.  Though I plan to add the checks for XUL.

Comment 94

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: Patch v1.7.1, updated tabbrowser stuff

The updated parts of tabbrowser.xml (twice).
This one actually works.

Comment 95

[[e-mail]]

Comment on attachment 148136 [details] [diff] [review]
Patch v1.7.1, updated tabbrowser stuff

>+            var baseURIStr = safeGetProperty(targetDoc, "baseURI");
>+            var docCharset = safeGetProperty(targetDoc, "characterSet");
>+            var baseURI = ioService.newURI(baseURIStr, docCharset, null);
>+            var uri = ioService.newURI(href, docCharset, baseURI);
Surely href is already an absolute URI?
It's a pity that nsILink is unscriptable because that would give you a URI.

>+            var origURIStr = safeGetProperty(targetDoc, "documentURI");
>+            var origURI = ioService.newURI(href, docCharset, baseURI);
I assume you meant to use origURIStr here? Again, isn't it absolute?

>+            // Security says okay, now ask content policy
>+            try {
>+              var contentPolicy =
>+                Components.classes['@mozilla.org/layout/content-policy;1']
>+                          .getService(Components.interfaces.nsIContentPolicy);
>+            } catch(e) {
>+              return; // Refuse to load if we can't do a security check.
>+            }
Might be an idea to move this block up so we don't do the security manager
check if we can't access content policy?

Comment 96

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: Patch v1.7.2, updated tabbrowser again

Updated to neil's comments.

Comment 97

[[e-mail]]

Comment on attachment 148235 [details] [diff] [review]
Patch v1.7.2, updated tabbrowser again

Although I think you moved the contentPolicy load up a bit too enthusiastically
- after the if (!href) check should be fine.

Comment 98

[Mike Connor [:mconnor]]

Comment on attachment 148235 [details] [diff] [review]
Patch v1.7.2, updated tabbrowser again

looks fine for the toolkit changes, I won't pretend to have read the entire
discussion in this bug, but based on neil's r= and my own review of the changes
here, this looks good for Firefox.

Comment 99

[Michiel van Leeuwen (email: mvl+moz@)]

Patch checked in.

Comment 100

[HJ]

This might be a stupid question, because I didn't check, but don't we need
additional security checks for the icons in the personal toolbar, bookmarks
manager/menus or sidebar?

Comment 101

[Boris Zbarsky [:bzbarsky]]

HJ, please read the bug before commenting.  Especially comment 37.

Comment 102

[HJ]

(In reply to comment #101)
> HJ, please read the bug before commenting.  Especially comment 37.

Sorry I asked this blunt question, but I did, before posting my original question. 

Hey, I still don't get it. Must be me. Almost dead twice in the last two weeks.
Maybe you should do my job for a week, just to see how it effects you. It sure
does hurt a lot my friend.

Comment 103

[HJ]

I used to use nsIDOMWindow in the 'old' implementation but that's gone now. What
is the fastest way to get to nsIDOMWindow from nsIDOMNode? My javascript
component seems to be a lot slower now :(

Comment 104

[Boris Zbarsky [:bzbarsky]]

> What is the fastest way to get to nsIDOMWindow from nsIDOMNode?

In JS?  node.ownerDocument.defaultView.

Comment 105

[HJ]

(In reply to comment #104)
> > What is the fastest way to get to nsIDOMWindow from nsIDOMNode?
> 
> In JS?  node.ownerDocument.defaultView.

Yes, but that fails for background images! Here what I get after visiting
http://mozillazine.org/

aRequestingNode  = null
aContentType     = TYPE_IMAGE (3)
aContentLocation = http://mozillazine.org/images/default/sky.png

aRequestingNode  = null
aContentType     = TYPE_IMAGE (3)
aContentLocation = http://mozillazine.org/images/default/blimp.png

I would really like to see the return of nsIDOMWindow or someone should fix this
bug. You can't do shit without nsIDOMWindow you know.

Comment 106

[Michiel van Leeuwen (email: mvl+moz@)]

I guess you are hitting bug 243948. A fix for that has just been checked in.

Comment 107

[HJ]

(In reply to comment #106)
> I guess you are hitting bug 243948. A fix for that has just been checked in.

Will that patch also initialize aRequestingNode (should not be null) ?

Comment 108

[Boris Zbarsky [:bzbarsky]]

HJ, please try _reading_ bugs, ok.

As for nsIDOMWindow, there are cases when images are loaded completely outside
the context of a DOMWindow.  In those cases, the window _WILL_ be null (even if
the requesting node is not).  You have to deal with that anyway.

Comment 109

[HJ]

(In reply to comment #108)
> HJ, please try _reading_ bugs, ok.

Again, I do, but we are a real consumer of nsiContentPolicy and used to the
speed of compiled C code. Damnit, now we have to deal with this in much slower
javascript. This sucks big time!

> As for nsIDOMWindow, there are cases when images are loaded completely outside
> the context of a DOMWindow.  In those cases, the window _WILL_ be null (even if
> the requesting node is not).  You have to deal with that anyway.

Yeah right...that's why we had this perfectly working JS component that worked
for over one year without 'window' being null, until recently. I guess we all
were just lucky...

Comment 110

[Boris Zbarsky [:bzbarsky]]

HJ, feel free to mail me with your perf issues.  There shouldn't be any, from
what I see; if there are there is a bug in your code or in ours.

> I guess we all were just lucky...

YES!  You were in fact lucky -- all sorts of people who _should_ be calling
content policy were not because they had no window to pass in and the content
policy code would just make those calls no-ops anyway.  These callsites WILL be
getting updated to call content policy now.  With the change to use nsIDOMNode,
they will be able to pass in something sane, usually, since they do have a
document most of the time.  But expect to see some perfectly correct calls with
a null domnode coming up.

Comment 111

[timeless]

i'd like to repeat hj's complaint.

i just tried using the new api.

i can't.

a top level load of |about:| which should give me the chrome window or the 
tabbrowser or /something/ is giving me null as the requestnode.

Comment 112

[Michiel van Leeuwen (email: mvl+moz@)]

So, file a bug.

Comment 113

[Boris Zbarsky [:bzbarsky]]

That's because in a toplevel window load there is no requesting node.

Perhaps if you describe the use case you are trying to address we can see what
we can do.  There was a good bit of discussion in this bug about what context
was needed and no one piped up, so I went with a simple solution that provided
all the existing functionality to all the implementations I was aware off
without burdening callers with an unusable API (where it's not clear what should
be passed as a "context").

Comment 114

[timeless]

we have a series of things which run in the context of a chrome window. they
boss around a content window. they ask it to load a single url perhaps
<http://www.google.com>

now, google has <img src="/images/logo.gif" width=276 height=110 alt="Google">

which if the browser actually interprets the page will trigger it to load
<http://www.google.com/images/logo.gif> we don't want that to load right now,
and may or may not want it to load later. if we do want it to load we are likely
to specify headers which mozilla would never send.

to do this, our chrome code installs a contentpolicy and registers with the
content policy saying "please manage this [chrome] window". it then tells the
manager what url it plans to load. the manager may decide to only allow a url to
load once. when the browser (unfortunately) decides that it wants to load the
image, the content policy vetos the request.

now... the url could be a gopher or file or javascript or data or mailto url,
and we don't want to replace the io service (i could, please don't make me do
this), so we can't simply use the http on modify request topic and veto things
there.

our goal is to get a dom representation of the document we loaded, possibly
including the ability to interact with javascript. (yes i know, this would
require letting the javascript load, we might let the content policy load that,
but more likely we'll do something else). we intend to interact with the loaded
document.

we cannot control how content is shaped, we cannot prevent network hits entirely
since we have code that needs to talk to the network. and I should not have to
write my own application in order to do this.

From a quick glance, it looks like my immediate needs would be satisfied by
filling in the aRequestingNode with the browser/tabbrowser that's making the
request. from there i can get back to the window and back to what i was doing.

bz: in a top level load, there is a requesting node, it may happen to be chrome,
but that's fine, we're all chrome priv'd here, there's no need to pretend that
chrome doesn't exist.

in general i don't care about cases where there's no associated window, with the
distinct exception of xmlhttprequesuts, where I will definitely need to be able
to decide that a load is related to my window and that i want to veto it with
extreme prejudice.

note: for that case, i can not rely on domains since xmlhttprequests/soap
transactions (which i really do need to control - generally squash) have a
security exception which allows them to connect pretty much whereever they like.
it's been a few weeks since i've crashed our unusable soap impl, so i can't
remember if it happens to include any referer tag, but even if it did, i
wouldn't really be able to do anything with it. such requests when they
originate in a browser should have aRequestingNode set to something in some
document, possibly the document. The key is that I be able to trace the request
back to the document and the window from which it came. For such requests it
should not immediately claim the chrome window unless the chrome window made the
request, if a window has three content domains in framesets, i need to be able
to know which one made the request.

Comment 115

[Boris Zbarsky [:bzbarsky]]

> bz: in a top level load, there is a requesting node,

Well... not in a _real_ toplevel load.  You're doing a load in an iframe.

It may indeed makes sense to pass the frame element for loads in frames.  That
won't pass a useful element to real toplevel (window-level) loads, but that's
not likely to be an issue.  Like mvl said, file a bug.  It should be about a
1-line fix.  But see below.

> xmlhttprequesuts, where I will definitely need to be able to decide that a
> load is related to my window

Yeah, using the document there makes sense.  I doubt XMLHttpRequest makes any
sort of content policy checks, though.  File bugs.

At some point someone should look through the first set of patches Tim posted
and pull out the new content policy callers he added.  File one bug per caller.

Comment 116

[timeless]

*nod* for real top level loads, could we pass the window object in aExtra?

now i just have to figure out how many bugs to file and which they are...

Comment 117

[Boris Zbarsky [:bzbarsky]]

> *nod* for real top level loads, could we pass the window object in aExtra?

"file a bug", like we keep saying...  cc mvl and me.

Comment 118

[Joe Sabash [:JoeS1]]

Could this checkin have caused the thunderbird trunk regression in Bug #244780