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

VERIFIED FIXED in mozilla1.9alpha8

Status

()

Core
Layout
--
critical
VERIFIED FIXED
10 years ago
6 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: Mats Palmgren (vacation - back in August))

Tracking

(4 keywords)

Trunk
mozilla1.9alpha8
crash, testcase, verified1.8.0.14, verified1.8.1.8
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

10 years ago
Created attachment 276424 [details]
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: nobody → mats.palmgren
OS: Windows XP → All
Hardware: PC → All
Flags: blocking1.8.0.14?
Whiteboard: [sg:critical?] using freed frame
Created attachment 276611 [details]
stack + data
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.
Created attachment 276613 [details] [diff] [review]
Patch rev. 1 (diff -w)

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)
Attachment #276613 - Attachment description: Patch rev. 1 → Patch rev. 1 (diff -w)

Comment 4

10 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 -

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 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.
(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.
Created attachment 276633 [details] [diff] [review]
Patch rev. 2

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)
Attachment #276613 - Attachment is obsolete: true
Attachment #276613 - Flags: superreview?(bzbarsky)
Attachment #276613 - Flags: review?(enndeakin)
Created attachment 276635 [details] [diff] [review]
Patch rev. 2b

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)
Attachment #276635 - Flags: review?(enndeakin) → review+

Comment 9

10 years ago
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+
Attachment #276635 - Flags: approval1.9?
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+

Comment 11

10 years ago
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?
Oops, I'll do an empty commit as soon as the tree opens. Thanks.
mozilla/layout/xul/base/src/nsBoxFrame.cpp 	1.337 

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

Comment 14

10 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?
Created attachment 283302 [details] [diff] [review]
Branch patch

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 17

10 years ago
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+
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+
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?
Keywords: fixed1.8.0.14, fixed1.8.1.8
Flags: blocking1.8.0.14?
(Reporter)

Comment 20

10 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.
Keywords: fixed1.8.1.8 → verified1.8.1.8
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.
Keywords: fixed1.8.0.14 → verified1.8.0.14

Comment 22

8 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.