Closed Bug 1694174 Opened 5 months ago Closed 4 months ago

Enable the shared font-list by default for all channels

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
89 Branch
Fission Milestone M8
Tracking Status
firefox89 --- disabled
firefox90 --- fixed
firefox91 --- fixed

People

(Reporter: jfkthame, Assigned: jfkthame)

References

(Blocks 1 open bug, Regressed 2 open bugs)

Details

Attachments

(2 files)

Currently gfx.e10s.font-list.shared is enabled by default on Nightly/early Beta, but disabled for release.

To reduce content-process memory footprint and startup time, we should get this enabled all the way to the Release channel.

Depends on: 1694149

How much memory does the shared font-list save per content process?

Since Fission will have launch many more content processes than e10s, the shared font-list might be nice to have for Fission MVP.

Fission Milestone: --- → ?

@ Jonathan, SearchAllFontsForChar crash bug 1667977 and src:local() bug 1694123 are blocking this bug. I see you already have a patch up to fix bug 1694123. Does bug 1667977 also need to be fixed before we can let the shared font-list ride the trains to late Beta and Release?

Fission's current perf measurements are all from Nightly, so already include any perf and memory benefits from the shared font-list. Tracking for Fission M7 Beta milestone because we'd like to have the shared font-list in late Beta and when we ship to Release.

kmag estimates the shared font-list might save about 500 KB in each content process.

Fission Milestone: ? → M7
Flags: needinfo?(jfkthame)

Bug 1667977 currently means that on Windows there's a possibility of content processes crashing if the user installs/removes a bunch of fonts while the browser is running (so we get a flurry of notifications that trigger repeated, overlapping updates to our font list). This is probably not a common situation -- most users aren't installing and removing fonts frequently in the midst of a browser session -- but still, I'd be reluctant to ship something that is known to be crashy in such a scenario.

So I'd definitely prefer to have bug 1667977 fixed before it rides to Release. I've been attempting to debug but it's proving difficult to track down the root cause at the moment. (Not having a tool equivalent to rr or pernosco on Windows hurts...) I'll try to spend some more time digging into it this week.

Flags: needinfo?(jfkthame)

(In reply to Jonathan Kew (:jfkthame) from comment #3)

So I'd definitely prefer to have bug 1667977 fixed before it rides to Release. I've been attempting to debug but it's proving difficult to track down the root cause at the moment. (Not having a tool equivalent to rr or pernosco on Windows hurts...) I'll try to spend some more time digging into it this week.

A fix for SearchAllFontsForChar crash bug 1667977 probably won't be ready for 88 Beta, so I will move this bug from Fission milestone M7 to M8.

btw, the reporter of bug 1667977 says the crash is also reproducible on Linux, so you may be able to catch the crash in rr on Linux.

Fission Milestone: M7 → M8
Depends on: 1697666

This has been enabled on Nightly since 82, and also early Beta since 83. Now that a fix for bug 1667977 has landed, I think we should remove the early-beta condition and let this go all the way to release.

Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/622e8fe7e539
Pref-on the shared font-list by default for all channels. r=emilio
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → 89 Branch
Regressions: 1714543
Regressions: 1714197
Regressions: 1714318

Jonathan, we have already 4 regressions reported in 4 days since we shipped this feature in 89, among them 2 marked as S2 + a medium crasher. Should we consider deactivate this feature in a 89 dot release?

Flags: needinfo?(jfkthame)

If we're going to do a dot release, we might want to consider deactivating on Linux due to bug 1714282, which I think is actually the most serious issue we've seen. A patch has landed there (which I hope to uplift to 90), but for 89, I'd consider disabling it.

The crash on Windows (bug 1689379) is probably not too big of a concern; it's specifically at shutdown, and it's not really a "bug" that's crashing us, it's a timeout because directwrite is being slow, which we then detect as a hang and deliberately crash. Again, a patch has just landed, which I hope to uplift to 90. I suppose for 89, though, deactivating via the pref would be a better user experience.

I think the other issues were categorized as S2 simply due to being "regressions", but may not be so serious - there are several examples where users have odd font configurations, and have experienced a change because our behavior arguably became more correct; I don't think these type of edge cases would justify reverting the feature.

So - yes, I think deactivating in an 89.x dot-release, while we wait for bug 1689379 and bug 1714282 to be fixed in 90, may be a good option here.

Flags: needinfo?(jfkthame)
Regressions: 1714282
Regressions: 1664151

I'm not sure how to manage a patch intended for dot-release-only in phabricator, so simply attaching it here.

Approval Request Comment
[Feature/Bug causing the regression]: 1694174
[User impact if declined]: see regressions: bitmap font issues on linux; crashes for linux users with huge font directories; possible windows shutdown hang
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: simply reverts to earlier behavior
[Needs manual test from QE? If yes, steps to reproduce]: -
[List of other uplifts needed for the feature/fix]: -
[Is the change risky?]: no
[Why is the change risky/not risky?]: revert to previous code via pref flip
[String changes made/needed]: -

Attachment #9226420 - Flags: approval-mozilla-release?
Regressions: 1715975
Attachment #9226420 - Flags: approval-mozilla-release? → approval-mozilla-release+
Regressions: 1716433
You need to log in before you can comment on or make changes to this bug.