Crash [@ nsFrameList::DestroyFrames] with menuitem, tooltip and setting some attributes

VERIFIED FIXED in mozilla1.9alpha8

Status

()

defect
--
critical
VERIFIED FIXED
12 years ago
8 years ago

People

(Reporter: martijn.martijn, Assigned: mats)

Tracking

(4 keywords)

Trunk
mozilla1.9alpha8
Points:
---
Bug Flags:
blocking1.8.1.8 +
wanted1.8.1.x +
wanted1.8.0.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?] using freed frame, crash signature)

Attachments

(4 attachments, 2 obsolete attachments)

Reporter

Description

12 years ago
Posted file testcase
See testcase, which crashes Mozilla within 200ms. It also crashes on branch, so I'm marking it security sensitive for now.

http://crash-stats.mozilla.com/report/index/709bf521-4925-11dc-a0a1-001a4bd46e84
0  	nsFrameList::DestroyFrames()  	 e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\layout\generic\nsframelist.cpp:67
1 	nsContainerFrame::Destroy() 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\layout\generic\nscontainerframe.cpp:252
2 	nsBoxFrame::RemoveFrame(nsIAtom*, nsIFrame*) 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\layout\xul\base\src\nsboxframe.cpp:1012
3 	nsFrameManager::RemoveFrame(nsIFrame*, nsIAtom*, nsIFrame*) 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\layout\base\nsframemanager.cpp:690
4 	nsCSSFrameConstructor::ContentRemoved(nsIContent*, nsIContent*, int, int) 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\layout\base\nscssframeconstructor.cpp:9607
5 	nsCSSFrameConstructor::RecreateFramesForContent(nsIContent*) 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\layout\base\nscssframeconstructor.cpp:11119
6 	nsCSSFrameConstructor::RestyleElement(nsIContent*, nsIFrame*, nsChangeHint) 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\layout\base\nscssframeconstructor.cpp:10022
7 	nsCSSFrameConstructor::ProcessOneRestyle(nsIContent*, nsReStyleHint, nsChangeHint) 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\layout\base\nscssframeconstructor.cpp:12925
8 	nsCSSFrameConstructor::ProcessPendingRestyles() 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\layout\base\nscssframeconstructor.cpp:12978
9 	nsCSSFrameConstructor::RestyleEvent::Run() 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\layout\base\nscssframeconstructor.cpp:13049
10 	nsThread::ProcessNextEvent(int, int*) 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\xpcom\threads\nsthread.cpp:490
11 	NS_ProcessNextEvent_P(nsIThread*, int) 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\obj-fx-trunk\xpcom\build\nsthreadutils.cpp:227
12 	nsBaseAppShell::Run() 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\widget\src\xpwidgets\nsbaseappshell.cpp:154
13 	nsAppStartup::Run() 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\toolkit\components\startup\src\nsappstartup.cpp:170
14 	XRE_main 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\toolkit\xre\nsapprunner.cpp:3059
15 	main 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\browser\app\nsbrowserapp.cpp:153
16 	WinMain 	e:\builds\tinderbox\fx-trunk\winnt_5.2_depend\mozilla\browser\app\nsbrowserapp.cpp:166
17 	__tmainCRTStartup 	f:\rtm\vctools\crt_bld\self_x86\crt\src\crtexe.c:578
18 	BaseProcessStart
Assignee

Updated

12 years ago
Assignee: nobody → mats.palmgren
OS: Windows XP → All
Hardware: PC → All
Assignee

Updated

12 years ago
Flags: blocking1.8.0.14?
Whiteboard: [sg:critical?] using freed frame
Assignee

Comment 1

12 years ago
Posted file stack + data
Assignee

Comment 2

12 years ago
The problem is that when we set the 'ordinal' attribute on a tooltip
we end up in nsBoxFrame::RelayoutChildAtOrdinal() with a frame that
is on the ::popupList frame list and this function assumes all
frames are on the principal list.  Later, as frames are destroyed,
this frame is on the "wrong list" so we crash.
Assignee

Comment 3

12 years ago
Posted patch Patch rev. 1 (diff -w) (obsolete) — Splinter Review
I don't think 'ordinal' makes much sense for frames on ::popupList
so maybe just we should just ignore it, like so?
Attachment #276613 - Flags: superreview?(bzbarsky)
Attachment #276613 - Flags: review?(bzbarsky)
Assignee

Updated

12 years ago
Attachment #276613 - Attachment description: Patch rev. 1 → Patch rev. 1 (diff -w)
Comment on attachment 276613 [details] [diff] [review]
Patch rev. 1 (diff -w)

>Index: layout/xul/base/src/nsBoxFrame.cpp
>+      // Don't bother with popup frames that lives on the nsGkAtoms::popupList -

s/lives/live/ ?

>+      // RelayoutChildAtOrdinal() only handles principal children.

So... are we sure we can never hit captions, legends, etc, etc, etc in here?

>+      if ((GetStateBits() & NS_FRAME_OUT_OF_FLOW) ||

Isn't that test backwards?

Neil should take a look a this.
Attachment #276613 - Flags: review?(bzbarsky) → review?(enndeakin)

Comment 5

12 years ago
Comment on attachment 276613 [details] [diff] [review]
Patch rev. 1 (diff -w)

>Index: layout/xul/base/src/nsBoxFrame.cpp

>+      // Don't bother with popup frames that lives on the nsGkAtoms::popupList -
>+      // RelayoutChildAtOrdinal() only handles principal children.
>+      if ((GetStateBits() & NS_FRAME_OUT_OF_FLOW) ||
>+          GetStyleDisplay()->mDisplay != NS_STYLE_DISPLAY_POPUP) {
> 

Yes, as bz said, that looks like it should skip out of flow frames, as ordinal wouldn't apply to them.

The earlier code which calls GetPlaceholderFrameFor could then be removed.
Assignee

Comment 6

12 years ago
(In reply to comment #4)
> So... are we sure we can never hit captions, legends, etc, etc, etc in here?

I don't think nsBoxFrame::AttributeChanged can be reached for those frames.
Popup frames are the only *box* frames that are kept on a different child
list as far as I know.  Either as out-of-flow, which is taken care of
by the few lines above the suggested change (fixed in bug 326529 BTW),
or as in-flow but then on ::popupList.
Now that I think about it, maybe we should just combine the two and just
check "GetStyleDisplay()->mDisplay != NS_STYLE_DISPLAY_POPUP" ...

Looking at the frame constructor it seems popups are the only
XUL frames which can be out-of-flow:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1389&root=/cvsroot&mark=6128-6129,6131#6128
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/base/nsCSSFrameConstructor.cpp&rev=1.1389&root=/cvsroot&mark=1344-1350,1353,1360#1325

> >+      if ((GetStateBits() & NS_FRAME_OUT_OF_FLOW) ||
> 
> Isn't that test backwards?

No, if 'this' is out-of-flow then 'frameToMove' is the placeholder there,
so the test passes through everything except in-flow popups.
Assignee

Comment 7

12 years ago
Posted patch Patch rev. 2 (obsolete) — Splinter Review
I guess we could just add !(GetStateBits() & NS_FRAME_OUT_OF_FLOW)
to the 'if' as Neil suggested and skip the assert, maybe that's
more robust against future changes...
Attachment #276633 - Flags: superreview?(bzbarsky)
Attachment #276633 - Flags: review?(enndeakin)
Assignee

Updated

12 years ago
Attachment #276613 - Attachment is obsolete: true
Attachment #276613 - Flags: superreview?(bzbarsky)
Attachment #276613 - Flags: review?(enndeakin)
Assignee

Comment 8

12 years ago
Posted patch Patch rev. 2bSplinter Review
Perhaps this is better after all... there's no need to make assumptions here...
Attachment #276633 - Attachment is obsolete: true
Attachment #276635 - Flags: superreview?(bzbarsky)
Attachment #276635 - Flags: review?(enndeakin)
Attachment #276633 - Flags: superreview?(bzbarsky)
Attachment #276633 - Flags: review?(enndeakin)

Updated

12 years ago
Attachment #276635 - Flags: review?(enndeakin) → review+
Comment on attachment 276635 [details] [diff] [review]
Patch rev. 2b

Yeah, I like this.  Applying ordinal to out-of-flow popups would just be weird.
Attachment #276635 - Flags: superreview?(bzbarsky) → superreview+
Assignee

Updated

12 years ago
Attachment #276635 - Flags: approval1.9?
Assignee

Updated

12 years ago
Flags: blocking1.8.1.7?
Comment on attachment 276635 [details] [diff] [review]
Patch rev. 2b

a1.9=dbaron
Attachment #276635 - Flags: approval1.9? → approval1.9+
The bug number never made it into the checkin comment.  Please do a commit -f followup or file a bug to get the comment cvs adminned?
Assignee

Comment 12

12 years ago
Oops, I'll do an empty commit as soon as the tree opens. Thanks.
Assignee

Comment 13

12 years ago
mozilla/layout/xul/base/src/nsBoxFrame.cpp 	1.337 

-> FIXED
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M8
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Reporter

Comment 14

12 years ago
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007082005 Minefield/3.0a8pre
Status: RESOLVED → VERIFIED
Flags: blocking1.8.1.7? → blocking1.8.1.7+
Does this patch work for the 1.8 branch or do we need a separate one?
Assignee

Comment 16

12 years ago
Posted patch Branch patchSplinter Review
Sync with trunk -- ignore 'ordinal' for out-of-flows and popups.
Compare with trunk:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/xul/base/src/nsBoxFrame.cpp&rev=1.338&root=/cvsroot&mark=1191-1206#1190

Fixes the crash on both branches.
Attachment #283302 - Flags: superreview?(bzbarsky)
Attachment #283302 - Flags: review?(bzbarsky)
Comment on attachment 283302 [details] [diff] [review]
Branch patch

r+sr=bzbarsky
Attachment #283302 - Flags: superreview?(bzbarsky)
Attachment #283302 - Flags: superreview+
Attachment #283302 - Flags: review?(bzbarsky)
Attachment #283302 - Flags: review+
Assignee

Updated

12 years ago
Attachment #283302 - Flags: approval1.8.1.8?
Attachment #283302 - Flags: approval1.8.0.14?
Comment on attachment 283302 [details] [diff] [review]
Branch patch

approved for 1.8.1.8 and 1.8.0.14, a=dveditz
Attachment #283302 - Flags: approval1.8.1.8?
Attachment #283302 - Flags: approval1.8.1.8+
Attachment #283302 - Flags: approval1.8.0.14?
Attachment #283302 - Flags: approval1.8.0.14+
Assignee

Comment 19

12 years ago
MOZILLA_1_8_BRANCH
mozilla/layout/xul/base/src/nsBoxFrame.cpp 	1.273.10.7

MOZILLA_1_8_0_BRANCH
mozilla/layout/xul/base/src/nsBoxFrame.cpp 	1.273.10.1.4.5
Flags: in-testsuite?
Assignee

Updated

12 years ago
Flags: blocking1.8.0.14?
Reporter

Comment 20

12 years ago
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007100304 Minefield/3.0a9pre

I can see the crash with the testcase using Firefox2.0.0.7.
Group: security
Verified for 1.8.0 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.14pre) Gecko/20071210 Firefox/1.5.0.13pre. I can see the crash with Firefox 1.5.0.12.

Comment 22

10 years ago
crash test landed
http://hg.mozilla.org/mozilla-central/rev/5a21084d3b9e
Flags: in-testsuite? → in-testsuite+
Crash Signature: [@ nsFrameList::DestroyFrames]
You need to log in before you can comment on or make changes to this bug.