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

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

Attachment: 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

Comment 1

[Mats Palmgren (inactive)]

Attachment: stack + data

Comment 2

[Mats Palmgren (inactive)]

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.

Comment 3

[Mats Palmgren (inactive)]

Attachment: 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?

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[Neil Deakin]

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.

Comment 6

[Mats Palmgren (inactive)]

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

Comment 7

[Mats Palmgren (inactive)]

Attachment: 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...

Comment 8

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2b

Perhaps this is better after all... there's no need to make assumptions here...

Comment 9

[Boris Zbarsky [:bzbarsky]]

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.

Comment 10

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 276635 [details] [diff] [review]
Patch rev. 2b

a1.9=dbaron

Comment 11

[Boris Zbarsky [:bzbarsky]]

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?

Comment 12

[Mats Palmgren (inactive)]

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

Comment 13

[Mats Palmgren (inactive)]

mozilla/layout/xul/base/src/nsBoxFrame.cpp 	1.337 

-> FIXED

Comment 14

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007082005 Minefield/3.0a8pre

Comment 15

[Daniel Veditz [:dveditz]]

Does this patch work for the 1.8 branch or do we need a separate one?

Comment 16

[Mats Palmgren (inactive)]

Attachment: 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.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 283302 [details] [diff] [review]
Branch patch

r+sr=bzbarsky

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 283302 [details] [diff] [review]
Branch patch

approved for 1.8.1.8 and 1.8.0.14, a=dveditz

Comment 19

[Mats Palmgren (inactive)]

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

Comment 20

[Martijn Wargers (dead)]

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.

Comment 21

[Al Billings [:abillings - ex-MoCo]]

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

[Bob Clary [:bc] (inactive)]

crash test landed
http://hg.mozilla.org/mozilla-central/rev/5a21084d3b9e