Open Bug 762332 Opened 8 years ago Updated 4 years ago

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

Categories

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

defect
Not set

Tracking

()

People

(Reporter: jruderman, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase, Whiteboard: [font-inflation][leave open])

Attachments

(3 files)

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
Attached file stack trace
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
OS: Mac OS X → All
Hardware: x86_64 → All
Whiteboard: [font-inflation]
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.
Attached patch wallpaperSplinter Review
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
Attachment #736063 - Flags: review?(sjohnson)
Comment on attachment 736063 [details] [diff] [review]
wallpaper

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

LGTM
Attachment #736063 - Flags: review?(sjohnson) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a1a4d67cc93
Severity: critical → normal
Flags: in-testsuite+
Summary: "ABORT: EnsureTextRun should have set font size inflation" with -moz-column, rel&abs pos → "ASSERTION: EnsureTextRun should have set font size inflation" with -moz-column, rel&abs pos
Whiteboard: [font-inflation] → [font-inflation][leave open]
It's going to go away for crashtest-e10s when 45 merges to aurora tomorrow, making the test permaorange.
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.
"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.
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.
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.
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.
You need to log in before you can comment on or make changes to this bug.