140218 - Crash dereferencing box the QI result of a null mTreeBoxObject [@nsTreeBodyFrame::SetView] [@ nsTreeBodyFrame::GetMinSize]

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[timeless]

cvs trunk w2k nondebug profile
-	box	{...}
\-	nsCOMPtr_base	{...}
 \+	mRawPtr	0x00000000
-	mTreeBoxObject	{...}
\-	nsCOMPtr_base	{...}
 \+	mRawPtr	0x00000000

    box->SetPropertyAsSupports(view.get(), mView);

nsTreeBodyFrame::SetView(nsTreeBodyFrame * const 0x0273f000, nsITreeView * 
0x00000000) line 654 + 9 bytes
nsTreeBodyFrame::GetPrefSize(nsTreeBodyFrame * const 0x02775c48, 
nsBoxLayoutState & {...}, nsSize & {...}) line 442
nsBox::GetAscent(nsBox * const 0x0273eff0, nsBoxLayoutState & {...}, int &) 
line 1032
nsSprocketLayout::GetAscent(nsSprocketLayout * const 0x01adb530, nsIBox * 
0x0273eff0, nsBoxLayoutState & {...}, int &) line 1521
nsContainerBox::GetAscent(nsContainerBox * const 0x0273ef1c, nsBoxLayoutState & 
{...}, int &) line 591
nsBoxFrame::GetAscent(nsBoxFrame * const 0x0273ef1c, nsBoxLayoutState & {...}, 
int & 0) line 1101
nsSprocketLayout::GetAscent(nsSprocketLayout * const 0x01adb530, nsIBox * 
0x0273ef1c, nsBoxLayoutState & {...}, int &) line 1521
nsContainerBox::GetAscent(nsContainerBox * const 0x0273ee3c, nsBoxLayoutState & 
{...}, int &) line 591
nsBoxFrame::GetAscent(nsBoxFrame * const 0x0273ee3c, nsBoxLayoutState & {...}, 
int & 0) line 1101
nsSprocketLayout::GetAscent(nsSprocketLayout * const 0x01adb530, nsIBox * 
0x0273ee3c, nsBoxLayoutState & {...}, int &) line 1521
nsContainerBox::GetAscent(nsContainerBox * const 0x0276532c, nsBoxLayoutState & 
{...}, int &) line 591
nsBoxFrame::GetAscent(nsBoxFrame * const 0x0276532c, nsBoxLayoutState & {...}, 
int & 0) line 1101
nsSprocketLayout::GetAscent(nsSprocketLayout * const 0x01adb530, nsIBox * 
0x0276532c, nsBoxLayoutState & {...}, int &) line 1521
nsContainerBox::GetAscent(nsContainerBox * const 0x0275ab98, nsBoxLayoutState & 
{...}, int &) line 591
nsBoxFrame::GetAscent(nsBoxFrame * const 0x0275ab98, nsBoxLayoutState & {...}, 
int & 0) line 1101
nsSprocketLayout::Layout(nsSprocketLayout * const 0x01adb530, nsIBox * 
0x0275ab98, nsBoxLayoutState & {...}) line 245
nsContainerBox::DoLayout(nsContainerBox * const 0x0275ab98, nsBoxLayoutState & 
{...}) line 606 + 8 bytes
nsBox::Layout(nsBox * const 0x0275ab98, nsBoxLayoutState & {...}) line 1052
nsPopupSetFrame::DoLayout(nsPopupSetFrame * const 0x0253c90c, nsBoxLayoutState 
& {...}) line 253
nsBox::Layout(nsBox * const 0x0253c90c, nsBoxLayoutState & {...}) line 1052
nsSprocketLayout::Layout(nsSprocketLayout * const 0x01adb530, nsIBox * 
0x0253c7f8, nsBoxLayoutState & {...}) line 529
nsContainerBox::DoLayout(nsContainerBox * const 0x0253c7f8, nsBoxLayoutState & 
{...}) line 606 + 8 bytes
nsBox::Layout(nsBox * const 0x0253c7f8, nsBoxLayoutState & {...}) line 1052
nsStackLayout::Layout(nsStackLayout * const 0x01bd0998, nsIBox * 0x0253c618, 
nsBoxLayoutState & {...}) line 331
nsContainerBox::DoLayout(nsContainerBox * const 0x0253c618, nsBoxLayoutState & 
{...}) line 606 + 8 bytes
nsBox::Layout(nsBox * const 0x0253c618, nsBoxLayoutState & {...}) line 1052
nsBoxFrame::Reflow(nsBoxFrame * const 0x0253c5e4, nsIPresContext * 0x01b1dd98, 
nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, unsigned int & 0) 
line 1001
nsRootBoxFrame::Reflow(nsRootBoxFrame * const 0x0253c5e4, nsIPresContext * 
0x01b1dd98, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0) line 243
nsContainerFrame::ReflowChild(nsContainerFrame * const 0x00000000, nsIFrame * 
0x0253c5e4, nsIPresContext * 0x01b1dd98, nsHTMLReflowMetrics & {...}, const 
nsHTMLReflowState & {...}, int 0, int 0, unsigned int 0, unsigned int & 0) line 
807
ViewportFrame::Reflow(ViewportFrame * const 0x0253c5ac, nsIPresContext * 
0x01b1dd98, nsHTMLReflowMetrics & {...}, const nsHTMLReflowState & {...}, 
unsigned int & 0) line 588
nsHTMLReflowCommand::Dispatch(nsHTMLReflowCommand * const 0x00000000, 
nsIPresContext * 0x01b1dd98, nsHTMLReflowMetrics & {...}, const nsSize & {...}, 
nsIRenderingContext & {...}) line 224
PresShell::ProcessReflowCommand(PresShell * const 0x00000000, nsVoidArray & 
{...}, int 0, nsHTMLReflowMetrics & {...}, nsSize & {...}, nsIRenderingContext 
& {...}) line 6189
PresShell::ProcessReflowCommands(PresShell * const 0x00000000, int 0) line 6244
PresShell::FlushPendingNotifications(PresShell * const 0x00000001, int 0) line 
5049
nsXULDocument::FlushPendingNotifications(nsXULDocument * const 0x01ae9968, int 
1, int 0) line 2501 + 13 bytes
nsXBLResourceLoader::NotifyBoundElements(nsXBLResourceLoader * const 
0x00000000) line 281
nsXBLResourceLoader::StyleSheetLoaded(nsXBLResourceLoader * const 0x02702108, 
nsICSSStyleSheet * 0x02702108, int 1) line 207
CSSLoaderImpl::InsertSheetInDoc(CSSLoaderImpl * const 0x00000000, 
nsICSSStyleSheet * 0x0272b8d8, int 2, nsIContent * 0x00000000, int 1, 
nsICSSLoaderObserver * 0x027699a8) line 1194 + 10 bytes
InsertPendingSheet(void * 0x02702038, void * 0x02683ba0) line 757
nsStringArray::EnumerateForwards(nsStringArray * const 0x00000000, int 
(nsString &, void *)* 0x01e03876 InsertPendingSheet(void *, void *), void * 
0x02683ba0) line 660 + 11 bytes
CSSLoaderImpl::Cleanup(CSSLoaderImpl * const 0x00000000, URLKey & {...}, 
SheetLoadData * 0x0276eed0) line 821
CSSLoaderImpl::SheetComplete(CSSLoaderImpl * const 0x00000000, nsICSSStyleSheet 
* 0x00000000, SheetLoadData * 0x0276eed0) line 914
CSSLoaderImpl::ParseSheet(CSSLoaderImpl * const 0x00000000, 
nsIUnicharInputStream * 0x02731168, SheetLoadData * 0x00000000, int & 1, 
nsICSSStyleSheet * & 0x01eb96d8 const  CSSParserImpl::`vftable') line 949
CSSLoaderImpl::DidLoadStyle(CSSLoaderImpl * const 0x00000000, nsIStreamLoader * 
0x0276edd0, nsString * 0x00000001, SheetLoadData * 0x0272b9e8, unsigned int 
41095528) line 985
SheetLoadData::OnStreamComplete(SheetLoadData * const 0x0276eed0, 
nsIStreamLoader * 0x0276edd0, nsISupports * 0x00000000, unsigned int 0, 
unsigned int 6675, const char * 0x0277e890) line 745
nsStreamLoader::OnStopRequest(nsStreamLoader * const 0x00000000, nsIRequest * 
0x0276eed0, nsISupports * 0x00000000, unsigned int 0) line 163
nsJARChannel::OnStopRequest(nsJARChannel * const 0x02667f44, nsIRequest * 
0x0274c55c, nsISupports * 0x00000000, unsigned int 0) line 606 + 28 bytes
nsOnStopRequestEvent::HandleEvent(nsOnStopRequestEvent * const 0x00000000) line 
213
PL_HandleEvent(PLEvent * 0x02727e94) line 597
PL_ProcessPendingEvents(PLEventQueue * 0x1003342f) line 526 + 6 bytes
_md_EventReceiverProc(HWND__ * 0x00cb6128, unsigned int 4202408, unsigned int 
13196728, long 2013534116) line 1078
nsAppShellService::Run(nsAppShellService * const 0x00c95db8) line 451
main1(int 3, char * * 0x00262ed8, nsISupports * 0x00262f20) line 1431 + 9 bytes
main(int 3, char * * 0x00262ed8) line 1779 + 26 bytes
WinMain(HINSTANCE__ * 0x00400000, HINSTANCE__ * 0x00400000, char * 0x001334b9, 
HINSTANCE__ * 0x00400000) line 1799 + 23 bytes
MOZILLA! WinMainCRTStartup + 308 bytes
KERNEL32! 77e97d08()

The problem is in nsTreeBodyFrame::GetPrefSize(nsTreeBodyFrame * const 
0x02775c48, nsBoxLayoutState & {...}, nsSize & {...}) line 442

The code is basically:
  if (!mView) {
    EnsureBoxObject();
    nsCOMPtr<nsIBoxObject> box = do_QueryInterface(mTreeBoxObject);
    if (box) {
      ...
    }
    if (!mView) {
      ...
        if (view)
          SetView(view); // crashes because !box
      ...
    }
  }

Comment 1

[Jan Varga [:janv]]

-> me

Comment 2

[Olivier Cahagne]

related: bug 139656 ?

Comment 3

[Brant Gurganus]

By the definitions on <http://bugzilla.mozilla.org/bug_status.html#severity> and
<http://bugzilla.mozilla.org/enter_bug.cgi?format=guided>, crashing and dataloss
bugs are of critical or possibly higher severity.  Only changing open bugs to
minimize unnecessary spam.  Keywords to trigger this would be crash, topcrash,
topcrash+, zt4newcrash, dataloss.

Comment 4

[Bernd]

Attachment: testcase

Comment 5

[Bernd]

Attachment: testcase without table related display type

just to make sure there are no tables involved in this thing ;-)

Comment 6

[Boris Zbarsky [:bzbarsky]]

This just needs a null-check, no?

Comment 7

[timeless]

Attachment: null check?

Comment 8

[[e-mail]]

Comment on attachment 214654 [details] [diff] [review]
null check?

Not sure that comment 6 counts as moa ;-)

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 214654 [details] [diff] [review]
null check?

Sure does

Comment 10

[timeless]

Comment on attachment 214654 [details] [diff] [review]
null check?

mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp 	1.267

Comment 11

[Jesse Ruderman]

I still see a crash with both testcases.  (Testing on Mac with a tinderbox build and my own debug build.)

New stack:

0    nsTreeBodyFrame::GetMinSize(nsBoxLayoutState&, nsSize&) + 44
1    nsBox::GetPrefSize(nsBoxLayoutState&, nsSize&) + 156
2    nsBox::GetAscent(nsBoxLayoutState&, int&) + 112
3    nsSprocketLayout::GetAscent(nsIFrame*, nsBoxLayoutState&, int&) + 156
4    nsBoxFrame::GetAscent(nsBoxLayoutState&, int&) + 156
5    nsSprocketLayout::Layout(nsIFrame*, nsBoxLayoutState&) + 432
6    nsBoxFrame::DoLayout(nsBoxLayoutState&) + 68
...

Comment 12

[timeless]

neil: any ideas? :(

Comment 13

[Mats Palmgren (inactive)]

Attachment: Additional patch, rev. 1

GetBaseElement() return null if there is no <select> or <tree> ancestor:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp&rev=1.275&root=/cvsroot&mark=3873-3875#3865

This patch adds the missing null checks for all calls to GetBaseElement().

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 221388 [details] [diff] [review]
Additional patch, rev. 1

r=bzbarsky.  Good to have you back, Mats!

Comment 15

[Mats Palmgren (inactive)]

Checked in "Additional patch, rev. 1" to trunk at 2006-06-03 09:57 PDT:

Checking in layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp;
/cvsroot/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp,v  <--  nsTreeBodyFrame.cpp
new revision: 1.278; previous revision: 1.277
done

Comment 16

[:Gavin Sharp [email: [e-mail]]]

Attachment: combined branch patch

This is a combination of attachment 221388 [details] [diff] [review] and attachment 214654 [details] [diff] [review], minus the change to GetScrollParts(), because I don't understand why it's needed (GetPrimaryFrameFor seems to null check it's argument and return null as needed, on branch and trunk).

Mats, this should be fine for the branch, right?

Comment 18

[Mats Palmgren (inactive)]

Comment on attachment 264111 [details] [diff] [review]
combined branch patch

(In reply to comment #16)
> ... because I don't understand why it's needed

One shouldn't call GetPrimaryFrameFor with null content -
it will result in a warning in debug builds here:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsFrameManager.cpp&rev=1.245&root=/cvsroot&mark=324#320

> Mats, this should be fine for the branch, right?

Yep, adding null-checks is pretty safe and this has baked a long time.

r=mats, but please also include the change to GetScrollParts() -
not because a warning more or less means much on branch, but rather to
keep the code differences to a minimum.  In general we should merge
patches as is without changes (unless there's a specific reason not to).
It also lowers the risk of accidental mistakes in the process.

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Ah, right, I forgot about the NS_ENSURE_* macro warnings. I'll port an updated patch and request review, thanks!

Comment 20

[:Gavin Sharp [email: [e-mail]]]

Attachment: combined branch patch

Comment 21

[Mats Palmgren (inactive)]

Comment on attachment 264132 [details] [diff] [review]
combined branch patch

Good for 1.8.0 branch too

Comment 22

[Mats Palmgren (inactive)]

Fixing bug 374102 is required before this goes on branch though...

Comment 23

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #22)
> Fixing bug 374102 is required before this goes on branch though...

Are you sure? Bug 374103 was filed because bug 374102 supposedly didn't affect the branch (I can't reproduce it, anyways).

Comment 24

[Mats Palmgren (inactive)]

Yes.  See bug 374102 comment 7 and forward.

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 264132 [details] [diff] [review]
combined branch patch

approved for 1.8.1.5 and 1.8.0.13, a=dveditz for release-drivers

Comment 26

[Mats Palmgren (inactive)]

Comment on attachment 264132 [details] [diff] [review]
combined branch patch

Checked in to MOZILLA_1_8_BRANCH:
layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:  1.240.10.10

Checked in to MOZILLA_1_8_0_BRANCH:
layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:  1.240.10.1.2.9

Comment 27

[Adam Guthrie]

Verified FIXED using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.5pre) Gecko/20070710 BonEcho/2.0.0.5pre.

Tested using a build from 2007-06-29-04 without Mats's patch and crashed on the first testcase and hung on the second, but using a build with the patch I don't crash or hang with either testcase.

Comment 28

[u279076]

This is verified fixed on 1.8.0.13 on Linux

Comment 29

[Jesse Ruderman]

Crashtest added as part of http://hg.mozilla.org/mozilla-central/rev/54417ebbaea2