Closed Bug 1113216 Opened 5 years ago Closed 5 years ago

Assertion failure: aCoord.IsCoordPercentCalcUnit() with <fieldset> in vertical writing mode

Categories

(Core :: Layout: Form Controls, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: smontagu, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

Attached file Testcase
STR: Set one of the vertical writing modes in the attached testcase in a debug build (with vertical support enabled, of course).

Assertion failure: aCoord.IsCoordPercentCalcUnit(), at ...layout/base/nsLayoutUtils.h:1373
Attached file Top of stacktrace
The assertion (and subsequent crash, at least in my local build) here arises because forms.css includes

  fieldset > legend {
    padding-left: 2px;
    padding-right: 2px;
    width: -moz-fit-content;
  }

and the use of -moz-fit-content for the block-size dimension of the fieldset is not supported.

We should at least avoid crashing on this, but we'll also need to modify the stylesheet to properly support vertical writing modes once the necessary logical properties are implemented.
From a quick test motivated by bug 1119475 comment 7, there seems to be a very similar crash with <marquee>, presumably because of
 width: -moz-available;
in html.css. Not sure if it needs its own bug or can ride along with this one.
(In reply to Simon Montagu :smontagu from comment #3)
> From a quick test motivated by bug 1119475 comment 7, there seems to be a
> very similar crash with <marquee>, presumably because of
>  width: -moz-available;
> in html.css. Not sure if it needs its own bug or can ride along with this
> one.

My guess about |width: -moz-available;| is wrong, or at best not the whole story. Removing that line from html.css doesn't prevent the fatal assertion.
The problem is that for block-size, we don't support the enumerated values such as -moz-fit-content; but we do parse them as values for 'width', and in vertical modes that becomes block-size, at which point ComputeBSizeValue gets faced with a type of value it didn't expect. We need to fix this, either by supporting these values for both inline and block sizes, or by making their parsing depend on writing mode, but in the meantime here's a bandaid that at least prevents us crashing on the mismatch (in either the fieldset or marquee cases). It'll still issue (debug-build) warnings to remind us of the unfinished work here.
Attachment #8549003 - Flags: review?(smontagu)
Sorry, attached an old version of the patch. This is a bit tidier.
Attachment #8549005 - Flags: review?(smontagu)
Attachment #8549003 - Attachment is obsolete: true
Attachment #8549003 - Flags: review?(smontagu)
I think it would be preferable to make IsAutoBSize return true for the enumerated values for now.  That would avoid calling ComputeBSizeValue, since all caller check IsAutoBSize before calling.

That would actually do the right thing for height/width: min/max/fit-content, though it wouldn't be quite right for the rest.
Flags: needinfo?(jfkthame)
OK, that sounds reasonable. And as far as I can determine, this does prevent the assertions/crashes as expected.
Attachment #8549097 - Flags: review?(dbaron)
Attachment #8549005 - Attachment is obsolete: true
Flags: needinfo?(jfkthame)
Attachment #8549005 - Flags: review?(smontagu)
Comment on attachment 8549097 [details] [diff] [review]
Avoid assertions/crashes by treating enumerated block-size values as auto, pending proper implementation of all the values

>   static bool IsAutoBSize(const nsStyleCoord &aCoord, nscoord aCBBSize)
>   {
>     nsStyleUnit unit = aCoord.GetUnit();
>     return unit == eStyleUnit_Auto ||  // only for 'height'
>            unit == eStyleUnit_None ||  // only for 'max-height'
>+           // The enumerated values were originally aimed at inline-size
>+           // (or width, as it was before logicalization). For now, let them
>+           // return true here, so that we don't call ComputeBSizeValue with
>+           // value types that it doesn't understand. (See bug 1113216.)
>+           unit == eStyleUnit_Enumerated ||
>            (aCBBSize == nscoord_MAX && aCoord.HasPercent());
>   }

Could you add an explicit "FIXME (bug NNNNNNN)" comment saying that this isn't correct for the 'fill' value or for the 'min-*' or 'max-*' properties?  (And if there isn't already a bug filed, please file one.)

r=dbaron with that
Attachment #8549097 - Flags: review?(dbaron) → review+
... and the comment should probably say that the values need to be handled differently by IsAutoBSize's callers.
Pushed, with added comment as requested.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0aee1e7b3e11
Assignee: nobody → jfkthame
Target Milestone: --- → mozilla38
(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #9)
> (And if there isn't already a bug filed, please file one.)

Jonathan, when you file this bug can you make it block bug 1122253?  Thanks.
I think bug 567039 and bug 527285 cover this, so I noted those in the comment. I'll add them as blocking  bug 1122253, too.
Oh, they're already there. :)
You need to log in before you can comment on or make changes to this bug.