164086 - File upload vulnerability using event.rangeParent

Status: VERIFIED FIXED

(Core :: Security, defect, P1)

Description

[Jesse Ruderman]

This is file upload hole smiilar to bug 163598 (event.originalTarget) and 
bug 164023 (event.relatedTarget).  Does anyone know what event.rangeParent is?

Comment 1

[Jesse Ruderman]

Attachment: demo

Comment 2

[Jesse Ruderman]

cc everyone from bug 164023.

Comment 3

[Jesse Ruderman]

A commercial build from today that has the fix for bug 163598 is still
vulnerable to this bug.

Comment 4

[scottputterman]

reassigning to jkeiser.

Comment 5

[kinmoz]

FYI, it looks like rangeParent and rangeOffset are used internally by code that
wants to figure out the Content and Offset into the dom tree at the (x,y)
position in the event, so that it can be used in a range to position the selection.

This is used for example by the editor to figure out where to put selection when
middle-mouse-pasting on Unix.

Comment 6

[Asa Dotzler [:asa]]

Isn't it time to do what jkeiser proposed, and wall off the non-web-XBL-created
anonymous content from web-content, at the nsIClassInfo level?

/be (using Asa's laptop)

Comment 7

[John Keiser (jkeiser)]

Yes, that's what I'm working on.  It's not looking terribly easy, but it may be
doable by day's end.  I believe I can create a quick check whether content is
anonymous, but I need help finding the right place to actually perform the check.

JST?

Comment 8

[John Keiser (jkeiser)]

Aha.

http://lxr.mozilla.org/seamonkey/source/caps/src/nsScriptSecurityManager.cpp#2254
looks like my friend.

Comment 9

[Jesse Ruderman]

Since rangeParent and rangeOffset are only used internally, can they be made
inaccessible to scripts?

Comment 10

[John Keiser (jkeiser)]

If rangeParent and rangeOffset are accessible to scripts, how do you know
they're only used internally? ;)  The answer is, if your assumption is correct,
Yes.  but we should deny anonymous content access anyway.  It's looking
possible, but I have to think about how to avoid a pageload hit.

Comment 11

[Brendan Eich [:brendan]]

jruderman: no more symptom patching, or we will be patching symptoms till the
cows come home.  I've been there before (JS first shipped in 2.0; soon there was
2.01, then 2.02, ....  2.06 was the last one, IIRC).

/be

Comment 12

[Johnny Stenback (:jst)]

Preventing access from JS to non-XBL anonymous content should be doable, but it
looks harder than I'd like it to be. We can prevent creation of a wrapper for
non-XBL anonymous content, but it looks like it's non-trivial to prevent access
to an already created wrapper for non-XBL anonymous content. Given that, we can
prevent access to non-XBL anonymous content from web content as long as we don't
go and touch the non-XBL anonymous content from chrome, ever. I don't think we
can guarantee that that doesn't happen.

/me thinks more and reads more code

Comment 13

[Jaime Rodriguez, Jr.]

cc'ing bob and doron for evangelism input on any potential fix.

jkeiser: is possible (i.e. its not 3 am), pls remember to provide teri with a
private build so that she can run input form controls test before applying the
patch to the 1.0 branch. thanks! with that, adt1.0.1+ for to wall off these
exploits.

Comment 14

[Doron Rosenberg (IBM)]

I have 2 uses of relatedTarget that is given to gecko browsers. (top german
sites - allianz and lego).  It is also in the dom ref
(http://www.mozilla.org/docs/dom/domref/dom_event_ref20.html).  Google shows
quite a few occurances

No occurances of rangeParent though.

Comment 15

[John Keiser (jkeiser)]

doron: what sites use it?  We need to verify that this will not break them, but
if it does break them they are using extreme internals of our code they should
not be messing with :)

Comment 16

[Bob Clary [:bc] (inactive)]

I am not sure what use event.rangeParent has in the wild. If they are using
non-standard, non documented features I say it is ok to break them. rangeParent
is not in the Mozilla DOM reference and is not found in any search of
www.mozilla.org.

Regarding relatedTarget however, it is a standard and I don't have access to bug
164023 (PITA!). You *can not remove relatedTarget*.

Comment 17

[John Keiser (jkeiser)]

Attachment: IsAnonymous() patch (no checkin-ez-vouz)

this patch makes IsAnonymous() work in nsIContent.  JST has the second part of
this working (in ClassInfo), posting for transfer

Comment 18

[Johnny Stenback (:jst)]

Attachment: Complete, but untested, diff, contains the above patch too.

This diff, in addition to the above diff, adds support for a new scriptable
helper hook that is called every time a wrapper is accessed in XPConnect, if
the scriptable helper says it wants to be asked that question. This lets us add
the security check in the scriptable helper code and it will only be called for
DOM nodes, except for document nodes.

Comment 19

[Jaime Rodriguez, Jr.]

bclary: you now have access to bug 164023. :-)

Comment 20

[John Bandhauer]

jst: the check you added to XPCWrappedNative::InitTearOff is not "called every
time a wrapper is accessed in XPConnect" as you said. It is only called when the
wrapper is created. Is this what you intended? I'm still not clear why the
security manager is not the thing that should gate this.

Comment 21

[John Keiser (jkeiser)]

jband: yes, AFAICT it should be in FindTearOff for this to work right.

Comment 22

[John Keiser (jkeiser)]

Is there a method in the SecurityContext to deny access to an object of a
particular type?  We only saw CanCreateWrapper() ... we need to make sure it
can't access the wrapper either.

Comment 23

[John Bandhauer]

See the call to sm->CanAccess near the top of XPCWrappedNative::CallMethod .
This is where the security manager is called (assuming it wants to be called) on
every property access and method call.

Comment 24

[John Bandhauer]

Note that the security manager is already getting called each time. The security
manager is passed the classinfo and it should be able to tell if the node needs
special handling based on a flag you could return from the classinfo's GetFlags
method. THe security manager has its own scheme for caching policies for speed
sake. I'm claiming that all security checks ought to funnel through the security
manger (even if *it* then delegates to native object) and not be hacked into
xpconnect.

Comment 25

[John Keiser (jkeiser)]

I need to go home.  I'll look into this further from there.

jband and hyatt, with this patch I'm seeing some failures to access anonymous
while constructing textareas in http://www.johnkeiser.com/cgi-bin/mozform.pl. 
This page does not access anything with JS onload.  I presume this will still
happen if we mess with security manager.

The place I think is triggering it is
http://lxr.mozilla.org/seamonkey/source/content/xbl/src/nsXBLProtoImpl.cpp#79
... in other words, if I read this correctly, XBL's binding initialization (my
page does not have XBL on it!) is trying to initialize with the SGO's
privileges, which I presume are document scope.  This specifically happens when
we try to load the bindings on the <scrollbar> inside the <textarea>.

http://lxr.mozilla.org/seamonkey/source/content/xbl/src/nsXBLService.cpp#724 is
where it all starts.

Strangely, this does not seem to affect anything working--we must load the
binding again later in chrome privilege, because everything on a textarea works,
including both vertical and horizontal scrollbars (I tested vertical on Bugzilla
and horizontal on the page above).

I'd still feel more comfy knowing with some certainty that these warnings are OK
or what they might affect that we can test.

Comment 26

[John Keiser (jkeiser)]

Please to be noting that I posted that text from the patched build.  Scrollbar
and submit and everything worked fine.

Comment 27

[Johnny Stenback (:jst)]

I agree that this would ideally go into the security manager code, but if we do
it there it's really hard to isolate the code so that we only take the
perfornace hit that this incolves (which isn't all that huge, this code path is
rather performance critical) in the places where it really matters. Maybe I'm
over-reacting here (no, I don't have numbers that show that this actually does
matter, but from what I recall from my previous staring at profiles of this
code, it does), but I don't want this to be done in the security manager based
on the fact that a QI succeeds or anything like that (since QI misses can be
rather expensive in some cases, think XBL and binding manager code in QI). And
besides, the native object really shouldn't know anything about this, the
JS-specific helper is where this decision needs to be made. Given that, the
natural place to do this is in the DOM classinfo code, and our path into that
code up to this point has always (except for one exception) been throgh XPConnect.

If we say that it's safe to steal flags that are used by
nsIXPCScriptable::GetScriptableFlags() (or nsIClassInfo::GetFlags()) and make
the security manager call into the scriptable helper (or some other related
code) through some new interface then maybe that's the way to go, but that seems
hackish to me.

Jband, I don't see this as something that's "baing hacked into xpconnect", it's
more like a new feature that we now use for plugging this security hole, and
maybe it can be used to plug more holes (or who knows what) in the future. IMO
this is not that different from the existing checkAccess() hook, even if there's
no JS engine API equivalent to this new hook. I think it's a useful hook, but
then again, maybe that's just me...

Comment 28

[Johnny Stenback (:jst)]

... minus all the typos in the above comment :-)

Comment 29

[John Keiser (jkeiser)]

Attachment: Patch using CanAccess

This patch uses CanAccess.  This is good because:
(a) it is more secure (it catches *all* cases where anon content could get to
web content)
(b) it carries less risk of breaking webpages since those who *are* using
relatedTarget are likely just using == to compare it to other things

This is, however, likely less performant than the other patch

Comment 30

[John Bandhauer]

jst: I'm not going to throw my weight around here. I'm just spouting opinions.
You guys decide.

I do think that routing security policy through the security manager is a good
thing. But perhaps that is religious in the face of critial perf paths. Maybe
the security owner(s) ought to chime in.

I agree that "QI 'till you die"  is bad - I suggested to jkeiser that a
classinfo flag would be a good way to decide up front whether to do the QI or
not in the first place (the security manager already gets the classinfo flags -
at least in some cases).

I'm still thinking that whomever you call to make this decision, the call should
happen at access time rather than at wrapper building time. This protects from
the "real callers", rather than just the "first accessors". Do you disagree?

Comment 31

[David Hyatt]

JS in XBL bindings executes with the permissions of the document that contains the 
element to which the binding is attached.  This means that if you have an XBL binding for 
a textarea, and that textarea is in an HTML document that any JS code in the XBL 
binding executes with the permissions of the HTML document (and not chrome).

Comment 32

[John Keiser (jkeiser)]

Attachment: Patch using CanAccess (v1.1)

This version of the patch should make things slightly more performant by
following jband's suggestion of an nsIClassInfo flag.  At least that way things
that don't use DOMClassInfo won't waste time.

Comment 33

[John Keiser (jkeiser)]

Attachment: Patch using CanAccess (v1.1.1)

OK, this one doesn't have ^M's and doesn't change nsVoidArray :)

Comment 34

[Jaime Rodriguez, Jr.]

got a verbal a=chofmann for Drivers. pls check this in before 4 am, so that it
makes today's builds, then replace the keyword "Mozilla1.0.1+" with "fixed1.0.1".

once the builds are out, let's make sure and run the perf tests again, as well
as test file input on top sites.

Comment 35

[John Keiser (jkeiser)]

Attachment: Patch using CanAccess (v1.1.2)

This version also contains the nsSecurityScriptManager.cpp, which is rather
important to this patch.

Comment 36

[John Keiser (jkeiser)]

Attachment: Patch using CanAccess (v1.1.3)

I have a new disorder, a patch impediment.  I stutter patches.

Comment 37

[Christopher Aillon (sabbatical, not receiving bugmail)]

cc'ing tor who can hopefully sr= this patch.

Comment 38

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

Comment on attachment 96425 [details] [diff] [review]
Patch using CanAccess (v1.1.3)

>+NS_IMETHODIMP_(PRBool)
>+nsXULElement::IsAnonymous() const
>+{
>+  return PR_FALSE;
>+}
>+
>+NS_IMETHODIMP_(void)
>+nsXULElement::SetAnonymous(PRBool aAnonymous)
>+{
>+}

Add an NS_ERROR in here to make sure that noone accidently calls this and
expect it to work.


>Index: caps/src/Makefile.in
>@@ -45,6 +45,7 @@
> 		  intl \
> 		  docshell \
> 		  windowwatcher \
>+			content \
> 		  $(NULL)

Fix the whitespace.


>+      content->SetAnonymous(PR_TRUE);
>+//      NS_ASSERTION(!content->IsAnonymous() && !aParent->IsAnonymous(),
>+//                   "non-anonymous-supporting content with non-anonymous 
>+//                   );

Please remove the assertion since it's commented out anyway (and wrong afaict)

with that, r=sicking

Comment 39

[John Keiser (jkeiser)]

Anyone that can sr= here, please do so or at least nitpick.  Assuming perf tests
come back OK, we are going to try to land this on 1.0 branch so as not to kill
the schedule (as I am told landing tomorrow afternoon would do).  Given that
this is the *only* patch that will make the 1.0 tree anyway, it's not a bad
gamble and it won't hurt anybody.  If further sr= from jband or jst in the
morning picks up necessary changes we will respin.

We have a driver and adt approval for this action (chofmann and jaimejr). 
Please note that tonight's landing will be *branch only*.  Further debate will
proceed vigorously in the morning, I am sure.

Comment 40

[John Keiser (jkeiser)]

Attachment: Patch using CanAccess (v1.1.4)

Fix sicking's review

Comment 41

[John Keiser (jkeiser)]

Attachment: Backout of bug 163593

This portion of the patch backs out bug 163593 (posted separately so that it
will be easier for people who want to back-port to builds without the fix).

Comment 42

[John Keiser (jkeiser)]

Comment on attachment 96435 [details] [diff] [review]
Backout of bug 163593

JST sr'd, and chofmann a='d in bug 163593.

Comment 43

[John Keiser (jkeiser)]

eek, bug 163598 is the bug being backed out.

Comment 44

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

Comment on attachment 96434 [details] [diff] [review]
Patch using CanAccess (v1.1.4)

r=sicking

Comment 45

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

Comment on attachment 96435 [details] [diff] [review]
Backout of bug 163593

r=sicking

Comment 46

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 96434 [details] [diff] [review]
Patch using CanAccess (v1.1.4)

Note: I'm *not* an sr= but I'm somewhat familiar with the code and since it
seems you really really _really_ need a super-review, I hope the sr=s can
forgive me for giving a faux sr=caillon since this is a critical bug.  I'm
leaving has-super-review alone though in case a real sr= happens to come by and
can help here.	Looks good but please fix the following nits though before
checking in:


>Index: caps/src/nsScriptSecurityManager.cpp

>+static inline PRBool
>+ShouldCheckAnonymous(nsIClassInfo* aClassInfo)
>+{
>+    if (!aClassInfo)
>+        return PR_FALSE;

Callers passing in null?  Seems like we'd want an ENSURE_TRUE here...


>Index: content/base/src/nsGenericElement.h

>@@ -71,26 +71,34 @@
> 
> typedef unsigned long PtrBits;
> 
>-// This bit will be set if the nsGenericElement has nsDOMSlots
>+/** This bit will be set if the nsGenericElement has nsDOMSlots */

If you're going to make this change, please do

  /**
   * foo
   */

as that is consistent with our other javadoc stuff, as well as the javadoc
comments directly following this.


> #define GENERIC_ELEMENT_HAS_LISTENERMANAGER    0x00000004U
> 
>-// The number of bits to shift the bit field to get at the content ID
>-#define GENERIC_ELEMENT_CONTENT_ID_BITS_OFFSET 3
>+/** Whether this content is anonymous */

Same javadoc issue here.  I notice you do this a few other places below as
well.  Fix those too.

Comment 47

[John Keiser (jkeiser)]

> Callers passing in null?  Seems like we'd want an ENSURE_TRUE here...

This is copied from the other spot that does this (IsDOMClass).  I do not know
if there is a legitimate circumstance where aClassInfo could be null, but I
assume it can from the code.


> If you're going to make this change, please do
> 
>   /**
>    * foo
>    */

In general we do one line when we can, and then convert to this when we wrap or
have two lines.  See property JavaDoc almost anywhere in the tree for examples.
 I know this one, I wrote a good deal of JavaDoc in our tree ;)

Comment 48

[John Keiser (jkeiser)]

Attachment: Branch Necessities

Here are the changes I had to make to make the branch happen.  Most notably,
there is a new boolean in nsDOMSlots.  Glad that doesn't get created often. 
Wish jst's DOMSlots improvements on trunk were in the branch :)

Comment 49

[Christopher Aillon (sabbatical, not receiving bugmail)]

Just saw bryner awake, nagging him for sr=.

Comment 50

[John Morrison]

My test build is not going to be finished in time to have a result by 4am.
I suggest that we'll have to just land and cross our fingers. I'll have results
shortly after 4am, and the tinderboxes will dump numbers when they finish too.
(Who doesn't love a patch that makes you rebuild the world. kerz: can you 
look at some mac tests on the branch in the morning?).

Comment 51

[John Keiser (jkeiser)]

Attachment: Branch Necessities v2.0

This version of the branch patch doesn't add bloat or require the creation of
DOMSlots by mimicking a page from JST's book (what we've been doing on trunk
for a long time), reusing the lower bit of content ID.

Comment 52

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

+  // Shift content id to store stuff in the lower order bytes
+  mContentID |= (aID << 1);

you have to unset the upper bits first

just do mContentID = aID << 1 | (mContentID & 1)

Comment 53

[John Keiser (jkeiser)]

Attachment: Branch Necessities v2.1

Thanks.

Comment 54

[Brian Ryner (not reading)]

Comment on attachment 96434 [details] [diff] [review]
Patch using CanAccess (v1.1.4)

My only suggestion would be to clarify somehow (through a comment, since I
can't think of a concise method name to indicate this) that IsAnonymous refers
only to C++-generated anonymous content, not XBL-generated content.  sr=bryner.

Comment 55

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

Comment on attachment 96441 [details] [diff] [review]
Branch Necessities v2.1

r=sicking

Comment 56

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 96441 [details] [diff] [review]
Branch Necessities v2.1

<bryner> you can apply my sr to the branch patch too

Comment 57

[John Morrison]

On win2k/500MHz/128MB on the trunk, I do not see any significant difference in 
pageload performance between a build with patch attachment 96425 [details] [diff] [review] and a the same 
build without that patch (in fact the difference for the single test pair is 
less than 0.5% for either avg. or avg. median comparison). Excellent job, 
jkeiser!

Comment 58

[John Keiser (jkeiser)]

OK, this fix is checked in to 1.0.  I will do 1.1 in the morning (this is a
relative term, so feel free to check in yourself if you like--hopefully you can
do it with just "Patch using CanAccess (v1.1.4)" and "Backout of bug 163593"),
since it seems we have missed the cut for those builds anyway and it is past my
bedtime.

JST, jband, let's go over this and figure out if we did something acceptable for
branch or not, and then figure out if we want to make changes for trunk.

Comment 59

[Jaime Rodriguez, Jr.]

bsharma: bindu, pls test todat's builds and verify this issue has been resolved
without regression. thanks!

chrisn/susiew: what we need from the entire organization, is to test the site
list(s) and make sure we did not cause any regressions wrt to file input. pls
parse the list into smaller lists (10 - 25 sites), then walk tem around the
building, and get commitment  from each individual to test the sites on all
pltforms. should anyone find a problem, pls have them report the issue to
[e-mail], w/cc to jkeiser. thanks!

cathleen/jrgm: let's make sure we did not cause any performance regressions with
this checkin on the 1.0 branch. thanks!

gerardo: pls have your team rerun DOM certification tests on todays builds. thanks!

we need everyone's help on this one, and there is not anything more improtant
that we could do today. pls make every effort to make sure Mozilla1.0.1 is of
the highest quality it can be. thanks!

Comment 60

[saari (gone)]

Bryner, if IsAnonymous only applies to C++ generated content and not XBL, that
would mean that XBL form controls could be open to this bug?

Comment 61

[David Hyatt]

IsAnonymous and SetAnonymous seem like very confusing names to be using here.  I 
mean, XBL-created content is called "anonymous content", so it seems confusing to have 
methods that ask this question that apply only to C++ anonymous content.

I think we need a better term here for this type of content.  I don't mind calling it 
"anonymous", but we need to qualify it with an additional adjective or something to 
distinguish it from XBL-generated anonymous content.

Comment 62

[David Hyatt]

How about the term "UAAnonymous" or "AnonymousUA" to reflect that this is user agent 
anonymous content? 

We need some kind of qualifier on these methods and on the bit, though, because this is 
just too confusing otherwise. :)

Comment 63

[chris hofmann]

+jdunn

Comment 64

[Brendan Eich [:brendan]]

UAAnonymous?  Yuck.

How about NativeAnonymous?

/be

Comment 65

[David Hyatt]

Yeah, that sounds good.

Comment 66

[Daniel (Leaf) Nunes]

Attachment: roll-up patch for 1.1 branch

i removed some of the patch for nsGenericElement.h that didn't seem to pertain
at all to the code the rest of the patch refers to (mostly a comment-style
change on lines of code that only exist on the trunk).

The rest of the patch(es) applied cleanly, and i'd love it if someone a lot
smarter than me could review it for 1.1 suitability.

Comment 67

[Daniel (Leaf) Nunes]

Attachment: same as before, but with IsAnonymous -> IsNativeAnonymous, and same for SetAnonymous

sorry, forgot to read the comments before uploading.

Comment 68

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 96473 [details] [diff] [review]
same as before, but with IsAnonymous -> IsNativeAnonymous, and same for SetAnonymous

looks good to me. Carrying forward previous r and sr from 1.0.1 branch patch.

Comment 69

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 96473 [details] [diff] [review]
same as before, but with IsAnonymous -> IsNativeAnonymous, and same for SetAnonymous

a=roc+moz for 1.1 branch and trunk

Comment 70

[Daniel (Leaf) Nunes]

building 1.1 to test now; this could take a while.

Comment 71

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This fix shouldn't have any effect on XBLFC as far as I can tell. Nor should
XBLFC be vulnerable. XBLFC doesn't do anything with the file name control.

It's probably possible for people to muck with XBLFC controls using techniques
similar to the ones in this bug, but that doesn't lead to any security issues
because XBLFC is unpriviledged; Web authors can't abuse XBLFC to do anything
that they couldn't already do by writing their own XBL. I think.

Comment 72

[John Keiser (jkeiser)]

Yes, people *are* allowed to access XBL anonymous content.  It's part of the
spec.  We only care about the anonymous content we create internally.

Comment 73

[Daniel (Leaf) Nunes]

hey, jkeiser? where does SetFlags come from? my 1.1 tree is failing to build
with the patches that use SetFlags and UnsetFlags.

Comment 74

[bsharma]

Verfied on 2002-08-23-branch build on Win 2000.

Load the above demo.
Do mouseover, the text filed is NOT filled in by c:\test.txt
Clicked on Submit button
An exception is thrown in the JS console.

I will change the keywords once the bug is marked Fixed.

Comment 75

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

Leaf: Get/SetFlags landed after 1.1 branched. You should be able to use the
1.0.1-patch for the 1.1 branch though

Comment 76

[John Keiser (jkeiser)]

This is not on trunk yet, and it's fixed, not worksforme :)  Please do not
commit to trunk until jst and jband have had time to comment.

Comment 77

[Daniel (Leaf) Nunes]

Attachment: patch for the 1.1 branch (for realsies!)

patch 96473 probably applies to the trunk (haven't tested), but this patch is
the one that seems to build (so far) on the 1.1 branch.

Comment 78

[John Keiser (jkeiser)]

Comment on attachment 96502 [details] [diff] [review]
patch for the 1.1 branch (for realsies!)

All the essential components from the other patches are there, and since it
builds and works, r=jkeiser :)

Comment 79

[chris hofmann]

folks,  go have a look at http://bugzilla.mozilla.org/show_bug.cgi?id=117611 and
see if I'm smoking too much of something, or if this is something that we might
expect and can live with......

Comment 80

[Johnny Stenback (:jst)]

I take it this didn't land on the trunk yet?

This patch that went in on the branch is not at all far from a raw "QI 'till you
die" (as jband put it :-) ) since this patch makes us do the QI in *every call*
to CheckAccess() where the object we're accessing is a "DOM" thing, note, not
just for Elements and Text nodes and the like, but for *everything* that uses
nsDOMClassInfo (and anything else that sets the nsIClassInfo::DOM_OBJECT flag).
That means that in probably 95% of the calls that go through CheckAccess() we do
the QI when we would only need to do the QI when the object in question is an
object that actually implements nsIContent. That's not cool.

So on the trunk, at least, we should leave nsDOMClassInfo::GetFlag() unchanged
and over-ride the GetFlags() method on the helpers that are used for things that
actually are DOM *nodes* and implement nsIContent. I.e. we should add a
GetFlags() to nsNodeSH that returns this new flag in addition to the ones the
base class returns (#define those in a macro and just or in the new bit in the
new GetFlags() method), and we should also add a GetFlags() to nsDocumentSH
since it inherits nsNodeSH, but classes that use nsDocumentSH do *not* implement
nsIContent.

If we'd have more time on the branch we should do the same there, but since we
don't, I guess we can live with this "QI 'till you die" approach on the branch.

As a side note, I really don't like the fact that we're adding a content
specific CHECK_ANONYMOUS flag to nsIClassInfo, which IMO really does not belong
there. But then again, I don't have anything better to suggest as an alternative
at the moment...

Comment 81

[John Bandhauer]

Yeah, what jst said (or was that the jst simulacrum talking?). I don't mind
burning a classinfo flag for this use. A better name might be in order though.
Just calling it nsIClassInfo::CONTENT_NODE would be fine by me.

Comment 82

[Johnny Stenback (:jst)]

That was me talking, the real me! :-)

Or maybe nsIClassInfo::DOM_NODE? I'm feeling a lot better about this already.

Comment 83

[John Bandhauer]

We already have nsIClassInfo::DOM_OBJECT. Would DOM_NODE be a good way to
indicate that the object implements nsIContent?

I started to feel better when jkeiser pointed out that your scheme didn't really
have less QIs than his (modulo getting the scope of the flag usage right).
Otherwise I was leaning back toward your scheme (like my leanings matter much :)
He also promised to take a whack at not making the secman redundantly call GetFlags.

Comment 84

[Johnny Stenback (:jst)]

Talked this over with jkeiser, CONTENT_NODE makes more sense than DOM_NODE, then
we can truly say that if this flag is set the instance of the class implements
nsIContent (whereas all DOM node's don't implement nsIContent).

Feeling better and better by the minute here :-)

Comment 85

[John Keiser (jkeiser)]

Attachment: Patch using CanAccess (v1.1.5)

This patch renames the ClassInfo flag to CONTENT_NODE, which means "implements
nsIContent".  We set this on nsNodeSH (for all DOM nodes) and unset on
nsDocumentSH.  We *will crash* if there is a node that does not implement
nsIContent that says it does.

Comment 86

[John Keiser (jkeiser)]

Attachment: Patch using CanAccess (v1.2)

This makes IsAnonymous() -> IsNativeAnonymous() like it's supposed to :)

Comment 87

[John Keiser (jkeiser)]

Attachment: Patch using CanAccess (v1.2.1)

Follow jst's suggestion of having a class to hold the classinfo flags (to avoid
double virtual calls).

Comment 88

[Johnny Stenback (:jst)]

Comment on attachment 96533 [details] [diff] [review]
Patch using CanAccess (v1.2.1)

I'm in a nit-picky mood, so here goes :-)

- In nsScriptSecurityManager.cpp

+// Helper class to get stuff from the ClassInfo and not waste extra time with
+// virtual method calls for things it has already gotten
+class ClassInfoData
+ {

Uh, space between the opening brace? :-)

+public:
+    ClassInfoData(nsIClassInfo *aClassInfo)
+	 : mClassInfo(aClassInfo), mDidGetFlags(PR_FALSE) { }

No one-line inline functions please, move the braces onto their own lines.

+    PRUint32 GetFlags() {

Opening brace on it's own line for functions (and classes).

+    PRBool IsDOMClass() { return GetFlags() & nsIClassInfo::DOM_OBJECT; }
+    PRBool IsContentNode() { return GetFlags() & nsIClassInfo::CONTENT_NODE; }

4-line in stead of 1-line functions! :-)

- In dom/src/base/nsDOMClassInfo.h:

+  NS_IMETHOD GetFlags(PRUint32 *aFlags) {
+    nsEventRecieverSH::GetFlags(aFlags);
+    *aFlags |= nsIClassInfo::CONTENT_NODE;
+    return NS_OK;
+  }

No inline virtual methods here, no matter how small. Put the function body in
nsDOMClassInfo.cpp, in the same order that they appear in the header file. More
of the same below.

And, instead of making a virtual call into the base class' GetFlags(), #define
the flags that are used in nsDOMClassInfoSH::GetGlags() and set *aFlags to that
plus the new bit, that's faster and less code, and no more work from a
maintanence point of view.

sr=jst with that.

Comment 89

[John Keiser (jkeiser)]

Attachment: Patch using CanAccess (v1.2.2)

Fix JST's nits, and a leetle problem with that new class not working at all due
to uninitialized memory.

Comment 90

[Johnny Stenback (:jst)]

Comment on attachment 96537 [details] [diff] [review]
Patch using CanAccess (v1.2.2)

The assertion in nsXULElement::SetNativeAnonymous() is now missing, is that
intentional?

- In nsScriptSecurityManager's ClassInfoData:

+    PRBool IsDOMClass()
+    {
+      return GetFlags() & nsIClassInfo::DOM_OBJECT;
+    }
+    PRBool IsContentNode()
+    {
+      return GetFlags() & nsIClassInfo::CONTENT_NODE;
+    }

Indent the function bodies 4 spaces, not just 2 spaces in this file.

sr=jst

Comment 91

[John Keiser (jkeiser)]

A review by someone who is around during the weekend would be nice ... jband,
sicking, brendan, bryner, hyatt?

I'd like to get this checked in to trunk.  1.1 and 1.0 already have it.

Comment 92

[John Keiser (jkeiser)]

*** Bug 163768 has been marked as a duplicate of this bug. ***

Comment 93

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

Comment on attachment 96537 [details] [diff] [review]
Patch using CanAccess (v1.2.2)

r=sicking

you might wanna add an

// XXX need to fix this, see bug YYY

in nsXULElement::SetNativeAnonymous and a couple of other places

Comment 94

[John Keiser (jkeiser)]

Fix checked in to trunk.  leaf checked in to 1.1 a couple of days ago.  Only
0.9.4 left.

Comment 95

[Bradley Baetz (:bbaetz)]

reopened - jkeiser left the no-longer-inline methods out of the checkin turning
the tree red, and it wasn't obvious that a streight copy from the previous patch
would be what was requried.

Comment 96

[John Keiser (jkeiser)]

Fix checked in again earlier.  It stuck.

Comment 97

[John Keiser (jkeiser)]

Attachment: Patch for the 0.9.4 branch

This is the patch against 0.9.4.

Comment 98

[John Keiser (jkeiser)]

Fix checked in to 0.9.4.  Somebody please let me know if there are problems; I
compiled and ran and all was well on Linux here.

Comment 99

[Jaime Rodriguez, Jr.]

jkeiser: are the the checkins for 1.1 and the trunk different than what landed
on the 1.0 branch? if yes, then, should we get the updated patch on the 1.0 branch?

Comment 100

[John Keiser (jkeiser)]

Jaime: the updated patch may or may not be nice to land on the 1.0 branch
depending on whether DHTML performance slipped.  The trunk changes (they were
only in post-1.1 trunk) mainly were a few extra cautions to avoid some
performance hit.  The impact, in practice, may or may not be noticeable.  Needs
further testing of JS performance between 1.0 and 1.0.1.

Comment 101

[Randell Jesup [:jesup] (needinfo me)]

jkeiser/et al - please decide if we need the newer patch for the 1.0 branch; we
expect to tag it Thursday for 1.0.2

Comment 102

[bsharma]

Verified on 7.0RTM build on Win2000

The demo works as expected.

Comment 103

[Daniel Veditz [:dveditz]]

If this has landed on the branch could you please change the mozilla1.0.2
keyword to fixed1.0.2 for verification? If it hasn't and doesn't need to land on
the branch then please remove the mozilla1.0.2 nomination.