Last Comment Bug 391974 - Crash [@ nsFrameList::DestroyFrames] with menuitem, tooltip and setting some attributes
: Crash [@ nsFrameList::DestroyFrames] with menuitem, tooltip and setting some ...
Status: VERIFIED FIXED
[sg:critical?] using freed frame
: crash, testcase, verified1.8.0.14, verified1.8.1.8
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla1.9alpha8
Assigned To: Mats Palmgren (:mats)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-08-12 15:58 PDT by Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
Modified: 2011-06-13 10:01 PDT (History)
6 users (show)
dveditz: blocking1.8.1.8+
dveditz: wanted1.8.1.x+
dveditz: wanted1.8.0.x+
bob: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (515 bytes, application/vnd.mozilla.xul+xml)
2007-08-12 15:58 PDT, Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( )
no flags Details
stack + data (6.89 KB, text/plain)
2007-08-14 02:33 PDT, Mats Palmgren (:mats)
no flags Details
Patch rev. 1 (diff -w) (1.81 KB, patch)
2007-08-14 02:43 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Review
Patch rev. 2 (2.28 KB, patch)
2007-08-14 07:42 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Review
Patch rev. 2b (2.21 KB, patch)
2007-08-14 07:56 PDT, Mats Palmgren (:mats)
enndeakin: review+
bzbarsky: superreview+
dbaron: approval1.9+
Details | Diff | Review
Branch patch (2.06 KB, patch)
2007-10-02 20:51 PDT, Mats Palmgren (:mats)
bzbarsky: review+
bzbarsky: superreview+
dveditz: approval1.8.1.8+
dveditz: approval1.8.0.14+
Details | Diff | Review

Description Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2007-08-12 15:58:00 PDT
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
Comment 1 Mats Palmgren (:mats) 2007-08-14 02:33:20 PDT
Created attachment 276611 [details]
stack + data
Comment 2 Mats Palmgren (:mats) 2007-08-14 02:39:00 PDT
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 (:mats) 2007-08-14 02:43:01 PDT
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?
Comment 4 Boris Zbarsky [:bz] 2007-08-14 05:56:36 PDT
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 2007-08-14 07:07:13 PDT
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 (:mats) 2007-08-14 07:31:27 PDT
(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 (:mats) 2007-08-14 07:42:27 PDT
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...
Comment 8 Mats Palmgren (:mats) 2007-08-14 07:56:38 PDT
Created attachment 276635 [details] [diff] [review]
Patch rev. 2b

Perhaps this is better after all... there's no need to make assumptions here...
Comment 9 Boris Zbarsky [:bz] 2007-08-14 10:00:55 PDT
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-7 (review requests must explain patch) 2007-08-14 20:55:18 PDT
Comment on attachment 276635 [details] [diff] [review]
Patch rev. 2b

a1.9=dbaron
Comment 11 Boris Zbarsky [:bz] 2007-08-16 09:17:53 PDT
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 (:mats) 2007-08-16 09:29:32 PDT
Oops, I'll do an empty commit as soon as the tree opens. Thanks.
Comment 13 Mats Palmgren (:mats) 2007-08-16 09:30:05 PDT
mozilla/layout/xul/base/src/nsBoxFrame.cpp 	1.337 

-> FIXED
Comment 14 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2007-08-21 20:01:36 PDT
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] 2007-09-26 13:27:41 PDT
Does this patch work for the 1.8 branch or do we need a separate one?
Comment 16 Mats Palmgren (:mats) 2007-10-02 20:51:38 PDT
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.
Comment 17 Boris Zbarsky [:bz] 2007-10-02 21:05:33 PDT
Comment on attachment 283302 [details] [diff] [review]
Branch patch

r+sr=bzbarsky
Comment 18 Daniel Veditz [:dveditz] 2007-10-02 21:55:12 PDT
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 (:mats) 2007-10-02 23:02:31 PDT
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 [:mwargers] (gone per 2016-05-31 :-( ) 2007-10-04 16:54:32 PDT
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] 2007-12-10 18:04:20 PST
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:] 2009-04-24 10:38:22 PDT
crash test landed
http://hg.mozilla.org/mozilla-central/rev/5a21084d3b9e

Note You need to log in before you can comment on or make changes to this bug.