Crash [@ nsComboboxControlFrame::Reflow] with select using display: absolute

RESOLVED FIXED

Status

()

Core
Layout: Form Controls
--
critical
RESOLVED FIXED
12 years ago
7 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: mats)

Tracking

({crash, regression, testcase})

Trunk
x86
All
crash, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.8.1.x -
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?] maybe freed mem? post 1.8-branch, crash signature)

Attachments

(5 attachments)

(Reporter)

Description

12 years ago
See upcoming testcase, which crashes current trunk Mozilla builds. It doesn't crash Firefox1.5 builds, so seems like a regression.

The testcase consists of this:
<select style="position: absolute;">
<meta>
<option style="position: absolute;">
<body style="display: block;">

Talkback ID: TB20560549W
nsComboboxControlFrame::Reflow
(Reporter)

Comment 1

12 years ago
Created attachment 227999 [details]
Testcase (crashes on load)
Regression between 1.9a1_2006041003 and 1.9a1_2006041022.
(Reporter)

Comment 3

12 years ago
Ok, thanks!
I've backed out the patch for bug 332499 in my debug build and after that it doesn't crash anymore.
Blocks: 332499
(Reporter)

Comment 4

12 years ago
Created attachment 228026 [details]
backtrace

Backtrace from debug build.
Prior to the crash, I get an assertion (stack included):
###!!! ASSERTION: index out of range: '0 <= aIndex && aIndex < Count()', file ..
/../dist/include/xpcom/nsVoidArray.h, line 373

Then the crash:
#0  0x05c41442 in nsComboboxControlFrame::Reflow (this=0x104cf2fc,
    aPresContext=0x10751ad0, aDesiredSize=@0x22e9d8, aReflowState=@0x22e8f8,
    aStatus=@0x22eb50)
    at c:/mozilla/mozilla/layout/forms/nsComboboxControlFrame.cpp:1132
#1  0x05ba6a42 in nsAbsoluteContainingBlock::ReflowAbsoluteFrame (
    this=0x10771a40, aDelegatingFrame=0x107719f4, aPresContext=0x10751ad0,
    aReflowState=@0x22f0c8, aContainingBlockWidth=-1,
    aContainingBlockHeight=-1, aKidFrame=0x104cf2fc,
    aReason=eReflowReason_Incremental, aStatus=@0x22eb50)
    at c:/mozilla/mozilla/layout/generic/nsAbsoluteContainingBlock.cpp:561
#2  0x05ba6458 in nsAbsoluteContainingBlock::IncrementalReflow (
    this=0x10771a40, aDelegatingFrame=0x107719f4, aPresContext=0x10751ad0,
    aReflowState=@0x22f0c8, aContainingBlockWidth=-1,
    aContainingBlockHeight=-1)
    at c:/mozilla/mozilla/layout/generic/nsAbsoluteContainingBlock.cpp:414
etc.
(Assignee)

Comment 5

12 years ago
I have a fix for this locally. I think we're using free'd memory here
so marking Security-Sensitive.

The problem is that with iterate the reflow path while reflowing children,
which removes items from the path (I'm assuming that's ok and that the path
isn't meant to be immutable?)
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/forms/nsComboboxControlFrame.cpp&rev=1.370&root=/cvsroot&mark=1124-1126,1158,1178#1123

The patch I have iterates the path, picking out the children we're looking for
to a local list and then iterates that instead.
Assignee: nobody → mats.palmgren
Group: security
OS: Windows XP → All
(Assignee)

Comment 6

12 years ago
Boris, see last comment. Does that seem like an acceptable fix?
(Reporter)

Comment 7

12 years ago
(In reply to comment #5)
> I have a fix for this locally. I think we're using free'd memory here
> so marking Security-Sensitive.

I didn't mark it security-sensitive, because it's a trunk only crasher, which I (sort of) expect to be fixed before an official release comes out.
So is it really necessary to mark it security sensitive? I guess this would mean that I would have to file every crasher bug as security sensitive.
The reflow branch would fix this since it gets rid of reflow paths. Since this is only on trunk (right?) maybe we should just not bother fixing it until the reflow branch lands.
(Assignee)

Comment 9

12 years ago
The current reflow looks strange here:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/forms/nsComboboxControlFrame.cpp&rev=1.371&root=/cvsroot&mark=1178,1180,1181#1150

This is while we iterate with 'iter' - none of the three marked lines looks at
*iter, so we reflow the list frame each time. I believe there should be a
"*iter == plainLstFrame" in there somewhere. And why test that the first
child is scrollable? (doesn't seem relevant to me)
(Assignee)

Comment 10

12 years ago
Created attachment 228544 [details] [diff] [review]
Patch rev. 1

* fix the crash and issues mentioned above
* cleanup #include list
* get rid of NS_GET_IID
Attachment #228544 - Flags: review?(roc)
+    // Store the frames in the reflow path to |framesToReflow| since we can't
+    // use a nsReflowPath::iterator on the path while reflowing, see bug 343510.
+    nsAutoVoidArray framesToReflow;

Is this really necessary? the path/iterator is designed to be safe if you remove the current node from the path during iteration.

+        if (frameToReflow == listFrame) {

Seems to me that this might be all that's needed...
fwiw, should we also !important static positioning and no float for options in forms.css?
Sounds reasonable to me.
(Assignee)

Comment 14

12 years ago
(In reply to comment #11)
> Is this really necessary? 

It seems so, here's a detailed trace of the loop iterator and reflow path:

frame 1: 0x8896a20 (mButtonFrame):
frame 2: 0x8889fc4 (mDropdownFrame):
frame 3: 0x88989dc (other frame): abs.pos.
aReflowState.path is: { mNode=0x889b7d8 mIndex=2 (frame=0x8896a20) } { mNode=0x889b7d8 mIndex=1 (frame=0x8889fc4) } { mNode=0x889b7d8 mIndex=0 (frame=0x88989dc) }
aReflowState.path->EndChildren() is: { mNode=0x889b7d8 mIndex=-1 }
Loop iterator is: { mNode=0x889b7d8 mIndex=2  (frame=0x8896a20) }
Now reflowing 0x8896a20
aReflowState.path is: { mNode=0x889b7d8 mIndex=0 (frame=0x8889fc4) }
aReflowState.path->EndChildren() is: { mNode=0x889b7d8 mIndex=-1 }
Loop iterator is: { mNode=0x889b7d8 mIndex=1 ###!!! ASSERTION: index out of range: '0 <= aIndex && aIndex < Count()', file ../../dist/include/xpcom/nsVoidArray.h, line 373

Program received signal SIGSEGV, Segmentation fault.


(In reply to comment #12)
> fwiw, should we also !important static positioning and no float for options
> in forms.css?

Ok, I'll fix that too.
(Assignee)

Comment 15

12 years ago
Created attachment 230256 [details] [diff] [review]
Patch forms.css, rev. 1

UA stylesheet changes for <option> and <optgroup>.
This actually fixes the crash for the testcase, but I suspect there are other
ways to still trigger the crash with this change...
Attachment #230256 - Flags: superreview?(bzbarsky)
Attachment #230256 - Flags: review?(bzbarsky)
Comment on attachment 230256 [details] [diff] [review]
Patch forms.css, rev. 1

Looks good.
Attachment #230256 - Flags: superreview?(bzbarsky)
Attachment #230256 - Flags: superreview+
Attachment #230256 - Flags: review?(bzbarsky)
Attachment #230256 - Flags: review+
(Assignee)

Comment 17

12 years ago
Comment on attachment 230256 [details] [diff] [review]
Patch forms.css, rev. 1

Checked in to trunk at 2006-07-25 09:16 PDT.
(Assignee)

Updated

11 years ago
Attachment #228544 - Flags: review?(roc)
(Assignee)

Comment 18

11 years ago
The reflow branch landing fixed the original cause of the problem.
I think we should back out the forms.css changes (attachment 230256 [details] [diff] [review])
since they are no longer needed.
Depends on: 300030
Well.  Does it really make sense to position the options and optgroups?  I'd say no... do any other UAs allow it?
(Assignee)

Comment 20

11 years ago
Comment on attachment 230256 [details] [diff] [review]
Patch forms.css, rev. 1

Request for back out.
Attachment #230256 - Flags: review?(bzbarsky)
See comment 19.
(Assignee)

Comment 22

11 years ago
(In reply to comment #19)
> Well.  Does it really make sense to position the options and optgroups?  I'd
> say no... do any other UAs allow it?

Who are we to question the wisdom of the author stylesheet? ;-)
As a matter of principle I'd like us to have as few !important limitations
as possible in the UA stylesheet. I think it's a slippery slope for us
to decide what is meaningful styling, in most cases I'd rather see that
we allow the author to shoot himself in the foot.

OTOH, I doubt other UAs [will] support it (I tried a few samples in IE7,
Webkit and Opera9 and the float/position seemed to be completely ignored by
IE/Webkit and mostly ignored by Opera (with some weird results - probably
just a bug) so I guess compatibility is an argument to keep it as is.
I don't feel that strongly about this particular case. Your call.
(Assignee)

Comment 23

11 years ago
Created attachment 253188 [details]
Just for fun...

Does this make sense? ;-)
> Who are we to question the wisdom of the author stylesheet? ;-)

People implementing the CSS spec, which explicitly says that CSS applied to form controls has undefined behavior.  Therefore, ignoring the CSS is a better approach than doing things that authors will depend on, since it allows CSS for forms to be defined in a sane way later, if need be.

So for form controls I think we should have as many !important limitations as we can get away with without breaking the de-facto stuff people already depend on (mostly because UA implementors were dumb and allowed CSS to apply to form controls at all, mostly in inconsistent ways).

Put another way, a <select> is a replaced element from the point of view of CSS, so I feel that we should treat it that way.  I'd be most happy with a !important display for options and optgroups too.
(Assignee)

Comment 25

11 years ago
Well, we need a spec that defines the styling of form controls then... ;-)

Anyway, the crash here is fixed...
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Assignee)

Updated

11 years ago
Attachment #230256 - Flags: review?(bzbarsky)
Yeah, the CSS WG is planning to work on one.  It's just a low priority.  ;)
(Reporter)

Comment 27

11 years ago
Is Mozilla allowed to crash under certain circumstances, when I have removed certain css rules from xul.css, forms.css or html.css?
As things stand, yes...
Whiteboard: post 1.8-branch
(In reply to comment #7)
> I didn't mark it security-sensitive, because it's a trunk only crasher, which I
> (sort of) expect to be fixed before an official release comes out.
> So is it really necessary to mark it security sensitive?

If you think it potentially exploitable then it needs to be flagged, either security sensitive and/or with a security-group rating in the whiteboard, otherwise it might get lost.

> I guess this would
> mean that I would have to file every crasher bug as security sensitive.

Only the ones that might be exploitable. Probably have to run in a debug build to be sure its not, in an optimized build some unsafe crashes look like null-derefs but that's just an accident of the testcase.


Flags: wanted1.8.1.x-
Whiteboard: post 1.8-branch → [sg:critical?] maybe freed mem? post 1.8-branch
Group: security

Comment 30

9 years ago
crash test landed
http://hg.mozilla.org/mozilla-central/rev/e57e5ef7fc19
Flags: in-testsuite+
Crash Signature: [@ nsComboboxControlFrame::Reflow]
You need to log in before you can comment on or make changes to this bug.