Last Comment Bug 336999 - Crash involving XUL menus, position: fixed, position: absolute [@ nsIFrame::GetNextSibling] [@ nsCSSFrameConstructor::FindFrameWithContent]
: Crash involving XUL menus, position: fixed, position: absolute [@ nsIFrame::G...
Status: VERIFIED FIXED
[sg:critical] caused 341047
: crash, testcase, verified1.8.0.7, verified1.8.1
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: All All
: -- critical (vote)
: ---
Assigned To: Alfred Peng
:
Mentors:
Depends on: 341047 351938
Blocks: randomstyles
  Show dependency treegraph
 
Reported: 2006-05-07 05:29 PDT by Jesse Ruderman
Modified: 2011-06-13 10:01 PDT (History)
8 users (show)
mtschrep: blocking1.8.1+
dveditz: blocking1.8.0.7+
jruderman: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (446 bytes, application/vnd.mozilla.xul+xml)
2006-05-07 05:30 PDT, Jesse Ruderman
no flags Details
stack trace (mac debug) (6.59 KB, text/plain)
2006-05-07 05:31 PDT, Jesse Ruderman
no flags Details
Patch v1 (1.84 KB, patch)
2006-05-11 06:11 PDT, Alfred Peng
no flags Details | Diff | Review
Patch v2 (1.30 KB, patch)
2006-05-12 06:02 PDT, Alfred Peng
neil: review+
bzbarsky: superreview+
dveditz: approval1.8.0.7+
mtschrep: approval1.8.1+
Details | Diff | Review
Core stack after the patch applied (4.38 KB, text/plain)
2006-07-28 02:33 PDT, Alfred Peng
no flags Details

Description Jesse Ruderman 2006-05-07 05:29:42 PDT
#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.
Comment 1 Jesse Ruderman 2006-05-07 05:30:16 PDT
Created attachment 221170 [details]
testcase
Comment 2 Jesse Ruderman 2006-05-07 05:31:22 PDT
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.
Comment 3 Jesse Ruderman 2006-05-07 05:31:57 PDT
Created attachment 221171 [details]
stack trace (mac debug)
Comment 4 Jesse Ruderman 2006-05-07 05:51:29 PDT
Before I reduced this, it crashed at nsCSSFrameConstructor::FindFrameWithContent instead, with kidContent == (nsIContent *) 0xdddddddd.
Comment 5 Bob Clary [:bc:] 2006-05-07 10:54:43 PDT
Also see this in Windows XP.
Comment 6 Alfred Peng 2006-05-11 06:11:38 PDT
Created attachment 221702 [details] [diff] [review]
Patch v1

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?
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-05-11 10:57:33 PDT
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 :-(
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-05-11 11:02:22 PDT
You might want to change that nsCSSFrameConstructorCode to not happen if the attribute is menugenerated and the attribute has been removed.
Comment 9 Alfred Peng 2006-05-12 06:02:17 PDT
Created attachment 221802 [details] [diff] [review]
Patch v2

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.
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2006-05-12 19:37:08 PDT
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.
Comment 11 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-05-12 19:55:36 PDT
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 12 Alfred Peng 2006-05-16 05:51:52 PDT
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 13 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-05-16 09:53:22 PDT
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.
Comment 14 Alfred Peng 2006-05-17 05:28:35 PDT
(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 15 Alfred Peng 2006-05-23 02:36:42 PDT
Comment on attachment 221802 [details] [diff] [review]
Patch v2

As roc recommended in comments #10, ask neil for review.
Comment 16 neil@parkwaycc.co.uk 2006-05-24 06:50:25 PDT
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.
Comment 17 Alfred Peng 2006-05-24 20:53:47 PDT
Checking in layout/base/nsCSSFrameConstructor.cpp;
/cvsroot/mozilla/layout/base/nsCSSFrameConstructor.cpp,v  <--  nsCSSFrameConstructor.cpp
new revision: 1.1231; previous revision: 1.1230
done
Comment 18 Alfred Peng 2006-06-01 13:33:52 PDT
=> Fixed
Comment 19 Daniel Veditz [:dveditz] 2006-07-07 01:39:45 PDT
Same crash on the 1.8 branches
Comment 20 Alfred Peng 2006-07-13 21:16:48 PDT
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
Comment 21 Alfred Peng 2006-07-28 02:33:55 PDT
Created attachment 231074 [details]
Core stack after the patch applied

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
Comment 22 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-07-28 09:39:27 PDT
That crash is bug 253479 (fixed on 1.8, but not 1.8.0 yet).
Comment 23 Alfred Peng 2006-07-28 10:02:21 PDT
I think we'd better wait for that bug fixed in 1.8.0.6 first.
Comment 24 Boris Zbarsky [:bz] (Out June 25-July 6) 2006-07-28 10:05:54 PDT
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 25 Alfred Peng 2006-07-28 10:12:05 PDT
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 26 Daniel Veditz [:dveditz] 2006-08-09 14:32:36 PDT
Comment on attachment 221802 [details] [diff] [review]
Patch v2

approved for 1.8.0 branch, a=dveditz for drivers
Comment 27 Alfred Peng 2006-08-10 06:03:19 PDT
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.
Comment 28 Bob Clary [:bc:] 2006-08-22 00:20:40 PDT
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

Comment 29 alice nodelman [:alice] [:anode] 2006-08-22 15:00:51 PDT
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
Comment 30 Jesse Ruderman 2007-12-19 14:51:57 PST
Crashtest checked in.

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