Closed Bug 1561792 Opened 6 years ago Closed 6 years ago

Intermittent TEST-UNEXPECTED-PASS | w3c-css/submitted/variables/variable-font-face-01.html == w3c-css/submitted/variables/variable-font-face-01-ref.html | image comparison, max difference: 0, number of differing pixels: 0

Categories

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

Product:
Core
Component:
Graphics: Text
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla70
Tracking Flags:
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox68 --- unaffected
firefox69 --- unaffected
firefox70 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: dbaron)

References

Details

(Keywords: intermittent-failure, regression, Whiteboard: [retriggered])

Attachments

(2 files)

Bug 1561792 - Handle dynamic changes of gfx.font_ahem_antialias_none (and make FlushFontAndWordCaches stronger for other callers) and fix a few test failures/intermittents using it.
47 bytes, text/x-phabricator-request
Details | Review
Bug 1561792 - Rename the pref gfx.font_ahem_antialias_none to gfx.font_rendering.ahem_antialias_none to avoid the additional pref observer added in the previous patch.
47 bytes, text/x-phabricator-request
Details | Review

https://wiki.mozilla.org/Bug_Triage#Intermittent_Test_Failure_Cleanup

Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → INCOMPLETE

I'll look more closely tomorrow or Monday, but my initial guess is that it's a latent bug that's a result of rechunking of reftests, and the fuzzy-if() annotation needs to be adjusted. I'm also highly suspicious of that annotation to begin with given the number of pixels; I suspect the test is simply failing.

(Note that the screenshots in the logs show that the UNEXPECTED-PASS behavior that's happening is actually the test passing -- it's not blank or something like that.)

Also, I'd note these unexpected passes have spiked and then gone away three times:

  • June 26-27
  • July 22-23
  • August 1-3 (i.e., August 1 - present)

which also suggests it might be related to test chunking.

Here's the intermittent failures graph up to the present.

And here's a try run to better understand what the "normal" failures are, which might help understand the intermittent passes.

OK, the normal failure mode is actually basically fuzz, not a true failure -- the annotation is basically correct. The normal failure mode is that the Ahem glyphs are antialiased in the test and sharp in the reference. That seems like it might be a subpixel alignment difference.

In the case where we're getting an unexpected pass, the glyphs are antialiased in both test and reference.

Status: REOPENED → NEW
Component: CSS Parsing and Computation → Layout: Text and Fonts
Flags: needinfo?(dbaron)

(In reply to David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7 from comment #10)

OK, the normal failure mode is actually basically fuzz, not a true failure -- the annotation is basically correct. The normal failure mode is that the Ahem glyphs are antialiased in the test and sharp in the reference. That seems like it might be a subpixel alignment difference.

The glyphs are sharp in the reference because we explicitly disable antialiasing for "Ahem" when running tests (because so many tests compare Ahem glyphs to images or <div>s with a solid background or whatever). But this is done only for the exact font-family name "Ahem".[1]

Here, the reference files use "Ahem", but the test files load a copy of ahem.ttf with a different family name, so the disable-AA option should not apply to it.

In the case where we're getting an unexpected pass, the glyphs are antialiased in both test and reference.

So the question, then, is why the option to disable AA for Ahem is sometimes failing to take effect for the reference files.

[1] https://searchfox.org/mozilla-central/rev/0ffa9e372df56c95547fed9c3433ddec4fbf6f11/gfx/thebes/gfxFont.cpp#772-776

Component: Layout: Text and Fonts → Graphics: Text

It also seems like the tests could disable that pref -- like the test a few lines above in the manifest does -- but then there's the question of why that test also fails.

(And yes, I did poke around a little before seeing the previous comment, and found that (a) there were no subpixel layout differences and (b) at least locally, the test passes when I put that pref in the manifest.)

This bug is also presumably the same problem as bug 1559851.

And a bunch of the annotations come from bug 1545859.

Here's another try run to poke at the results of.

Should changes to gfx.font_ahem_antialias_none call FlushFontAndWordCaches or something similar?

Flags: needinfo?(jfkthame)

(And I think that pref failing to flush the necessary caches could explain the behavior, given that a test just a few lines earlier in the manifest toggles that pref. If that toggling happens to happen after the cache(s) were cleared for other reasons, it might cache a gfxFont with the wrong mAntialiasOption, and that might get used in this test.)

And here's a try run to test that theory; so far it seems to fix the existing failures on Mac but not Windows (with only Windows debug results so far).

(In reply to David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7 from comment #19)

And here's a try run to test that theory; so far it seems to fix the existing failures on Mac but not Windows (with only Windows debug results so far).

That does sound plausible, yes.

Not immediately sure why you're still seeing those windows-debug failures.

Flags: needinfo?(jfkthame)

Would gfxFontCache::Flush be more effective? It's not immediately clear to me what the difference is between that and just AgeAllGenerations, although it has much fewer callers.

(Here's another try run for that theory... which I think makes sense after I read a bit of the gfxFontCache code.)

It does fix it -- I think that suggests that in Windows debug builds something is holding on to gfxFont objects longer than on other platforms, causing them to get reused even though they've aged out of the expiration tracker, but just because they haven't been destroyed?

Looking at the code, I think it makes more sense to call gfxFontCache::Flush than AgeAllGenerations there, which won't clear out the cache's hashtable of "active" fonts.

You could then also drop the call to FlushShapedWordCaches, which will be a no-op when the font cache has just been flushed anyway.

Does that apply to all callers of gfxPlatform::FlushFontAndWordCaches()?

Flags: needinfo?(jfkthame)

I think so. Looking at all the callsites in searchfox, I can't see any reason why they wouldn't want the more thorough "flush" that gfxFontCache::Flush does.

Flags: needinfo?(jfkthame)
Pushed by dbaron@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/847d101909cb Handle dynamic changes of gfx.font_ahem_antialias_none (and make FlushFontAndWordCaches stronger for other callers) and fix a few test failures/intermittents using it. r=jfkthame https://hg.mozilla.org/integration/autoland/rev/3f835dfd987d Rename the pref gfx.font_ahem_antialias_none to gfx.font_rendering.ahem_antialias_none to avoid the additional pref observer added in the previous patch. r=jfkthame

https://hg.mozilla.org/mozilla-central/rev/847d101909cb
https://hg.mozilla.org/mozilla-central/rev/3f835dfd987d

Status: ASSIGNED → RESOLVED
Closed: 6 years ago → 6 years ago
status-firefox70: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: