Closed Bug 1233135 Opened 4 years ago Closed 4 years ago

"Assertion failure: fieldsetContentDisplay->mDisplay == 1 (bug in nsRuleNode::ComputeDisplayData?)"

Categories

(Core :: Layout, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox46 --- wontfix
firefox47 --- fixed

People

(Reporter: jruderman, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, regression, testcase)

Attachments

(3 files)

Attached file testcase
Assertion failure: fieldsetContentDisplay->mDisplay == 1 (bug in nsRuleNode::ComputeDisplayData?), at layout/base/nsCSSFrameConstructor.cpp:3180

This assertion is part of code added in https://hg.mozilla.org/mozilla-central/rev/76a5cbbc0600
Attached file stack
The problem is caused by this code block:
http://hg.mozilla.org/mozilla-central/annotate/0babaa3edcf9/layout/style/nsStyleContext.cpp#l708

ShouldSuppressLineBreak returns true because we have 'display:ruby':
http://hg.mozilla.org/mozilla-central/annotate/0babaa3edcf9/layout/style/nsStyleContext.cpp#l514
and then it modifies 'display' from 'block' (from forms.css) to 'inline-block'
on the inner ::-moz-fieldset-content frame.

It seems dangerous to modify 'display' like this on inner anonymous frames.
I think we need to avoid that somehow.
Flags: needinfo?(quanxunzhen)
Assignee: nobody → quanxunzhen
Flags: needinfo?(quanxunzhen)
Attachment #8699307 - Flags: review?(dbaron) → review+
Comment on attachment 8699307 [details]
MozReview Request: Bug 1233135 - Do not touch display value of anonymous box for ruby.

https://reviewboard.mozilla.org/r/28235/#review28863

::: layout/style/nsStyleContext.cpp:724
(Diff revision 1)
> +    if (GetPseudoType() != nsCSSPseudoElements::ePseudo_AnonBox) {

Instead of putting this here, I think it would be a lot clearer if you passed aStyleContext into ShouldSuppressLineBreak, and put this condition next to the IsOutOfFlowStyle check there.

Could you also add some tests with various form controls (fieldset, <select>, <select size=2>) and an element with overflow:scroll (and display:inline-block), with, and inside of, display:ruby?  It would also be good to test the overflow:scroll case with stuff inside the overflow:scroll element that would cause problems if linebreak suppression failed in a more normal case.


I'm not crazy about the assumptions this makes about what's in the frame tree, but I guess this seems OK, with that change.
(In reply to David Baron [:dbaron] ⌚️UTC+13 from comment #4)
> Comment on attachment 8699307 [details]
> MozReview Request: Bug 1233135 - Do not touch display value of anonymous box
> for ruby.
> 
> https://reviewboard.mozilla.org/r/28235/#review28863
> 
> ::: layout/style/nsStyleContext.cpp:724
> (Diff revision 1)
> > +    if (GetPseudoType() != nsCSSPseudoElements::ePseudo_AnonBox) {
> 
> Instead of putting this here, I think it would be a lot clearer if you
> passed aStyleContext into ShouldSuppressLineBreak, and put this condition
> next to the IsOutOfFlowStyle check there.

As comment above this line said, we still need to set the SUPPRESS_LINEBREAK flag on some anonbox e.g. text frame, so I think this condition probably should be here.
Comment on attachment 8699307 [details]
MozReview Request: Bug 1233135 - Do not touch display value of anonymous box for ruby.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28235/diff/1-2/
Attachment #8699307 - Flags: review+ → review?(dbaron)
OK -- I suspect you should just exclude text frames, then?

My guess is that currently a select inside of ruby is pretty broken because of the linebreak suppression on options, for example.  Although I suppose it's possible that what I was suggesting wouldn't even fix that.
Flags: needinfo?(quanxunzhen)
Hmmm... and ruby anonymous boxes need to be excluded as well.
(In reply to David Baron [:dbaron] ⌚️UTC+13 from comment #7)
> My guess is that currently a select inside of ruby is pretty broken because
> of the linebreak suppression on options, for example.  Although I suppose
> it's possible that what I was suggesting wouldn't even fix that.

It seems so if the select has an inline display value. So anything I should do in this bug for this issue?
Flags: needinfo?(quanxunzhen)
Comment on attachment 8699307 [details]
MozReview Request: Bug 1233135 - Do not touch display value of anonymous box for ruby.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28235/diff/2-3/
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #9)
> It seems so if the select has an inline display value. So anything I should
> do in this bug for this issue?

Does the revised patch fix the problem?  I would expect that it does.
Flags: needinfo?(quanxunzhen)
Hmmm, partially. It seems to fix the combobox case, but not the listbox case (with size>1 or multiple set).

From code in CSS frame constructor, it seems listbox doesn't have any anonymous box in-between.
Flags: needinfo?(quanxunzhen)
Attachment #8699307 - Flags: review?(dbaron) → review+
Comment on attachment 8699307 [details]
MozReview Request: Bug 1233135 - Do not touch display value of anonymous box for ruby.

https://reviewboard.mozilla.org/r/28235/#review28895

I think it's worth filing a followup bug on the remaining issue with listboxes, but I don't think it's worth spending effort on now.

::: layout/style/crashtests/crashtests.list:136
(Diff revision 3)
> +load 1233135-1.html
> +load 1233135-2.html

Seems like you might want to construct a reftest here rather than just a crashtest.
(In reply to David Baron [:dbaron] ⌚️UTC+13 from comment #13)
> ::: layout/style/crashtests/crashtests.list:136
> (Diff revision 3)
> > +load 1233135-1.html
> > +load 1233135-2.html
> 
> Seems like you might want to construct a reftest here rather than just a
> crashtest.

It doesn't seem to me we are testing any rendering effect here. I'm just testing that form controls won't crash even if inside ruby or have ruby display value.
Filed bug 1242344.
https://hg.mozilla.org/mozilla-central/rev/9f993400fcbd
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Too late for assertion fixes in 46.
You need to log in before you can comment on or make changes to this bug.