#0 0x073e7a9c in nsIFrame::GetNextSibling (this=0xdddddddd) at /Users/admin/trunk/mozilla/layout/base/../generic/nsIFrame.h:676 this=0xdddddddd means we're playing with deleted memory, so this is probably a security hole.

You'll probably need a debug build to reproduce the crash. I was able to reproduce reliably with a debug build but not at all with a nightly.

Before I reduced this, it crashed at nsCSSFrameConstructor::FindFrameWithContent instead, with kidContent == (nsIContent *) 0xdddddddd.

Also see this in Windows XP.

The "child->Destroy" will invoke function on the child frames in LineBox list again. If there're not only one child frames in the list, some child frames will have been destroyed at that time. Then comes the released memory access, which causes the crash. So I think we should destroy the LineBox list first. I'm not quite sure whether the patch is the right direction. roc, do you have any comments?

I don't think this is the right patch. This will cause the frame(s) to not be found, so they won't be cleaned up properly. The basic problem here is that nsMenuFrame is setting an attribute which triggers a restyle during frame destruction. That is REALLY bad. Restyles are usually delayed until later (which would make this OK) but it's done synchronously here: http://lxr.mozilla.org/seamonkey/source/layout/base/nsCSSFrameConstructor.cpp#10778 specially for menus :-(

You might want to change that nsCSSFrameConstructorCode to not happen if the attribute is menugenerated and the attribute has been removed.

Patch v2 based on Roc's comments. Roc, do I catch your idea by this patch? Is this bug related to bug 279769? To fix that one, nsMenuFrame::UngenerateMenu was added to the Destroy function to get rid of the Menu. I've run the testcase for that one. No regression.

I think this is OK as a hack fix. The non-hack fix is probably to restructure XUL menu popups completely, which is a big project. CCing Boris and Neil though since they know this code better than I.

Hmm... This might be ok, yes. Please check all testcase in the various bugs that are mentioned in the CVS blame there, going back to when async restyling landed; if all those pass this is fine. ;) And yes, what we really need to do is to stop changing the DOM from frame code.

Comment on attachment 221802 [details] [diff] [review] Patch v2 As bz pointed out, I tracked back to the bug 230170. I divide all the bug fixes from that bug till now to the following categories: 1. Bugs causes crash & Hang. Run the testcases on Solaris, work well with the patch. 2. Standard-compatibility bugs(CSS standard etc.). Run the testcase on Solaris on two different builds: with and without this patch and compare the results. The results are the same. 3. SVG related bugs. Run the testcases on Fedora, work well with the patch. 4. Performance and function improvement bugs. No testcases. Besides the above bugs, there are several bugs I suspect to have regression on the trunk, even without the patch(run on Solaris): 1. bug 3247(attachment 179566 [details]) 2. bug 266968(attachment 164045 [details]) 3. bug 271422(attachment 167944 [details]) bz, can this patch be reviewed now?

Comment on attachment 221802 [details] [diff] [review] Patch v2 Er... So what I _meant_ was to test the 2-3 bugs that actually added this sync code and modified it since. Those bugs fit in none of your categories that I can see. Things like bug 262031, for example. Or bug 279308. And so on. Just look at the blame for this little chunk of code, going back to when it was first checked in, and check those bugs. If all those work, sr=bzbarsky.

(In reply to comment #13) > (From update of attachment 221802 [details] [diff] [review] [edit]) > Er... So what I _meant_ was to test the 2-3 bugs that actually added this sync > code and modified it since. Those bugs fit in none of your categories that I > can see. Things like bug 262031, for example. Or bug 279308. And so on. > These two bugs don't regress with the patch. I classify them into category 2.:) > Just look at the blame for this little chunk of code, going back to when it was > first checked in, and check those bugs. If all those work, sr=bzbarsky. > Besides the above two, the following bugs are also related: bug 282792, bug 288365, bug 281957. No testcases for them. Category 4.

Comment on attachment 221802 [details] [diff] [review] Patch v2 As roc recommended in comments #10, ask neil for review.

Comment on attachment 221802 [details] [diff] [review] Patch v2 Whatever... I have no prospect of knowing layout well enough to know why there isn't some other way that menupopups can be synchronously generated.

Checking in layout/base/nsCSSFrameConstructor.cpp; /cvsroot/mozilla/layout/base/nsCSSFrameConstructor.cpp,v <-- nsCSSFrameConstructor.cpp new revision: 1.1231; previous revision: 1.1230 done

=> Fixed

Same crash on the 1.8 branches

Checking in nsCSSFrameConstructor.cpp; /cvsroot/mozilla/layout/base/nsCSSFrameConstructor.cpp,v <-- nsCSSFrameConstructor.cpp new revision: 1.1110.6.37; previous revision: 1.1110.6.36 done

With the patch applied to MOZILLA_1_8_0_BRANCH, Firefox still crashes for the testcase. And the core stack is different. I also get the following assertion: ###!!! ASSERTION: We don't support out-of-flow kids: '!aListName', file nsBoxFrame.cpp, line 1164 Break: at file nsBoxFrame.cpp, line 1164 ###!!! ASSERTION: Must have frame for destination coordinate system!: 'aOther', file nsFrame.cpp, line 2359 Break: at file nsFrame.cpp, line 2359 ###!!! ASSERTION: No view on any parent? How did that happen?: 'Not Reached', file nsFrame.cpp, line 4320 Break: at file nsFrame.cpp, line 4320

That crash is bug 253479 (fixed on 1.8, but not 1.8.0 yet).

I think we'd better wait for that bug fixed in 1.8.0.6 first.

That bug is a null-dereference. This bug is a deleted-memory reference. So we should fix this even though we still crash -- at least we crash more safely.

Comment on attachment 221802 [details] [diff] [review] Patch v2 This is also a potential security bug for 1.8.0 branch. And the patch has been in trunk and 1.8 branch for some time not causing problem till now.

Comment on attachment 221802 [details] [diff] [review] Patch v2 approved for 1.8.0 branch, a=dveditz for drivers

Checking in layout/base/nsCSSFrameConstructor.cpp; /cvsroot/mozilla/layout/base/nsCSSFrameConstructor.cpp,v <-- nsCSSFrameConstructor.cpp new revision: 1.1110.6.12.2.19; previous revision: 1.1110.6.12.2.18 done => Land on 1.8.0 branch.

https://bugzilla.mozilla.org/attachment.cgi?id=221170 ff2b2 windows, linux, macppc no crash, macppc does not show the select box though... ff2b2 debug windows, linux no crash ###!!! ASSERTION: We don't support out-of-flow kids: '!aListName', file f:/work/mozilla/builds/ff/2.0/mozilla/layout/xul/base/src/nsBoxFrame.cpp, line 1164

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.7pre) Gecko/20060821 Firefox/1.5.0.7pre https://bugzilla.mozilla.org/attachment.cgi?id=221170&action=view, no crash. verified 1.8.0.7

Crashtest checked in.