762332 - "ASSERTION: EnsureTextRun should have set font size inflation" with -moz-column, rel&abs pos

Status: NEW

(Core :: Layout: Text and Fonts, defect)

Description

[Jesse Ruderman]

Attachment: testcase (requires pref)

With
  user_pref("font.size.inflation.emPerLine", 15);

The testcase triggers:

###!!! ABORT: EnsureTextRun should have set font size inflation: 'GetFontSizeInflation() == fontSizeInflation', file layout/generic/nsTextFrameThebes.cpp, line 7389

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Mats Palmgren (inactive)]

The assertion still occurs.  Is the bug that we forgot to remove
the FontSizeInflationProperty somewhere?

(gdb) p GetFontSizeInflation()
$3 = 1.0333333
(gdb) p fontSizeInflation
$4 = 1
(gdb) p this->IsCurrentFontInflation(fontSizeInflation)
$5 = false
(gdb) p nsLayoutUtils::FontSizeInflationFor(this)
$6 = 1
(gdb) p this->HasFontSizeInflation()
$9 = true

Comment 3

[Mats Palmgren (inactive)]

If we can't come up with a proper fix for this soon, then I suggest we
change it to a non-fatal assertion (or warning) so it doesn't block
fuzz-testing font-inflation and/or moz-columns.

Comment 4

[Mats Palmgren (inactive)]

Attachment: wallpaper

Let's make the assertion non-fatal for now, and add the testcase so we
can detect if it starts crashing or goes away for some reason.

https://tbpl.mozilla.org/?tree=Try&rev=d4fb7ee1827a

Comment 5

[Scott Johnson (:jwir3)]

Comment on attachment 736063 [details] [diff] [review]
wallpaper

Review of attachment 736063 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM

Comment 6

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a1a4d67cc93

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9a1a4d67cc93

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac8c124250b5

Comment 9

[Phil Ringnalda (:philor)]

It's going to go away for crashtest-e10s when 45 merges to aurora tomorrow, making the test permaorange.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I don't understand what the "it" is that's going to go away, nor why RyanVM landed the above changeset or whether it's related to philor's comment.

Comment 11

[Phil Ringnalda (:philor)]

"It" is

(In reply to Mats Palmgren (:mats) from comment #4)
> Let's make the assertion non-fatal for now, and add the testcase so we
> can detect if it starts crashing or goes away for some reason.

and given the context of https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=ac8c124250b5 I would say that while adding crashtests Ryan added one which added an assertion to this test on e10s which it didn't have before, but it only adds it on nightly_build, not on aurora.

Comment 12

[Phil Ringnalda (:philor)]

Which must be https://hg.mozilla.org/integration/mozilla-inbound/rev/8ef6bfa78286

Comment 13

[Phil Ringnalda (:philor)]

And just to add to the fun, on current-aurora the tests appear to run in crashtest.list order, with font-inflation-762332.html running immediately after 750066.html, while on trunk and thus on try for my trunk-as-aurora simulaton they are sorted, so font-inflation-762332.html runs immediately after first-letter-638937-2.html and the first-letter tests run after 1225592.html. So depending on whether that change in ordering is in-tree mozharness or test harness behavior and will merge or is out-of-tree and won't change, I may or may not be right about font-inflation-762332.html having an unexpected-pass from having 0 assertions in crashtest-e10s on aurora tomorrow, depending also on whether it depends on 740199-1.xhtml having set font.size.inflation.emPerLine shortly before it runs, or only on it having set it at all anytime before.

Should be exciting for the sheriff on duty.

Comment 14

[Phil Ringnalda (:philor)]

The lack of an assertion did carry over to actual-aurora, we backed out https://hg.mozilla.org/integration/mozilla-inbound/rev/ac8c124250b5 to fix the orange, according to RyanVM's memory this isn't the first time we've had to, and it would be pretty sweet if someone would either figure out why it now asserts on e10s but only on trunk, or just give up on this whole exercise and either mark it as 0-10000 unconditionally or get rid of the test.

Comment 15

[Mats Palmgren (inactive)]

I've debugged this a little bit.  What we have is a frame tree like this:
block
  text
  text
block
  text
  text

The text frames are all for the same content and are next/prev-in-flow
with each other.  ditto for the block frames.
Also, the text frames share the same TextRun.

The problem is that we use the font inflation for the first
text frame when creating the textrun:
http://hg.mozilla.org/mozilla-central/annotate/cb66ffeb6725/layout/generic/nsTextFrame.cpp#l2124

Font size inflation depends on a frame's ancestors though:
http://hg.mozilla.org/mozilla-central/annotate/cb66ffeb6725/layout/base/nsLayoutUtils.cpp#l7594

and in this case the latter two text frames have a different
nsLayoutUtils::FontSizeInflationFor result from the former two.
(because nsFontInflationData::mInflationEnabled was false in the
latter block and true in the former)

So this assertion can't possibly be correct in the current setup.