Make WritingModes assert when NS_UNCONSTRAINEDSIZE is passed as container width

NEW
Assigned to

Status

()

Core
Layout: Text
3 years ago
3 years ago

People

(Reporter: smontagu, Assigned: smontagu)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
Created attachment 8510555 [details] [diff] [review]
Patch

To prevent and help pin down things like bug 1088151, we need to assert when passing NS_UNCONSTRAINED size as aContainerWidth to WritingModes methods
Attachment #8510555 - Flags: review?(jfkthame)
Attachment #8510555 - Flags: review?(jfkthame) → review+
(Assignee)

Comment 1

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=0c9d39ffbd48
Keywords: checkin-needed
the patch failed to apply:

patching file layout/generic/WritingModes.h
Hunk #10 FAILED at 1453
1 out of 11 hunks FAILED -- saving rejects to file layout/generic/WritingModes.h.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh unconstrained.width.diff

could you take a look, thanks!
Assignee: nobody → smontagu
Flags: needinfo?(smontagu)
Keywords: checkin-needed
(Assignee)

Comment 3

3 years ago
Sorry about that, the version on the try push at https://hg.mozilla.org/try/rev/0c9d39ffbd48 is rebased to current inbound.

But the new assertion is turning up on crashtests in the try push, so this will have to hold off for checkin anyway. Bug 1088547 will presumably catch at least some of the cases.
Depends on: 1088547
Flags: needinfo?(smontagu)
(In reply to Simon Montagu (PTO 2014-10-26 - 2014-11-10 not reading bugmail) :smontagu from comment #3)

> But the new assertion is turning up on crashtests in the try push, so this
> will have to hold off for checkin anyway. Bug 1088547 will presumably catch
> at least some of the cases.

I've started a try job that includes this together with bug 1088547; if that comes up clear, I'll go ahead and land it next time I'm ready to push.
Nope, still lots of assertions. So we'll need to investigate further before this patch lands.
It turns out that most (possibly all?) of the crashtest assertions this introduces are "expected", in the sense that they're not due to a code bug whereby we incorrectly pass NS_UNCONSTRAINEDSIZE; they're due to the use of huge dimensions in the testcases, so that we really do have a container width that exceeds NS_MAXSIZE.

(Compare various other places in layout that issue warnings such as "have unconstrained inline-size; this should only result from very large sizes, not attempts at intrinsic inline-size calculation". Well, these testcases *do* have "very large sizes" that appear to be "unconstrained" as far as layout is concerned.)

If we make the assertion a bit narrower, so that it tests aContainerWidth != NS_MAXSIZE rather than aContainerWidth < NS_MAXSIZE, a few of the testcases no longer assert, but several of them still do: https://tbpl.mozilla.org/?tree=Try&rev=53fa4c9db9da.

So either we can't have this assertion after all, or we'll need to annotate those as "expected" assertions (after checking each testcase to make sure that's really what's causing it; I haven't examined all of them yet).
You need to log in before you can comment on or make changes to this bug.