Closed Bug 1439954 Opened 2 years ago Closed 2 years ago

Intermittent run-by-manitest OSX debug assertion from various tests in layout/reftests/font-matching/reftest.list

Categories

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

x86_64
macOS
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: ahal, Assigned: jfkthame)

References

Details

Attachments

(1 file)

In bug 1353461 we are in the process of switching reftest to "run-by-manifest" mode. This means that between every manifest of tests we will restart Firefox. In general this leads to improved stability, though in a few cases existing tests that happened to depend on side-effects of previous tests can start failing. This is one of those cases.

This is very likely related to bug 1439937. Basically there appears to be an intermittent assertion (which happens more often than not), somewhere in or after the layout/reftests/font-family/localized-family-names-00{1-4}.html tests. So I think which test it happens in is just down to timing and luck.

The further we get from localized-family-names-001.html, the less likely it is to show up. I've seen it in weightmapping-45.html, but never weightmapping-458.html (though I'm sure it's possible).

Here is a push demonstrating this:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e140b0227133502411e5437db9815f529d1436b2

Example log:
https://treeherder.mozilla.org/logviewer.html#?job_id=163440603&repo=try&lineNumber=48080

We'll need to either fix this before we can land run-by-manifest, or land with the 'asserts-if' annotations in place.
OS: Unspecified → Mac OS X
Hardware: Unspecified → x86_64
Jonathan, can you please take a look?
Flags: needinfo?(jfkthame)
Looks like the problem is that when the gfxPlatformFontList code finishes loading font info (done in the background shortly after startup), it calls ForceGlobalReflow() to ensure that all content is reflowed in order to take account of font info that may not have been available when initially reflowed.

But ForceGlobalReflow() works by touching a preference that all presContext's observe for changes; and we can't do that from a content process. So it asserts.

Really, however, ForceGlobalReflow() is primarily meant to be used by the parent process when something in the system environment has changed (e.g. the user installed new fonts on their OS), such that everything needs to be reflowed (in all content processes, as well as the chrome process). That's not the case here: when a child process finishes loading font info, it should force a reflow only for its own content, not globally for all processes.

So what we need is an alternative that works in a content process and forces all its presContexts (but not other processes) to reflow.
Flags: needinfo?(jfkthame)
This fixes the assertion here (see https://treeherder.mozilla.org/#/jobs?repo=try&revision=0841c72497af6132d2badf97eb97d099280f71a0&group_state=expanded&selectedJob=163528027), by making gfxPlatform::ForceGlobalReflow safely usable in the content process. Note that it doesn't prevent these font-matching tests failing with a mismatch, however; that's a related issue, to be considered in bug 1439937.
Attachment #8952880 - Flags: review?(jmuizelaar)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Attachment #8952880 - Flags: review?(jmuizelaar) → review+
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b85566707940
Make gfxPlatform::ForceGlobalReflow when called in a content process trigger reflows only within that process; it should not try (and fail, with an assertion) to affect the parent or other content processes. r=jrmuizel
https://hg.mozilla.org/mozilla-central/rev/b85566707940
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.