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

VERIFIED FIXED

Status

()

Core
XUL
--
critical
VERIFIED FIXED
16 years ago
7 years ago

People

(Reporter: timeless, Assigned: mats)

Tracking

(4 keywords)

Trunk
crash, testcase, verified1.8.0.13, verified1.8.1.5
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9a1 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

16 years ago
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
      ...
    }
  }
(Reporter)

Updated

16 years ago
Keywords: crash

Comment 1

16 years ago
-> me
Assignee: hewitt → varga

Comment 2

16 years ago
related: bug 139656 ?

Comment 3

16 years ago
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

Comment 4

13 years ago
Created attachment 207889 [details]
testcase

Updated

13 years ago
Blocks: 320996
Keywords: testcase

Comment 5

13 years ago
Created attachment 207892 [details]
testcase without table related display type

just to make sure there are no tables involved in this thing ;-)
This just needs a null-check, no?
Flags: blocking1.9a1+
(Reporter)

Comment 7

13 years ago
Created attachment 214654 [details] [diff] [review]
null check?
Assignee: Jan.Varga → timeless
Status: NEW → ASSIGNED
Attachment #214654 - Flags: superreview?(neil)
Attachment #214654 - Flags: review?(neil)

Comment 8

13 years ago
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)
(Reporter)

Comment 10

13 years ago
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
(Reporter)

Updated

13 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Comment 11

13 years ago
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 → ---
(Reporter)

Comment 12

12 years ago
neil: any ideas? :(
(Assignee)

Comment 13

12 years ago
Created attachment 221388 [details] [diff] [review]
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().
Attachment #221388 - Flags: superreview?(neil)
Attachment #221388 - Flags: review?(bzbarsky)
(Assignee)

Updated

12 years ago
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+

Updated

12 years ago
Attachment #221388 - Flags: superreview?(neil) → superreview+

Updated

12 years ago
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]

Updated

12 years ago
Assignee: timeless → mats.palmgren
Status: REOPENED → NEW
(Assignee)

Comment 15

12 years ago
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
Last Resolved: 13 years ago12 years ago
Resolution: --- → FIXED
Created attachment 264111 [details] [diff] [review]
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?
Attachment #264111 - Flags: review?
Duplicate of this bug: 374103
Attachment #264111 - Flags: review? → review?(mats.palmgren)
(Assignee)

Comment 18

11 years ago
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!
Created attachment 264132 [details] [diff] [review]
combined branch patch
Attachment #264111 - Attachment is obsolete: true
Attachment #264132 - Flags: approval1.8.1.5?
(Assignee)

Comment 21

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

Good for 1.8.0 branch too
Attachment #264132 - Flags: approval1.8.0.13?
(Assignee)

Comment 22

11 years ago
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).
(Assignee)

Comment 24

11 years ago
Yes.  See bug 374102 comment 7 and forward.
(Assignee)

Updated

11 years ago
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+
(Assignee)

Comment 26

11 years ago
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
(Assignee)

Updated

11 years ago
Flags: in-testsuite?
Keywords: fixed1.8.0.13, fixed1.8.1.5

Comment 27

11 years ago
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
Keywords: fixed1.8.1.5 → verified1.8.1.5
This is verified fixed on 1.8.0.13 on Linux
Keywords: fixed1.8.0.13 → verified1.8.0.13
(Reporter)

Updated

10 years ago
Component: XP Toolkit/Widgets: Trees → XUL
QA Contact: shrir → xptoolkit.widgets

Comment 29

10 years ago
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.