257607 - nsScrollBoxObject::::EnsureElementIsVisible dereferences a null pointer if element is not a nsIDOMXULElement

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Justin]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040619 Firefox/0.9
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040619 Firefox/0.9

The nsIScrollBoxObject interface constrains the parameter to a nsIDOMElement. In
the funtion body, a QueryInterface is done without a check to determine if it
was successful. After a failed QI a dereference of a null nsCOMPtr<> is performed.

lxr url where the dereference of a null pointer happens:
http://lxr.mozilla.org/seamonkey/source/layout/xul/base/src/nsScrollBoxObject.cpp#320



Reproducible: Always
Steps to Reproduce:

Comment 1

[Justin]

Attachment: Patch to fix for 1.7 branch

Fixes null deref by checking for QI success/failure (see comments in patch)

Comment 2

[Boris Zbarsky [:bzbarsky]]

I don't think this is the right fix (as in, it does fix the crash, but I don't
think it does it the right way).  It would be preferable to use GetBoxObjectFor
on the document's nsIDOMNSDocument interface (which is what
nsXULElement::GetBoxObject ends up doing anyway), and then bail (with error, or
without) if the box object is null.

Note also that both callers of GetBoxObject in this code leak the box object by
not using an nsCOMPtr... Fixing that while fixing the call that gets the box
object would be nice.

Justin, do you feel up to making those changes?

Comment 3

[Justin]

Basically, what should be happening is replacing all raw pointer with nsCOMPtr<>
and using a QI on the nsIDOMDocument from child parameter. 

Will give it a spin.

I am using the 1.7 branch though.  Will try to get a patch for trunk too

Comment 4

[Justin]

Attachment: using nsCOMPtr instead of raw pointers

Moved almost all the raw pointers to nsCOMPtr. nsIFrame can't be moved,
Release() is private. On a quick glance around, a lot of code needs this work
done on it.

I am still hunting for a way to get to nsIDOMNSDocument without going through
nsIDOMXULElement. Though returning on QI failure and catching the exception in
JS allows me to move forward for now.

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 157586 [details] [diff] [review]
using nsCOMPtr instead of raw pointers

please do NOT store views and frames in comptrs. they are not refcounted. views
will "soon" not have addref/release anymore.

+    nsCOMPtr<nsIScrollableView> scrollableView(GetScrollableView());
+    nsCOMPtr<nsIBox> box(do_QueryInterface(frame));


+    nsresult childQiRv=NS_OK;

any raeson not to name this rv?

Comment 6

[Boris Zbarsky [:bzbarsky]]

> I am still hunting for a way to get to nsIDOMNSDocument

You have an nsIDOMElement, no?  You get its ownerDocument and then QI that.

Also, nsIBox is not refcounted (just like nsIFrame and nsIView).

Comment 7

[Justin]

Attachment: using nsIDOMNSDocument to get box

- Removed frivolous nsCOMPtr's on boxes, frames and views.
- Performing lots of failure checks, maybe too many
- Finally using nsIDOMNSDocument to get box (forgot the include earlier)

If there are no issues with this patch, I will go ahead and do the same to
nsScrollBoxObject::ScrollToElement

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 157635 [details] [diff] [review]
using nsIDOMNSDocument to get box

>Index: nsScrollBoxObject.cpp

>+  //yes, I like my javascript calls to continue functioning
>+    NS_ASSERTION(child,"null child parameter value");
>+    if(!child)
>+      return NS_ERROR_NULL_POINTER;

No need to assert, and just use:

  NS_ENSURE_ARG_POINTER(child);

>-       return NS_ERROR_FAILURE;
>+      return NS_ERROR_FAILURE;

Don't change the file's indentation, please (and follow the 4-space indentation
in the code you add).

>+    rv = mPresShell->GetPresContext(getter_AddRefs(context));

No need to check rv here.  In fact, on trunk the method just returns a
prescontext, not an nsresult.

>-    nsIFrame* frame = GetFrame();
>-    nsIBox* box;
>-    CallQueryInterface(frame, &box);
>+    nsIFrame *frame = GetFrame();

Why change the code that was already there (moved the space)?

>+    nsIBox *box=0;
>+    rv = CallQueryInterface(frame, &box);

I believe the frame for a nsScrollBoxObject will always be a boxframe.	It may
be worth asserting success here, but it's not worth checking for it.

>+    nsIBox *scrolledBox;

Why change the code that was already there (moved the space)?

>+    nsCOMPtr<nsIDOMDocument> doc;
>+    nsIDOMNSDocument *docNs=0;
>+    
>+    rv = child->GetOwnerDocument(getter_AddRefs(doc));
>+    if(NS_FAILED(rv) || !doc)

You can just check for !doc and return NS_ERROR_UNEXPECTED when that happens. 
But see below.

>+    rv = CallQueryInterface(doc,&docNs);

You're leaking docNs.  Use an nsCOMPtr and do_QueryInterface.  Since that's
null-safe, skip checking for !doc and just check for !docNs.

>+    nsIBoxObject * childBoxObject=0;

And you're _still_ leaking the box object.  Wasn't not leaking this the reason
for all the nsCOMPtr changes?

Comment 9

[Justin]

Attachment: fixed from comments

Ok, I think this one is a winner. 
Implemented all the changes (and yes that wonderful oversight). Thnx for having
patience with me on this.

If this one is good, then I will do the samething to the other function.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Yeah, that looks good (if you remove the random empty line you added and add
spaces after commas in argument lists in both the assert and the
GetBoxObjectFor() call).

Comment 11

[Justin]

Attachment: Fixed whitespace, added fix to ScrollToElement

Fixed whitespace and commas
Added same fixes to nsScrollBoxObject::ScrollToElement as well

Comment 12

[Boris Zbarsky [:bzbarsky]]

Justin, in general you need to set review requests on patches if you want
someone to review them. I've been keeping track of this one, but it's easy for
normal bugmail to get lost in the shuffle....

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 157684 [details] [diff] [review]
Fixed whitespace, added fix to ScrollToElement

Please use "cvs diff -pu6" as a good starting point for diffs (more context,
and better labeling of where each hunk applies).  It makes diffs easier to
review.

>Index: nsScrollBoxObject.cpp

> #include "nsIDOMXULElement.h"

Is that include still needed?  I don't think it is...

> NS_IMETHODIMP nsScrollBoxObject::ScrollToElement(nsIDOMElement *child)
> {
>+    NS_ENSURE_ARG(child);

NS_ENSURE_ARG_POINTER.

With those two issues fixed, r+sr=bzbarsky; please attach a patch with those
changes and look for someone to check this in on irc?

Comment 14

[Justin]

Attachment: removed include and fixed ensure arg macro

Fixed the two remaining issues. Will now look at trunk and add a patch for
that. Does the trunk patch require a separate bug?

Comment 15

[Boris Zbarsky [:bzbarsky]]

No.  In fact, the trunk patch is what should be going into the tree.... once
that's tested, you can ask for branch approval.

Comment 16

[Justin]

Attachment: fixes applied to trunk

This is essentially the same fix, but applied to trunk instead of the 1.7
branch.

Comment 17

[[e-mail]]

Random comment: I would have used nsDoc rather than docNs

Comment 18

[Justin]

Attachment: changed docNs to nsDoc on trunk patch

Comment 19

[Justin]

Attachment: changed docNs to nsDoc on 1.7 branch patch

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 158524 [details] [diff] [review]
changed docNs to nsDoc on 1.7 branch patch

r+sr=bzbarsky.	I think it's worth taking this on branches... it fixes a crash
and a leak and is very low-risk (the only behavior change for the cases that
didn't crash, is that they no longer leak).

Comment 21

[Boris Zbarsky [:bzbarsky]]

Checked in for 1.8a4.  Marking fixed (I'll land this on branches if/when it gets
approved).

Comment 22

[chris hofmann]

lets take this on the aviary branch after we open up from the PR freexe next
week some time.

Comment 23

[Asa Dotzler [:asa]]

Comment on attachment 158524 [details] [diff] [review]
changed docNs to nsDoc on 1.7 branch patch

a=asa (on behalf of the aviary team and drivers) for checkin to the aviary
branch and the 1.7 branch.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Fixed on branches.