Closed Bug 164086 Opened 18 years ago Closed 18 years ago

File upload vulnerability using event.rangeParent


(Core :: Security, defect, P1)

Windows 2000





(Reporter: jruderman, Assigned: john)



(Keywords: topembed+, Whiteboard: [adt1 RTM] [ETA 08/23] [FIX])


(7 files, 14 obsolete files)

298 bytes, text/html
12.58 KB, patch
: review+
: superreview+
Details | Diff | Splinter Review
7.78 KB, patch
: review+
: approval+
Details | Diff | Splinter Review
5.17 KB, patch
: review+
: superreview+
Details | Diff | Splinter Review
22.97 KB, patch
: review+
Details | Diff | Splinter Review
15.45 KB, patch
Details | Diff | Splinter Review
594 bytes, patch
Details | Diff | Splinter Review
This is file upload hole smiilar to bug 163598 (event.originalTarget) and 
bug 164023 (event.relatedTarget).  Does anyone know what event.rangeParent is?
Attached file demo
cc everyone from bug 164023.
A commercial build from today that has the fix for bug 163598 is still
vulnerable to this bug.
reassigning to jkeiser.
Assignee: mstoltz → jkeiser
Blocks: 143047
Priority: -- → P1
Whiteboard: [adt1 RTM] [ETA Needed]
Target Milestone: --- → mozilla1.0.1
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.
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)
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.

looks like my friend.
Since rangeParent and rangeOffset are only used internally, can they be made
inaccessible to scripts?
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.
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).

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
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
Keywords: adt1.0.1+
Whiteboard: [adt1 RTM] [ETA Needed] → [adt1 RTM] [ETA 08/23]
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
(  Google shows
quite a few occurances

No occurances of rangeParent though.
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 :)
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

Regarding relatedTarget however, it is a standard and I don't have access to bug
164023 (PITA!). You *can not remove relatedTarget*.
this patch makes IsAnonymous() work in nsIContent.  JST has the second part of
this working (in ClassInfo), posting for transfer
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.
bclary: you now have access to bug 164023. :-)
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.
jband: yes, AFAICT it should be in FindTearOff for this to work right.
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.
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.
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
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 
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
... 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>. 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.
Please to be noting that I posted that text from the patched build.  Scrollbar
and submit and everything worked fine.
Blocks: 163598, 164023
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...
... minus all the typos in the above comment :-)
Attached patch Patch using CanAccess (obsolete) — Splinter Review
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
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?
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).
Attached patch Patch using CanAccess (v1.1) (obsolete) — Splinter Review
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.
Attachment #96417 - Attachment is obsolete: true
Attached patch Patch using CanAccess (v1.1.1) (obsolete) — Splinter Review
OK, this one doesn't have ^M's and doesn't change nsVoidArray :)
Attachment #96420 - Attachment is obsolete: true
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.
Attached patch Patch using CanAccess (v1.1.2) (obsolete) — Splinter Review
This version also contains the nsSecurityScriptManager.cpp, which is rather
important to this patch.
Attachment #96421 - Attachment is obsolete: true
Attached patch Patch using CanAccess (v1.1.3) (obsolete) — Splinter Review
I have a new disorder, a patch impediment.  I stutter patches.
Attachment #96423 - Attachment is obsolete: true
cc'ing tor who can hopefully sr= this patch.
Comment on attachment 96425 [details] [diff] [review]
Patch using CanAccess (v1.1.3)

>+nsXULElement::IsAnonymous() const
>+  return PR_FALSE;
>+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/
>@@ -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
Attachment #96425 - Flags: review+
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.
Fix sicking's review
Attachment #96425 - Attachment is obsolete: true
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 on attachment 96435 [details] [diff] [review]
Backout of bug 163593

JST sr'd, and chofmann a='d in bug 163593.
Attachment #96435 - Flags: superreview+
Attachment #96435 - Flags: approval+
eek, bug 163598 is the bug being backed out.
Comment on attachment 96434 [details] [diff] [review]
Patch using CanAccess (v1.1.4)

Attachment #96434 - Flags: review+
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.

>-// The number of bits to shift the bit field to get at the content ID
>+/** Whether this content is anonymous */

Same javadoc issue here.  I notice you do this a few other places below as
well.  Fix those too.
> 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 ;)
Attached patch Branch Necessities (obsolete) — Splinter Review
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 :)
Just saw bryner awake, nagging him for sr=.
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?).
Attached patch Branch Necessities v2.0 (obsolete) — Splinter Review
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.
Attachment #96438 - Attachment is obsolete: true
+  // 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)
Attachment #96439 - Attachment is obsolete: true
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.
Attachment #96434 - Flags: superreview+
Comment on attachment 96441 [details] [diff] [review]
Branch Necessities v2.1

<bryner> you can apply my sr to the branch patch too
Attachment #96441 - Flags: superreview+
Attachment #96441 - Flags: review+
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, 
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

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.
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, 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!
Keywords: fixed1.0.1
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?
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.

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. :)
UAAnonymous?  Yuck.

How about NativeAnonymous?

Yeah, that sounds good.
Attached patch roll-up patch for 1.1 branch (obsolete) — Splinter Review
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.
sorry, forgot to read the comments before uploading.
Attachment #96468 - Attachment is obsolete: true
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.
Attachment #96473 - Flags: superreview+
Attachment #96473 - Flags: review+
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
Attachment #96473 - Flags: approval+
building 1.1 to test now; this could take a while.
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.
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.
Whiteboard: [adt1 RTM] [ETA 08/23] → [adt1 RTM] [ETA 08/23] [FIX]
hey, jkeiser? where does SetFlags come from? my 1.1 tree is failing to build
with the patches that use SetFlags and UnsetFlags.
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.
Closed: 18 years ago
Resolution: --- → WORKSFORME
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
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.
Resolution: WORKSFORME → ---
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 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 :)
Attachment #96502 - Flags: review+
folks,  go have a look at and
see if I'm smoking too much of something, or if this is something that we might
expect and can live with......
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

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...
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.
That was me talking, the real me! :-)

Or maybe nsIClassInfo::DOM_NODE? I'm feeling a lot better about this already.
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.
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 :-)
Attached patch Patch using CanAccess (v1.1.5) (obsolete) — Splinter Review
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.
Attachment #96386 - Attachment is obsolete: true
Attachment #96398 - Attachment is obsolete: true
Attachment #96473 - Attachment is obsolete: true
Attached patch Patch using CanAccess (v1.2) (obsolete) — Splinter Review
This makes IsAnonymous() -> IsNativeAnonymous() like it's supposed to :)
Attachment #96522 - Attachment is obsolete: true
Attached patch Patch using CanAccess (v1.2.1) (obsolete) — Splinter Review
Follow jst's suggestion of having a class to hold the classinfo flags (to avoid
double virtual calls).
Attachment #96531 - Attachment is obsolete: true
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? :-)

+    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.
Attachment #96533 - Flags: superreview+
Fix JST's nits, and a leetle problem with that new class not working at all due
to uninitialized memory.
Attachment #96533 - Attachment is obsolete: true
Comment on attachment 96537 [details] [diff] [review]
Patch using CanAccess (v1.2.2)

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

- 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.

Attachment #96537 - Flags: superreview+
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.
*** Bug 163768 has been marked as a duplicate of this bug. ***
Comment on attachment 96537 [details] [diff] [review]
Patch using CanAccess (v1.2.2)


you might wanna add an

// XXX need to fix this, see bug YYY

in nsXULElement::SetNativeAnonymous and a couple of other places
Blocks: 165110
Fix checked in to trunk.  leaf checked in to 1.1 a couple of days ago.  Only
0.9.4 left.
Closed: 18 years ago18 years ago
Resolution: --- → FIXED
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.
Resolution: FIXED → ---
Fix checked in again earlier.  It stuck.
Closed: 18 years ago18 years ago
Resolution: --- → FIXED
This is the patch against 0.9.4.
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.
Keywords: mozilla1.2
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?
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.
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
Verified on 7.0RTM build on Win2000

The demo works as expected.
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.
Keywords: adt1.0.1+
Group: security
You need to log in before you can comment on or make changes to this bug.