167803 - Null pointer check for mContentTreeOwner in nsXULWindow::LoadWindowClassFromXUL

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Srilatha Moturi]

We need to check if mContentTreeOwner is null in 
nsXULWindow::LoadWindowClassFromXUL before dereferencing it. There is crash 
because mContentTreeOwner is null. See bugscape bug 19655
http://bugscape.netscape.com/show_bug.cgi?id=19655

Comment 1

[Srilatha Moturi]

Attachment: proposed patch

Comment 2

[Srilatha Moturi]

ccing danm and dougt for reviews.

Comment 3

[Dan M]

Comment on attachment 98627 [details] [diff] [review]
proposed patch

It looks to me that this usually doesn't crash because mContentTreeOwner
happens to have already been created for us by a quirk in one (of several)
paths in the window creation code. I think we want an active content tree owner
a little earlier on. I'd rather see:

Index: nsXULWindow.cpp
===================================================================
RCS file: /cvsroot/mozilla/xpfe/appshell/src/nsXULWindow.cpp,v
retrieving revision 1.117
diff -u -r1.117 nsXULWindow.cpp
--- nsXULWindow.cpp	7 Sep 2002 17:13:06 -0000	1.117
+++ nsXULWindow.cpp	10 Sep 2002 23:42:33 -0000
@@ -798,8 +798,9 @@
 {
   mChromeLoaded = PR_TRUE;

-  if(mContentTreeOwner)
-    mContentTreeOwner->ApplyChromeFlags();
+  NS_ENSURE_SUCCESS(EnsureContentTreeOwner(), NS_ERROR_FAILURE);
+
+  mContentTreeOwner->ApplyChromeFlags();

   LoadTitleFromXUL();
   LoadWindowClassFromXUL();

so that in the case where LoadWindowClassFromXUL is having problems we won't
skip applying the chromeflags, either.

But either is useful and will solve your problem. r=me.

Comment 4

[Srilatha Moturi]

Attachment: updated patch

since either one of the changes works for me, updated the patch so that we have
a better solution suggested by Dan.

Comment 5

[Srilatha Moturi]

Comment on attachment 98753 [details] [diff] [review]
updated patch

copying the r=danm from the previous patch

Comment 6

[Alec Flett]

Comment on attachment 98753 [details] [diff] [review]
updated patch

sr=alecf

Comment 7

[Alec Flett]

should we be adding an NS_ASSERTION() since this sounds like it only occurs in a
very specific case, that may be generating a strange path through the window
creation code?

Comment 8

[Srilatha Moturi]

Alec, I am not sure I understand your comment completely. Do you mean we should
be using NS_ASSERTION instead of NS_ENSURE_SUCCESS?

Comment 9

[racham]

Related changes landed on the trunk. 

Marking Fixed.