The default bug view has changed. See this FAQ.

Crash involving XUL menus, position: fixed, position: absolute [@ nsIFrame::GetNextSibling] [@ nsCSSFrameConstructor::FindFrameWithContent]

VERIFIED FIXED

Status

()

Core
XUL
--
critical
VERIFIED FIXED
11 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: Alfred Peng)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
crash, testcase, verified1.8.0.7, verified1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1 +
blocking1.8.0.7 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical] caused 341047, crash signature)

Attachments

(4 attachments, 1 obsolete attachment)

446 bytes, application/vnd.mozilla.xul+xml
Details
6.59 KB, text/plain
Details
1.30 KB, patch
neil@parkwaycc.co.uk
: review+
Mike Schroepfer
: approval1.8.1+
Details | Diff | Splinter Review
4.38 KB, text/plain
Details
(Reporter)

Description

11 years ago
#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.
(Reporter)

Comment 1

11 years ago
Created attachment 221170 [details]
testcase
(Reporter)

Comment 2

11 years ago
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.
Whiteboard: [sg:critical]
(Reporter)

Comment 3

11 years ago
Created attachment 221171 [details]
stack trace (mac debug)
(Reporter)

Updated

11 years ago
Flags: blocking1.9a1?
(Reporter)

Comment 4

11 years ago
Before I reduced this, it crashed at nsCSSFrameConstructor::FindFrameWithContent instead, with kidContent == (nsIContent *) 0xdddddddd.
Summary: Crash [@ nsIFrame::GetNextSibling] involving XUL menus, position: fixed, position: absolute → Crash involving XUL menus, position: fixed, position: absolute [@ nsIFrame::GetNextSibling] [@ nsCSSFrameConstructor::FindFrameWithContent]

Comment 5

11 years ago
Also see this in Windows XP.
OS: Mac OS X 10.4 → All
Hardware: Macintosh → PC
(Reporter)

Updated

11 years ago
Hardware: PC → All
(Assignee)

Comment 6

11 years ago
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?
Assignee: nobody → alfred.peng
Status: NEW → ASSIGNED
Attachment #221702 - Flags: review?(roc)
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.
(Assignee)

Comment 9

11 years ago
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.
Attachment #221702 - Attachment is obsolete: true
Attachment #221802 - Flags: review?(roc)
Attachment #221702 - Flags: review?(roc)
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.
(Assignee)

Comment 12

11 years ago
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?
Attachment #221802 - Flags: superreview?(bzbarsky)
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.
Attachment #221802 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 14

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

Comment 15

11 years ago
Comment on attachment 221802 [details] [diff] [review]
Patch v2

As roc recommended in comments #10, ask neil for review.
Attachment #221802 - Flags: review?(roc) → review?(neil)

Comment 16

11 years ago
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.
Attachment #221802 - Flags: review?(neil) → review+
(Assignee)

Comment 17

11 years ago
Checking in layout/base/nsCSSFrameConstructor.cpp;
/cvsroot/mozilla/layout/base/nsCSSFrameConstructor.cpp,v  <--  nsCSSFrameConstructor.cpp
new revision: 1.1231; previous revision: 1.1230
done
(Assignee)

Comment 18

11 years ago
=> Fixed
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Same crash on the 1.8 branches
Flags: blocking1.8.1?
Flags: blocking1.8.0.6?

Updated

11 years ago
Flags: blocking1.8.1? → blocking1.8.1+
(Assignee)

Updated

11 years ago
Attachment #221802 - Flags: approval1.8.1?

Updated

11 years ago
Attachment #221802 - Flags: approval1.8.1? → approval1.8.1+
(Assignee)

Comment 20

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

Updated

11 years ago
Keywords: fixed1.8.1
Flags: blocking1.8.0.6? → blocking1.8.0.6+
(Assignee)

Comment 21

11 years ago
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
That crash is bug 253479 (fixed on 1.8, but not 1.8.0 yet).
(Assignee)

Comment 23

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

Comment 25

11 years ago
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.
Attachment #221802 - Flags: approval1.8.0.6?
Comment on attachment 221802 [details] [diff] [review]
Patch v2

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #221802 - Flags: approval1.8.0.7? → approval1.8.0.7+
(Assignee)

Comment 27

11 years ago
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.
Keywords: fixed1.8.0.7

Updated

11 years ago
Depends on: 341047

Comment 28

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

Keywords: fixed1.8.1 → verified1.8.1
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
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.0.7 → verified1.8.0.7
Whiteboard: [sg:critical] → [sg:critical] caused 341047

Updated

11 years ago
Depends on: 351938
Group: security
Flags: in-testsuite?
(Reporter)

Comment 30

9 years ago
Crashtest checked in.
Flags: in-testsuite? → in-testsuite+

Updated

9 years ago
Flags: blocking1.9a1?

Updated

9 years ago
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
Crash Signature: [@ nsIFrame::GetNextSibling] [@ nsCSSFrameConstructor::FindFrameWithContent]
You need to log in before you can comment on or make changes to this bug.