The default bug view has changed. See this FAQ.

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)

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)

Updated

10 years ago
Assignee: nobody → mats.palmgren
OS: Windows XP → All
Hardware: PC → All
(Assignee)

Updated

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

Comment 1

10 years ago
Created attachment 276611 [details]
stack + data
(Assignee)

Comment 2

10 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

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

Updated

10 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

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 -
>+      // 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

10 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

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

Updated

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

Comment 8

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

Updated

10 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

10 years ago
Attachment #276635 - Flags: approval1.9?
(Assignee)

Updated

10 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

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

Comment 13

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

Comment 16

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

10 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

10 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?
Keywords: fixed1.8.0.14, fixed1.8.1.8
(Assignee)

Updated

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