1364714 - Update to Freetype 2.8

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect, P3)

Description

[NVD]

https://www.freetype.org/index.html#news

https://sourceforge.net/projects/freetype/files/freetype2/2.8/

"FreeType 2.8 has been released. CFF2 support and OpenType variation font handling is now complete; the auto-hinter now understands 25 more scripts, for example N'Ko and Tifinagh.

See here for a list of changes; noteworthy bug fixes are the handling of TrueType fonts: unhinted loading didn't work as expected, and the light auto-hinter used incorrect metrics."

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Also worth noting:
- CVE-2017-8105, CVE-2017-8287: Older FreeType versions have out-of-bounds writes caused by heap-based buffer overflows related to Type 1 fonts.

Not sure how severe that is for Firefox, though.

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Attachment: update freetype to version 2.8

Linux looks fine, Android less so.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ddece01e2f4dca6317cc134d6f1ed8629c5d7b93

Comment 3

[Jonathan Kew [:jfkthame]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #1)
> Also worth noting:
> - CVE-2017-8105, CVE-2017-8287: Older FreeType versions have out-of-bounds
> writes caused by heap-based buffer overflows related to Type 1 fonts.
> 
> Not sure how severe that is for Firefox, though.

Doesn't sound like a big concern; Type 1 fonts aren't supported via @font-face, so they wouldn't be a potential attack vector for Firefox.

(In reply to Ryan VanderMeulen [:RyanVM] from comment #2)
> Linux looks fine, Android less so.

:-(

Looking at some of the failures, I think many of them are happening because we're getting a change in the default line-height for some fonts -- in particular, for our default sans-serif -- perhaps related to a fix for "the light auto-hinter used incorrect metrics".

In many cases, this indicates the tests are making fragile assumptions and should be fixed to be more robust, but we should also make sure we understand what the effect of the change really is, and whether it's OK.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Sorry, I'm not going to have time to wade through the Android reftest issues.

Comment 5

[Jonathan Kew [:jfkthame]]

The majority of the reftest failures here occur because the new freetype version gives us a slightly greater 'normal' line-height for the Clear Sans font used as the default sans-serif on Android. The difference is quite marginal: with current Firefox on my Nexus tablet, line-height:normal computes to 21.5px, and with the updated freetype, it computes to 22px. (Exact results may vary depending on device resolution, because IIRC we "snap" ascent/descent metrics to device pixels.)

So the change does not appear to have any adverse impact on actual web content, AFAICS; but a number of reftests are fragile enough that it results in test failures. Explicitly setting line-height where necessary can make these more robust.

There are also a few cases where already-fuzzy tests just need an adjustment to the fuzz annotations due to changes in the antialiasing/rendering.

Comment 6

[Jonathan Kew [:jfkthame]]

Attachment: Fix fragile reference case for multicol-gap-001 test

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: Set explicit line-height on background-position-vrl-018-ref file for better reliability

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: Set explicit line-height in position-relative tests, to avoid fragile dependence on default font's metrics

Comment 9

[Jonathan Kew [:jfkthame]]

Attachment: Adjust fuzzy() annotation for box-decoration reftest

Comment 10

[Jonathan Kew [:jfkthame]]

Attachment: Modify text to avoid possibly-overflowing descender in last line of orthogonal float

Comment 11

[Jonathan Kew [:jfkthame]]

Attachment: Add fuzzy() annotation to mathml-mmultiscript test to allow for changed freetype rendering

Comment 12

[Jonathan Kew [:jfkthame]]

Attachment: Adjust fuzzy() annotation for table-background test to allow for change in freetype's rendering

Comment 13

[Jonathan Kew [:jfkthame]]

Attachment: Adjust fuzzy() annotation for pre-line-1 test to account for change in freetype's font rendering

Comment 14

[Jonathan Kew [:jfkthame]]

Attachment: Increase line-height to allow for default font with larger metrics

Comment 15

[Jonathan Kew [:jfkthame]]

Attachment: Adjust dimensions in fixed-table-layout-010 tests to allow for increased font metrics without disrupting layout of test elements

Comment 16

[Jonathan Kew [:jfkthame]]

Attachment: Avoid potential unwanted scrollbar when rendering test file

Comment 17

[Daniel Holbert [:dholbert]]

Comment on attachment 8898410 [details] [diff] [review]
Fix fragile reference case for multicol-gap-001 test

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

::: layout/reftests/w3c-css/received/css-multicol-1/multicol-gap-001-ref.xht
@@ +5,5 @@
>    <link rel="author" title="Opera Software ASA" href="http://www.opera.com/" />
>    <link rel="reviewer" title="Gérard Talbot" href="http://www.gtalbot.org/BrowserBugsSection/css21testsuite/" /> <!-- 2013-08-05 -->
>    <meta name="flags" content="image" />
>    <style type="text/css"><![CDATA[
> +  div {margin: 1.25em; line-height: 1;}

We're not really supposed to edit reftests in the "received" subdirectory, because our edits will be lost/stomped-on when we re-sync.

Better to mark the test as fuzzy/failing in the reftest manifest, and (optionally) submit a fix upstream, I think?

Comment 18

[Jonathan Kew [:jfkthame]]

Attachment: Adjust fuzzy() annotations on async-scrollbar tests

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 8898412 [details] [diff] [review]
Set explicit line-height on background-position-vrl-018-ref file for better reliability

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

(previous comment applies to this tweak as well)

Comment 20

[Jonathan Kew [:jfkthame]]

(In reply to Daniel Holbert [:dholbert] from comment #17)
> Comment on attachment 8898410 [details] [diff] [review]
> Fix fragile reference case for multicol-gap-001 test
> 
> Review of attachment 8898410 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/reftests/w3c-css/received/css-multicol-1/multicol-gap-001-ref.xht
> @@ +5,5 @@
> >    <link rel="author" title="Opera Software ASA" href="http://www.opera.com/" />
> >    <link rel="reviewer" title="Gérard Talbot" href="http://www.gtalbot.org/BrowserBugsSection/css21testsuite/" /> <!-- 2013-08-05 -->
> >    <meta name="flags" content="image" />
> >    <style type="text/css"><![CDATA[
> > +  div {margin: 1.25em; line-height: 1;}
> 
> We're not really supposed to edit reftests in the "received" subdirectory,
> because our edits will be lost/stomped-on when we re-sync.
> 
> Better to mark the test as fuzzy/failing in the reftest manifest, and
> (optionally) submit a fix upstream, I think?

This has been merged upstream already, so we'll get it when we next take an update from there. So applying the patch here is just an interim measure.

https://github.com/w3c/web-platform-tests/commit/cb90869af81f0cd7d184a44137c62ce73741b00e

Comment 21

[Jonathan Kew [:jfkthame]]

(In reply to Daniel Holbert [:dholbert] from comment #19)
> Comment on attachment 8898412 [details] [diff] [review]
> Set explicit line-height on background-position-vrl-018-ref file for better
> reliability
> 
> Review of attachment 8898412 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (previous comment applies to this tweak as well)

For this one, I just opened a PR upstream, awaiting review there.

But we can mark them as failing or fuzzy for now rather than applying the fixes locally, if you prefer.

Comment 22

[Daniel Holbert [:dholbert]]

(In reply to Jonathan Kew (:jfkthame) from comment #20)
> This has been merged upstream already, so we'll get it when we next take an
> update from there. So applying the patch here is just an interim measure.
> 
> https://github.com/w3c/web-platform-tests/commit/cb90869af81f0cd7d184a44137c62ce73741b00e

Ah, OK - thanks!  Could you link to this commit (or the merged pull request, https://github.com/w3c/web-platform-tests/pull/6879 ) in the extended commit message, with a note about the fact that this fix has already happened upstream?

(In reply to Jonathan Kew (:jfkthame) from comment #21)
> (In reply to Daniel Holbert [:dholbert] from comment #19)
> > Comment on attachment 8898412 [details] [diff] [review]
> > Set explicit line-height on background-position-vrl-018-ref file for better
> > reliability
> For this one, I just opened a PR upstream, awaiting review there.

Gotcha - indeed, looks like this is https://github.com/w3c/web-platform-tests/pull/6924 .  As with the other patch, please link to this in the commit message.

> But we can mark them as failing or fuzzy for now rather than applying the fixes locally, if you prefer.

I'm fine with taking these fixes as long as they're happening upstream too.

Comment 23

[Daniel Holbert [:dholbert]]

Comment on attachment 8898410 [details] [diff] [review]
Fix fragile reference case for multicol-gap-001 test

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

r=me with a link to upstream pull request or commit, in the extended commit message.

Comment 24

[Daniel Holbert [:dholbert]]

Comment on attachment 8898412 [details] [diff] [review]
Set explicit line-height on background-position-vrl-018-ref file for better reliability

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

r=me with a link to upstream pull request or commit, in the extended commit message.

Comment 25

[Daniel Holbert [:dholbert]]

Comment on attachment 8898414 [details] [diff] [review]
Adjust fuzzy() annotation for box-decoration reftest

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

::: layout/reftests/css-break/reftest.list
@@ +1,5 @@
>  default-preferences pref(layout.css.box-decoration-break.enabled,true)
>  
>  == box-decoration-break-1.html box-decoration-break-1-ref.html
>  fuzzy(1,20) fuzzy-if(skiaContent,1,700) fuzzy-if(webrender,4-4,3273-3273) == box-decoration-break-with-inset-box-shadow-1.html box-decoration-break-with-inset-box-shadow-1-ref.html
> +fuzzy(16,460) fuzzy-if(Android,10,3673) fuzzy-if(skiaContent,57,1330) fuzzy-if(styloVsGecko,2,1410) == box-decoration-break-with-outset-box-shadow-1.html box-decoration-break-with-outset-box-shadow-1-ref.html # Bug 1386543

Do you have a recent Try run that shows the mismatch here? (I tried the link from comment 2, but the reftest-analyzer "graph" links don't work because it seems the .log files are now 404.)

It looks like the change here is from...
 fuzzy-if(skiaContent,57,374)
...to
 fuzzy-if(skiaContent,57,1330)

And that's a big enough regression in nontrivially-mismatching-pixels that I'm curious what the mismatch actually looks like in practice...

Comment 26

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

Even better, would you mind running an update from upstream?  We don't have a process for doing that regularly, though we should...

(Though if it's a mess of annotation, that might be another story...)

Comment 27

[Daniel Holbert [:dholbert]]

Comment on attachment 8898414 [details] [diff] [review]
Adjust fuzzy() annotation for box-decoration reftest

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

::: layout/reftests/css-break/reftest.list
@@ +1,5 @@
>  default-preferences pref(layout.css.box-decoration-break.enabled,true)
>  
>  == box-decoration-break-1.html box-decoration-break-1-ref.html
>  fuzzy(1,20) fuzzy-if(skiaContent,1,700) fuzzy-if(webrender,4-4,3273-3273) == box-decoration-break-with-inset-box-shadow-1.html box-decoration-break-with-inset-box-shadow-1-ref.html
> +fuzzy(16,460) fuzzy-if(Android,10,3673) fuzzy-if(skiaContent,57,1330) fuzzy-if(styloVsGecko,2,1410) == box-decoration-break-with-outset-box-shadow-1.html box-decoration-break-with-outset-box-shadow-1-ref.html # Bug 1386543

Also -- it looks like the failure here was on Android, and this manifest line has annotations for *both* "Android" *and* "skiaContent". Current mozilla-central has fuzzy-if(Android,10,3673) fuzzy-if(skiaContent,57,374)

Is there a reason you went for adjusting the skiaContent annotation, rather than the Android one? (IIRC, "skiaContent" is true on multiple platforms, and it seems like a shame to reduce the strictness on multiple platforms, for what seems to be an Android-specific issue per comment 2...)

Comment 28

[Jonathan Kew [:jfkthame]]

(In reply to Daniel Holbert [:dholbert] from comment #25)
> Comment on attachment 8898414 [details] [diff] [review]
> Adjust fuzzy() annotation for box-decoration reftest

> Do you have a recent Try run that shows the mismatch here? (I tried the link
> from comment 2, but the reftest-analyzer "graph" links don't work because it
> seems the .log files are now 404.)

The logs from https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d1e561d09b55bc81fcf5695a7617064344bdf7c are still available, it looks like. I think you'll find all these failures there.

Comment 29

[Jonathan Kew [:jfkthame]]

(In reply to Daniel Holbert [:dholbert] from comment #27)
> Comment on attachment 8898414 [details] [diff] [review]
> Is there a reason you went for adjusting the skiaContent annotation, rather
> than the Android one? (IIRC, "skiaContent" is true on multiple platforms,
> and it seems like a shame to reduce the strictness on multiple platforms,
> for what seems to be an Android-specific issue per comment 2...)

IIRC, I first adjusted the Android annotation, but then it appeared that the Skia one must be overriding it in our current configuration, so I switched to doing that one instead. But I guess the better option is probably to re-order them so that the Android annotation comes last; I'll give that a try and check that it works as expected.

Comment 30

[Daniel Holbert [:dholbert]]

That makes sense -- I've definitely been bitten by annoying one-upsmanship between fails-if/fuzzy-if annotations in the past.  I forget the precedence rules, but I wouldn't be surprised if a reordering is needed.

(In reply to Jonathan Kew (:jfkthame) from comment #28)
> The logs from
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d1e561d09b55bc81fcf5695a7617064344bdf7c are still
> available, it looks like. I think you'll find all these failures there.

Thanks! But -- in that try run "R7", I'm seeing box-decoration-break-with-outset-box-shadow-1.html having "max difference: 57, number of differing pixels: 400" (which is only slightly-worse than our current fuzzy-if(skiaContent,57,374)).  I'd r+ a patch that bumps 374 to 400 there.

But your box-decoration patch was bumping the pixels to 1330 (much more than 400).  Maybe that was a mistake? Or maybe sometimes there's a larger difference?

Here's a link to the Try run with that job selected:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d1e561d09b55bc81fcf5695a7617064344bdf7c&selectedJob=121051154

Comment 31

[Daniel Holbert [:dholbert]]

Comment on attachment 8898415 [details] [diff] [review]
Modify text to avoid possibly-overflowing descender in last line of orthogonal float

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

r=me on this possibly-overflowing descender one (makes sense -- sneaky sideways-and-protruding "y" character)

Comment 32

[Daniel Holbert [:dholbert]]

Comment on attachment 8898416 [details] [diff] [review]
Add fuzzy() annotation to mathml-mmultiscript test to allow for changed freetype rendering

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

r=me on this mathml one, with a followup filed & noted on the reftest.list line.

::: layout/reftests/mathml/reftest.list
@@ +204,5 @@
>  == opentype-fraction-dynamic-linethickness.html opentype-fraction-dynamic-linethickness-ref.html
>  == operator-1.xhtml operator-1-ref.xhtml
>  == scriptshift-1.xhtml scriptshift-1-ref.xhtml
>  == number-size-1.xhtml number-size-1-ref.xhtml
> +fuzzy-if(skiaContent,1,80) fuzzy-if(Android,255,105) skip-if(winWidget) == multiscripts-1.html multiscripts-1-ref.html # Windows: bug 1314684

Hmm... I guess this is fine, though "fuzzy" with color-channel=255 and a nontrivial number of pixels sounds more like a failure than fuzziness.

I think this is really a test-bug, actually! The test run is https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d1e561d09b55bc81fcf5695a7617064344bdf7c&selectedJob=121051123 and here's my observation:
 - The mismatch is on the msup / msubsup line, the first "AAAbbb" (of two) is missing a 1px stripe of of red background at the bottom. (The testcase is missing that stripe, but the reference case has it.)
 - I see a similar difference between the testcase and reference if I view them in Firefox Nightly & zoom in with full-page-zoom as much as Firefox allows me to.

Could you file a bug on this mismatch and mention it in the comment at the end of the line here?

Comment 33

[Jonathan Kew [:jfkthame]]

(In reply to Daniel Holbert [:dholbert] from comment #30)
> But your box-decoration patch was bumping the pixels to 1330 (much more than
> 400).  Maybe that was a mistake? Or maybe sometimes there's a larger
> difference?

That'll be the difference I got when running the tests locally (on a higher-resolution device than we use in automation), so I picked the value that silenced the failure on a local run as well. We could stay with the lower value based on what tryserver shows, though, and accept a greater tendency for the test to fail locally depending on people's configurations. (It would be far from the only such test!) WDYT?

Comment 34

[Jonathan Kew [:jfkthame]]

(In reply to Daniel Holbert [:dholbert] from comment #30)

> (In reply to Jonathan Kew (:jfkthame) from comment #28)
> > The logs from
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d1e561d09b55bc81fcf5695a7617064344bdf7c are still
> > available, it looks like. I think you'll find all these failures there.
> 
> Thanks! But -- in that try run "R7", I'm seeing
> box-decoration-break-with-outset-box-shadow-1.html having "max difference:
> 57, number of differing pixels: 400" (which is only slightly-worse than our
> current fuzzy-if(skiaContent,57,374)).  I'd r+ a patch that bumps 374 to 400
> there.
> 
> But your box-decoration patch was bumping the pixels to 1330 (much more than
> 400).  Maybe that was a mistake? Or maybe sometimes there's a larger
> difference?

FWIW, I tried running this locally (Nexus 10) *without* the freetype update, and I get

REFTEST TEST-UNEXPECTED-FAIL | http://192.168.1.65:8888/tests/layout/reftests/css-break/box-decoration-break-with-outset-box-shadow-1.html == http://192.168.1.65:8888/tests/layout/reftests/css-break/box-decoration-break-with-outset-box-shadow-1-ref.html | image comparison, max difference: 57, number of differing pixels: 1239

So that's already a lot more than the 374 in the current annotation, and not much less than the 1330 I get with the updated freetype.

Comment 35

[Jonathan Kew [:jfkthame]]

Attachment: Adjust fuzzy() annotation for box-decoration reftest

So this version, which re-orders the annotations and then actually *reduces* the number of fuzzy pixels in the Android annotation, seems to be sufficient in local testing. (I'll double-check on tryserver before any of this lands, though.)

Comment 36

[Jonathan Kew [:jfkthame]]

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #26)
> Even better, would you mind running an update from upstream?  We don't have
> a process for doing that regularly, though we should...
> 
> (Though if it's a mess of annotation, that might be another story...)

Once I got the import script to run successfully, it didn't look too bad at all. I've filed it separately as bug 1391717.

This also led me to wonder how much duplication we have between tests we're importing here, and what we run under testing/web-platform. (Maybe you know the answer?) Rather than proceed down that rabbit-hole right now, I filed bug 1391730.

Comment 37

[Daniel Holbert [:dholbert]]

Comment on attachment 8898417 [details] [diff] [review]
Adjust fuzzy() annotation for table-background test to allow for change in freetype's rendering

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

r=me

Comment 38

[Daniel Holbert [:dholbert]]

Comment on attachment 8898418 [details] [diff] [review]
Adjust fuzzy() annotation for pre-line-1 test to account for change in freetype's font rendering

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

r=me

Comment 39

[Daniel Holbert [:dholbert]]

Comment on attachment 8898419 [details] [diff] [review]
Increase line-height to allow for default font with larger metrics

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

Commit message nit:
> Bug 1364714 - Increase line-height to allow for default font with larger metrics.

s/line-height/line-height in some ruby reftests/

Comment 40

[Daniel Holbert [:dholbert]]

Comment on attachment 8898420 [details] [diff] [review]
Adjust dimensions in fixed-table-layout-010 tests to allow for increased font metrics without disrupting layout of test elements

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

r=me

Comment 41

[Daniel Holbert [:dholbert]]

Comment on attachment 8898421 [details] [diff] [review]
Avoid potential unwanted scrollbar when rendering test file

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

Commit message nit:
>  Bug 1364714 - Avoid potential unwanted scrollbar when rendering test file.

This commit message is bit too vague -- it's unclear from this description whether it's a code change vs. a test change. Maybe use something like: "Adjust reftest text-emphasis-line-height-001z.html to avoid potential unwanted scrollbar"?

r=me with some sort of clarification there.

::: layout/reftests/w3c-css/submitted/text-decor-3/text-emphasis-line-height-001z.html
@@ +10,5 @@
>  textarea {
>    all: inherit;
>    width: 100%;
> +  height: 10em; /* ensure there's plenty of height even if the default font is quite tall,
> +                   to avoid risk of a vertical scrollbar showing up */

s/showing up/showing up inside textarea/

(At first, I interpreted this as talking about a scrollbar on the *viewport*, and I was confused.)

Comment 42

[Daniel Holbert [:dholbert]]

Comment on attachment 8898422 [details] [diff] [review]
Adjust fuzzy() annotations on async-scrollbar tests

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

::: gfx/layers/apz/test/reftest/reftest.list
@@ +4,3 @@
>  fuzzy-if(Android,4,5) skip-if(!Android) pref(apz.allow_zooming,true) == async-scrollbar-1-h.html async-scrollbar-1-h-ref.html
>  fuzzy-if(Android,3,5) skip-if(!Android) pref(apz.allow_zooming,true) == async-scrollbar-1-vh.html async-scrollbar-1-vh-ref.html
> +fuzzy-if(Android,43,8) skip-if(!Android) pref(apz.allow_zooming,true) == async-scrollbar-1-v-rtl.html async-scrollbar-1-v-rtl-ref.html

This change -- from fuzzy-if(Android,1,2) to fuzzy-if(Android,43,8) -- is quite large.  The old annotation was extremely subtle (a rounding error on 2 pixels), whereas the new annotation is arguably a test failure (8 pixels that differ by quite a lot).  Looking at the reftest snapshot, the scrollbar is simply at a different position between the testcase & reference case -- it's 1px lower in the reference case than in the testcase, I think (and this produces 4px of mismatch at the top and 4px at the bottom, from places where one file is painting the scrollbar edges and the other is not).

If this gets triggered by this freetype update, maybe this test just depends on font metrics in a way that it should not?  If there's no obvious easy fix, could you file a followup on this and CC someone who's worked on these tests?

(This is for both async-scrollbar-1-v.html and async-scrollbar-1-v-rtl.html)

Comment 43

[Daniel Holbert [:dholbert]]

Comment on attachment 8898857 [details] [diff] [review]
Adjust fuzzy() annotation for box-decoration reftest

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

::: layout/reftests/css-break/reftest.list
@@ +1,5 @@
>  default-preferences pref(layout.css.box-decoration-break.enabled,true)
>  
>  == box-decoration-break-1.html box-decoration-break-1-ref.html
>  fuzzy(1,20) fuzzy-if(skiaContent,1,700) fuzzy-if(webrender,4-4,3273-3273) == box-decoration-break-with-inset-box-shadow-1.html box-decoration-break-with-inset-box-shadow-1-ref.html
> +fuzzy(16,460) fuzzy-if(skiaContent,57,374) fuzzy-if(Android,57,1330) fuzzy-if(styloVsGecko,2,1410) == box-decoration-break-with-outset-box-shadow-1.html box-decoration-break-with-outset-box-shadow-1-ref.html # Bug 1386543

Please add a note in the extended commit message to note that you're re-ordering the annotations (and briefly why -- i.e. that the old Android annotation had become suppressed/ignored effectively due to clashing with the more general & higher-priority-due-to-ordering skiaContent annotation, or whatever)

Comment 44

[Jonathan Kew [:jfkthame]]

Sigh... I thought we were about ready to land this, with all the android reftest issues addressed one way or another. But tryserver indicates[1] that we also get a build failure in pdfium code (which has been added to the tree -- see bug 1368948 -- since the original try run in comment 2 was done).

This occurs because pdfium relies on an internal freetype function ft_get_adobe_glyph_index(), and changes in freetype 2.8 mean this is no longer publicly visible even when the (private) header file is included, unless the including file #defines a specific symbol.

There's already an upstream issue[2] related to this, so I expect something will be worked out between the pdfium and freetype projects, but for the time being we'll need to patch around it locally.


[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a2bf2b0cf87ae8d64083b2c65f63977ec5f7e0e&group_state=expanded&selectedJob=124518641
[2]  https://bugs.chromium.org/p/pdfium/issues/detail?id=733

Comment 45

[Jonathan Kew [:jfkthame]]

Attachment: Patch PDFium for compatibility with freetype 2.8, where the ft_get_adobe_glyph_index function is guarded by DEFINE_PS_TABLES

As noted above, this will probably be superseded once pdfium's issue #733 is fixed upstream, but for the time being this should be sufficient to fix the build failure.

Comment 46

[Jonathan Kew [:jfkthame]]

Attachment: Cherry-pick commit c87fec0299533fee9b20505a4955c7f2f5c5bed7 from upstream freetype, to avoid bloating the build with duplicate PSname data tables due to PDFium's use of ft_get_adobe_glyph_index

To avoid defining freetype's psnames data twice (due to pdfium including the internal header), we should cherry-pick this recent freetype commit (upstream commit message '[psnames] Really fix issue #49949'), which separates the preprocessor symbol controlling definitions from declarations. This allows pdfium to #include the header with DEFINE_PS_TABLES so that it gets access to the function it needs, without duplicating the definition of all the data.

Comment 47

[Jonathan Kew [:jfkthame]]

Comment on attachment 8867512 [details] [diff] [review]
update freetype to version 2.8

rs=me for the import of freetype 2.8 here, which looks fine (though it can't actually land until we have all the associated test fixups and other patches ready to go with it).

Comment 48

[Jonathan Kew [:jfkthame]]

Try run with the two patches from comment 46 and comment 47 added, to check that we can now build on Windows:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7cd0c939fc19ca1aa0edf6fa588cf2d123159dd8

Comment 49

[Kartikaya Gupta (email:[e-mail])]

(In reply to Daniel Holbert [:dholbert] from comment #42)
> ::: gfx/layers/apz/test/reftest/reftest.list
> @@ +4,3 @@
> >  fuzzy-if(Android,4,5) skip-if(!Android) pref(apz.allow_zooming,true) == async-scrollbar-1-h.html async-scrollbar-1-h-ref.html
> >  fuzzy-if(Android,3,5) skip-if(!Android) pref(apz.allow_zooming,true) == async-scrollbar-1-vh.html async-scrollbar-1-vh-ref.html
> > +fuzzy-if(Android,43,8) skip-if(!Android) pref(apz.allow_zooming,true) == async-scrollbar-1-v-rtl.html async-scrollbar-1-v-rtl-ref.html
> 
> This change -- from fuzzy-if(Android,1,2) to fuzzy-if(Android,43,8) -- is
> quite large.  The old annotation was extremely subtle (a rounding error on 2
> pixels), whereas the new annotation is arguably a test failure (8 pixels
> that differ by quite a lot).  Looking at the reftest snapshot, the scrollbar
> is simply at a different position between the testcase & reference case --
> it's 1px lower in the reference case than in the testcase, I think (and this
> produces 4px of mismatch at the top and 4px at the bottom, from places where
> one file is painting the scrollbar edges and the other is not).

I see bug 1392259 was filed for this, which is great. It's clear that the freetype update doesn't actually break async scrollbar dragging; this reftest must be getting affected accidentally. To ensure that the now-large fuzzy value doesn't paper over future failures though, I'd like to see the fuzzy-if annotation use precise ranges, i.e. fuzzy-if(Android,43-43,8-8). That way if any future change affects this reftest behaviour even by a pixel we can re-examine it to ensure the behaviour hasn't been broken.

Comment 50

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d653fc499c9b9c30b16c8ffd090f0f3553a41cfb
Bug 1364714 - Adjust fuzzy() annotation for box-decoration reftest. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/40e902ed7dbe983d096fa7d6218db29152119d8b
Bug 1364714 - Fix fragile reference case for multicol-gap-001 test. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/a297dc40177e8a2face062375e2ff4e22a693af0
Bug 1364714 - Set explicit line-height on background-position-vrl-018-ref file for better reliability. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/b02a1d99e6030a631091c6ed15681f79ba779a1f
Bug 1364714 - Set explicit line-height in position-relative tests, to avoid fragile dependence on default font's metrics. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/7634973c46ade547944ce62415cbfdfde20adca2
Bug 1364714 - Modify text to avoid possibly-overflowing descender in last line of orthogonal float. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa5a89aac5ea25ea64c36d2fc50254f60814850a
Bug 1364714 - Add fuzzy() annotation to mathml-mmultiscript test to allow for changed freetype rendering. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/2bd5a2d0ea09febca69e415c931697a445c65a45
Bug 1364714 - Adjust fuzzy() annotation for table-background test to allow for change in freetype's rendering. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/80ca8faf1dac1f487946ad6564769a042e4b5b09
Bug 1364714 - Adjust fuzzy() annotation for pre-line-1 test to account for change in freetype's font rendering. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b915e42b36d92cb47494cb85c5447e8526b09d1
Bug 1364714 - Increase line-height in some ruby reftests to allow for default font with larger metrics. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f758a7673d3d87cfaf815ba793998051631ff7d
Bug 1364714 - Adjust dimensions in fixed-table-layout-010 tests to allow for increased font metrics without disrupting layout of test elements. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/e0cf94cb8666e16dad764d0b49d79e3188c3bc6e
Bug 1364714 - Adjust reftest text-emphasis-line-height-001z.html to avoid potential unwanted scrollbar. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e26a8c07caee0885fef4d1359e1d0b37c69b8db
Bug 1364714 - Adjust fuzzy() annotations on async-scrollbar tests. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff188112de35e14dacd836293e477f5f70ca79c5
Bug 1364714 - Patch PDFium for compatibility with freetype 2.8, where the ft_get_adobe_glyph_index function is guarded by DEFINE_PS_TABLES. r=jwatt

https://hg.mozilla.org/integration/mozilla-inbound/rev/e3dda4fe9dae7d8afafd7d705a698e845c8a12fc
Bug 1364714 - Update Freetype to version 2.8. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f52db2530d3c55240ce9535be3a8d94d287a37a
Bug 1364714 - Cherry-pick commit c87fec0299533fee9b20505a4955c7f2f5c5bed7 from upstream freetype, to avoid bloating the build with duplicate PSname data tables due to PDFium's use of ft_get_adobe_glyph_index. r=ryanvm

Comment 51

[Jonathan Kew [:jfkthame]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #49)
> I see bug 1392259 was filed for this, which is great. It's clear that the
> freetype update doesn't actually break async scrollbar dragging; this
> reftest must be getting affected accidentally. To ensure that the now-large
> fuzzy value doesn't paper over future failures though, I'd like to see the
> fuzzy-if annotation use precise ranges, i.e. fuzzy-if(Android,43-43,8-8).
> That way if any future change affects this reftest behaviour even by a pixel
> we can re-examine it to ensure the behaviour hasn't been broken.

Sorry, I saw this comment right after pushing the patches above. :( I'll push a trivial follow-up to tighten the fuzzy-if as requested.

Comment 52

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b00b74e9d34bc219ac3c70fedb7efd79c78268ce
Bug 1364714 followup, use precise fuzzy-if() range in APZ scroll reftest annotations. r=me

Comment 53

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b00b74e9d34b
followup, use precise fuzzy-if() range in APZ scroll reftest annotations. r=me

Comment 54

[Kartikaya Gupta (email:[e-mail])]

Thanks!

Comment 55

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back these out for android reftest failures like https://treeherder.mozilla.org/logviewer.html#?job_id=124579672&repo=mozilla-inbound

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

Comment 56

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/0267fb03043a
Backed out 16 changesets for android reftest failures in out-of-bounds-selectedindex.html a=backout

Comment 57

[Jonathan Kew [:jfkthame]]

Sigh.... I thought I had enough tryserver coverage to be safe (e.g. comment 48), but apparently the android fuzziness on this test is debug-only, and it increased by one pixel. :(  Re-landing with the annotation adjusted accordingly.

Comment 58

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e6cd9dc49d54e193186c28fbe000da3630ca7e9
Bug 1364714 - (re-landing) Update Freetype to version 2.8, and fix/annotate tests affected by changes to font metrics & rasterization. r=dholbert,jwatt,ryanvm,jfkthame

Comment 59

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e6cd9dc49d5
(re-landing) Update Freetype to version 2.8, and fix/annotate tests affected by changes to font metrics & rasterization. r=dholbert,jwatt,ryanvm,jfkthame

Comment 60

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7e6cd9dc49d5

Comment 61

[Ionuț Goldan [:igoldan]]

Improvements noticed. Thanks for landing these!

== Change summary for alert #8900 (as of August 21 2017 13:48 UTC) ==

Improvements:

  2%  compiler warnings summary windows2012-64 pgo      154.00 -> 151.00
  2%  compiler warnings summary windows2012-64 opt      154.00 -> 151.00
  2%  compiler warnings summary windows2012-64-noopt debug 160.00 -> 157.00
  2%  compiler warnings summary windows2012-64 debug    160.00 -> 157.00

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=8900