Closed Bug 140218 Opened 19 years ago Closed 14 years ago

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

Categories

(Core :: XUL, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: timeless, Assigned: mats)

References

Details

(4 keywords)

Crash Data

Attachments

(4 files, 2 obsolete files)

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
      ...
    }
  }
Keywords: crash
-> me
Assignee: hewitt → varga
related: bug 139656 ?
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.
Severity: normal → critical
Attached file testcase
Blocks: xangle
Keywords: testcase
just to make sure there are no tables involved in this thing ;-)
This just needs a null-check, no?
Flags: blocking1.9a1+
Attached patch null check? (obsolete) — Splinter Review
Assignee: Jan.Varga → timeless
Status: NEW → ASSIGNED
Attachment #214654 - Flags: superreview?(neil)
Attachment #214654 - Flags: review?(neil)
Comment on attachment 214654 [details] [diff] [review]
null check?

Not sure that comment 6 counts as moa ;-)
Attachment #214654 - Flags: superreview?(neil)
Attachment #214654 - Flags: superreview+
Attachment #214654 - Flags: review?(neil)
Comment on attachment 214654 [details] [diff] [review]
null check?

Sure does
Attachment #214654 - Flags: review+
Comment on attachment 214654 [details] [diff] [review]
null check?

mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp 	1.267
Attachment #214654 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
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
...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
neil: any ideas? :(
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().
Attachment #221388 - Flags: superreview?(neil)
Attachment #221388 - Flags: review?(bzbarsky)
OS: Windows 2000 → All
Hardware: PC → All
Comment on attachment 221388 [details] [diff] [review]
Additional patch, rev. 1

r=bzbarsky.  Good to have you back, Mats!
Attachment #221388 - Flags: review?(bzbarsky) → review+
Attachment #221388 - Flags: superreview?(neil) → superreview+
Summary: Crash [@nsTreeBodyFrame::SetView] dereferencing box the QI result of a null mTreeBoxObject → Crash dereferencing box the QI result of a null mTreeBoxObject [@nsTreeBodyFrame::SetView] [@ nsTreeBodyFrame::GetMinSize]
Assignee: timeless → mats.palmgren
Status: REOPENED → NEW
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
Status: NEW → RESOLVED
Closed: 15 years ago14 years ago
Resolution: --- → FIXED
Attached patch combined branch patch (obsolete) — Splinter Review
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?
Attachment #264111 - Flags: review?
Duplicate of this bug: 374103
Attachment #264111 - Flags: review? → review?(mats.palmgren)
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.
Attachment #264111 - Flags: review?(mats.palmgren) → review+
Ah, right, I forgot about the NS_ENSURE_* macro warnings. I'll port an updated patch and request review, thanks!
Attachment #264111 - Attachment is obsolete: true
Attachment #264132 - Flags: approval1.8.1.5?
Comment on attachment 264132 [details] [diff] [review]
combined branch patch

Good for 1.8.0 branch too
Attachment #264132 - Flags: approval1.8.0.13?
Fixing bug 374102 is required before this goes on branch though...
(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).
Yes.  See bug 374102 comment 7 and forward.
Depends on: 374102
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
Attachment #264132 - Flags: approval1.8.1.5?
Attachment #264132 - Flags: approval1.8.1.5+
Attachment #264132 - Flags: approval1.8.0.13?
Attachment #264132 - Flags: approval1.8.0.13+
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
Flags: in-testsuite?
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.
Status: RESOLVED → VERIFIED
This is verified fixed on 1.8.0.13 on Linux
Component: XP Toolkit/Widgets: Trees → XUL
QA Contact: shrir → xptoolkit.widgets
Crashtest added as part of http://hg.mozilla.org/mozilla-central/rev/54417ebbaea2
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@nsTreeBodyFrame::SetView] [@ nsTreeBodyFrame::GetMinSize]
You need to log in before you can comment on or make changes to this bug.