Enumerating fonts should not block about:preferences from being displayed

RESOLVED FIXED in Firefox 57

Status

()

enhancement
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: nhnt11, Assigned: myk)

Tracking

(Depends on 2 bugs)

Trunk
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(2 attachments, 8 obsolete attachments)

(Reporter)

Description

2 years ago
We are waiting a long time for fonts before we first display the preferences page. I think it's really unreasonable and see some options:

- Enumerate fonts off the main thread ("hard" for me to implement personally)

- Don't put the fonts list in the main page, and instead stick it in a dialog box like we already do for "advanced" fonts settings.

- Initialize the font menulist with the current font and size, and only do the expensive stuff when the widgets are actually interacted with. The main thread will still be blocked but IMO it's better than what we have now.

Here's a profile: https://perf-html.io/public/35285e294b67ffe12261eacfe40c8e1fc53e420a/calltree/?callTreeFilters=prefixjs-yLg&implementation=js&thread=0
(In reply to Nihanth Subramanya [:nhnt11] from comment #0)
> - Initialize the font menulist with the current font and size, and only do
> the expensive stuff when the widgets are actually interacted with. The main
> thread will still be blocked but IMO it's better than what we have now.

This looks like the right approach. There is nothing in the impl suggests that you can't get current/default font without enumerate the list first.

http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/gfx/src/nsThebesFontEnumerator.cpp#90-107
(Assignee)

Comment 2

2 years ago
(In reply to Nihanth Subramanya [:nhnt11] from comment #0)
> Here's a profile:
> https://perf-html.io/public/35285e294b67ffe12261eacfe40c8e1fc53e420a/
> calltree/?callTreeFilters=prefixjs-yLg&implementation=js&thread=0

Does this issue also happen on Windows/Linux, or is it Mac-specific?  In this profile (and my own profiles on my Mac), it looks like the Mac method [NSFontManager availableMembersOfFontFamily] is responsible.

That method was also a problem on startup at one point, per this blog post:

https://web.archive.org/web/20120423051354/https://blog.mozilla.org/nattokirai/2009/10/05/making-font-initialization-lazier/

It looks like that was resolved in bug 519445.  There's also a similar-looking bug 491283 on Windows startup perf, so the issue may well be cross-platform.
Posted patch initfontlist.diff (obsolete) — Splinter Review
Tentatively, this removes EnumerateFonts() from the hot path. The only problem with this approach is that the menu list needs get a full list to tell the correct width it should expand, so as soon as the full list is populated the width will jump. That can be hidden with a |min-width| style if we really want to do this.

Let me know if this is the right approach.
Comment hidden (mozreview-request)
I've updated my patch to a review-able and land-able state. The reviewer is set to Florian because :jaws is quite overloaded with other Preferences changes. Please let me know we should still wait for his review.

I intentionally didn't change the function name nor the function of FontBuilder.buildFontList() to keep it working in old Preferences page. This patch can surely port to there too, but I don't want to spend my time on that.
Assignee: nobody → timdream
Status: NEW → ASSIGNED

Comment 6

2 years ago
mozreview-review
Comment on attachment 8889304 [details]
Bug 1375978 - Don't enumerate all fonts when loading about:preferences,

https://reviewboard.mozilla.org/r/160366/#review165774

Looks reasonable to me, thanks! I'll be on PTO for the next two weeks, I suggest :nhnt11 to review the next version of the patch.

::: browser/components/preferences/in-content-new/main.js:1040
(Diff revision 1)
>                     fonttype: aIsSerif ? "serif" : "sans-serif" },
>                   { format: kFontSizeFmtVariable,
>                     type: "int",
>                     element: "defaultFontSize",
>                     fonttype: null }];
> -    for (var i = 0; i < prefs.length; ++i) {
> +    prefs.forEach(pref => {

I would prefer:
for (let pref of prefs) {

::: browser/components/preferences/in-content-new/main.js:1044
(Diff revision 1)
>                     fonttype: null }];
> -    for (var i = 0; i < prefs.length; ++i) {
> -      var preference = document.getElementById(prefs[i].format.replace(/%LANG%/, aLanguageGroup));
> +    prefs.forEach(pref => {
> +      var preference = document.getElementById(pref.format.replace(/%LANG%/, aLanguageGroup));
>        if (!preference) {
>          preference = document.createElement("preference");
> -        var name = prefs[i].format.replace(/%LANG%/, aLanguageGroup);
> +        var name = pref.format.replace(/%LANG%/, aLanguageGroup);

This line should be moved up a bit so that we can avoid doing the .replace call twice.

::: browser/components/preferences/in-content-new/main.js:1063
(Diff revision 1)
> +        if (pref.fonttype) {
> +          if (element.hasChildNodes()) {
> +            FontBuilder.buildFontList(aLanguageGroup, pref.fonttype, element);
> +          } else {
> +            FontBuilder.initFontList(aLanguageGroup, pref.fonttype, element, preference.value);
> +            window.requestIdleCallback(() =>

If there's something CPU intensive that's constantly putting events in the queue, this idle callback may never run.

Possible ways to avoid the problem:
- build the font list immediately when the user opens the popup, if it hasn't been built yet. (I think that's the correct solution, but requires a bit of effort)
- provide a timeout on the requestIdleCallback call, to ensure the callback will start no later than eg. after 1s. (easy)

::: toolkit/mozapps/preferences/fontbuilder.js:30
(Diff revision 1)
> +   */
> +  initFontList(aLanguage, aFontType, aMenuList, value) {
> +    let popup = document.createElement("menupopup");
> +    if (!value) {
> +      let defaultFont = this.enumerator.getDefaultFont(aLanguage, aFontType);
> +      if (!defaultFont) {

This can be simplified using the || operator.
Attachment #8889304 - Flags: review?(florian)
Comment on attachment 8889304 [details]
Bug 1375978 - Don't enumerate all fonts when loading about:preferences,

https://reviewboard.mozilla.org/r/160366/#review166632

::: browser/components/preferences/in-content-new/main.js:1063
(Diff revision 1)
> +        if (pref.fonttype) {
> +          if (element.hasChildNodes()) {
> +            FontBuilder.buildFontList(aLanguageGroup, pref.fonttype, element);
> +          } else {
> +            FontBuilder.initFontList(aLanguageGroup, pref.fonttype, element, preference.value);
> +            window.requestIdleCallback(() =>

I had to use popupshowing event and keeping the same popup element to make this work. That's more change to buildFontList().
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8889304 [details]
Bug 1375978 - Don't enumerate all fonts when loading about:preferences,

https://reviewboard.mozilla.org/r/160366/#review166718

::: browser/components/preferences/in-content-new/main.js:1040
(Diff revision 3)
> -    for (var i = 0; i < prefs.length; ++i) {
> -      var preference = document.getElementById(prefs[i].format.replace(/%LANG%/, aLanguageGroup));
> +    for (let pref of prefs) {
> +      let name = pref.format.replace(/%LANG%/, aLanguageGroup);
> +      let preference = document.getElementById(name);

Using both 'pref' and 'preference' variable names seems confusing. Can you rename one of them?

::: browser/components/preferences/in-content-new/main.js:1067
(Diff revision 3)
> +            FontBuilder.initFontList(aLanguageGroup, pref.fonttype, element, preference.value);
> +            let buildFullFontList = () => {
> +              element.removeEventListener('popupshowing', buildFullFontList);
> +              FontBuilder.buildFontList(aLanguageGroup, pref.fonttype, element);
> +            };
> +            element.addEventListener('popupshowing', buildFullFontList);

Please use the {once: true} argument to addEventListener so you don't need to call removeEventListener.
Comment on attachment 8889304 [details]
Bug 1375978 - Don't enumerate all fonts when loading about:preferences,

https://reviewboard.mozilla.org/r/160366/#review167162

::: browser/components/preferences/in-content-new/main.js:1067
(Diff revision 3)
> +            FontBuilder.initFontList(aLanguageGroup, pref.fonttype, element, preference.value);
> +            let buildFullFontList = () => {
> +              element.removeEventListener('popupshowing', buildFullFontList);
> +              FontBuilder.buildFontList(aLanguageGroup, pref.fonttype, element);
> +            };
> +            element.addEventListener('popupshowing', buildFullFontList);

I will need to call `removeEventListener()` anyway because `buildFullFontList()` might be called by `requestIdleCallback()` instead of the event listener.
Comment hidden (mozreview-request)
(Assignee)

Comment 13

2 years ago
(In reply to Nihanth Subramanya [:nhnt11] from comment #0)
> We are waiting a long time for fonts before we first display the preferences
> page. I think it's really unreasonable and see some options:
> 
> - Enumerate fonts off the main thread ("hard" for me to implement personally)

I did some experimentation, and this seems feasible.  Here's a patch that adds EnumerateFontsAsync and EnumerateAllFontsAsync methods to nsIFontEnumerator, which return a promise that resolves to an array of font names that the methods retrieve asynchronously on a helper thread.

There may be thread-safety issues that I'm not aware of.  We'd want to check with someone more familiar with the gfx code (and get review from a gfx peer on the nsThebesFontEnumerator changes).  If it's safe to do, however, is it something you'd be interested in taking?

It does require some changes to the way that FontBuilder builds the font lists, which now happen asynchronously.  And the font tests also have to take that asynchronicity into account.  I've made those changes in this patch as well.

On the plus side, because the font lists get built as soon as the enumeration is complete, the interface should be responsive when you open a list.
Attachment #8891238 - Flags: feedback?(nhnt11)
(Reporter)

Comment 14

2 years ago
Comment on attachment 8891238 [details] [diff] [review]
enumerate fonts asynchronously

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

Thanks for attempting this! I'm not qualified to review the C++ bits, but I did a quick review of the JS part of the patch. Considering this potentially eliminates blocking the main thread without any UX impact in most cases (unless the user is a ninja who wants to open the fonts menu as quickly as they can), I'm all for it.

Disclaimer: it's late and I should probably look at this again in the morning ;)

::: browser/components/preferences/fonts.js
@@ +69,5 @@
>    },
>  
>    readFontLanguageGroup() {
>      var languagePref = document.getElementById("font.language.group");
> +    this._selectLanguageGroupPromise =

You're reassigning this._selectLanguageGroupPromise here, but _selectLanguageGroup wants to await the previous value of this promise, no? Seems like it will end up awaiting itself, so to speak, and will never resolve.

::: browser/components/preferences/in-content-new/main.js
@@ +993,5 @@
>      preferences.hidden = false;
>      // Force flush:
>      preferences.clientHeight;
>      var langGroupPref = document.getElementById("font.language.group");
> +    this._selectDefaultLanguageGroupPromise =

Same issue here.
Attachment #8891238 - Flags: feedback?(nhnt11) → feedback+

Updated

2 years ago
Duplicate of this bug: 1124416
(Reporter)

Comment 16

2 years ago
mozreview-review
Comment on attachment 8889304 [details]
Bug 1375978 - Don't enumerate all fonts when loading about:preferences,

https://reviewboard.mozilla.org/r/160366/#review167992

Thanks for working on this!

::: browser/components/preferences/in-content-new/main.js:1060
(Diff revision 4)
>  
> -      var element = document.getElementById(prefs[i].element);
> +      let element = document.getElementById(prefOption.element);
>        if (element) {
>          element.setAttribute("preference", preference.id);
> +        if (prefOption.fonttype) {
> +          if (element.hasChildNodes()) {

Hmm, I don't understand what this check is for. When does the element have child nodes? I assume there's a good explanation, but please add a comment for future perusers of this code :)

::: browser/components/preferences/in-content-new/main.js:1069
(Diff revision 4)
> +            let buildFullFontList = () => {
> +              element.removeEventListener('popupshowing', buildFullFontList);
> +              FontBuilder.buildFontList(aLanguageGroup, prefOption.fonttype, element);
> +            };
> +            element.addEventListener('popupshowing', buildFullFontList);
> +            window.requestIdleCallback(buildFullFontList);

Do we need to do this at all? It's going to block the main thread at *some* point, likely for long enough that the user will notice some jank that comes seemingly from nowhere.

I don't have any numbers but I feel like changing the font is a rare enough user interaction that we can just block the main thread when we really need to (when the popup is shown).

This will likely be a debatable point, but regardless of that, this code risks calling buildFullFontList twice, if the idle callback is executed after the popupshowing event has been fired.

::: toolkit/mozapps/preferences/fontbuilder.js:44
(Diff revision 4)
> +  /**
> +   * Refresh or build the font menulist with all fonts available for selection.
> +   * This is quite slow, so use initFontList() instead in the hot path.
> +   */
>    buildFontList(aLanguage, aFontType, aMenuList) {
> +    var popup = aMenuList.firstElementChild ||

Please add a check to ensure that the firstElementChild is actually a menupopup element.

::: toolkit/mozapps/preferences/fontbuilder.js:70
(Diff revision 4)
>      // Build the UI for the Default Font and Fonts for this CSS type.
> -    var popup = document.createElement("menupopup");
>      var separator;
>      if (fonts.length > 0) {
>        if (defaultFont) {
> -        var bundlePreferences = document.getElementById("bundlePreferences");
> +        popup.appendChild(this._createMenuItem(defaultFont, true));

Using a DocumentFragment (https://developer.mozilla.org/en/docs/Web/API/DocumentFragment) would be a potential win here, since we're adding a large number of elements.

From the MDN link:
"Because all of the nodes are inserted into the document at once, only one reflow and render is triggered instead of potentially one for each node inserted if they were inserted separately."
Attachment #8889304 - Flags: review?(nhnt11)
(Assignee)

Comment 17

2 years ago
Comment on attachment 8891238 [details] [diff] [review]
enumerate fonts asynchronously

(In reply to Nihanth Subramanya [:nhnt11] from comment #14)

> Thanks for attempting this! I'm not qualified to review the C++ bits, but I
> did a quick review of the JS part of the patch.

Perhaps peterv provide feedback on the C++ bits, before I clean this up for review.

peterv: do you know of anything that would be particularly dangerous about enumerating fonts off the main thread?


> Considering this potentially
> eliminates blocking the main thread without any UX impact in most cases
> (unless the user is a ninja who wants to open the fonts menu as quickly as
> they can), I'm all for it.

The only potential UX impact is that the #defaultFont menulist in the General panel can change its size after the page has loaded, once the enumeration returns and the menulist is populated.

But that doesn't happen in the Fonts subdialog, where the menulists all have |style="width: 0px;"|, and adding that style attribute to the #defaultFont menulist in the General panel fixes the problem there.


> Disclaimer: it's late and I should probably look at this again in the
> morning ;)

Sure thing! :-)


> ::: browser/components/preferences/fonts.js
> @@ +69,5 @@
> >    },
> >  
> >    readFontLanguageGroup() {
> >      var languagePref = document.getElementById("font.language.group");
> > +    this._selectLanguageGroupPromise =
> 
> You're reassigning this._selectLanguageGroupPromise here, but
> _selectLanguageGroup wants to await the previous value of this promise, no?
> Seems like it will end up awaiting itself, so to speak, and will never
> resolve.

Yes, _selectLanguageGroup wants to await the previous value of _selectLanguageGroupPromise.  It's able to do that because _selectLanguageGroupPromise doesn't get reassigned until the async call to _selectLanguageGroup returns its promise, which happens after _selectLanguageGroup awaits _selectLanguageGroupPromise.

So _selectLanguageGroupPromise is always the promise returned by the previous invocation of _selectLanguageGroup when the |await this._selectLanguageGroupPromise| statement is evaluated (except for the first time, when it's the initial value, a resolved promise).
Attachment #8891238 - Flags: feedback?(peterv)
I am going to move this to :myk so we can land his approach. We could revisit main thread only approach if it turned out to be not feasible.
Assignee: timdream → myk
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #18)
> I am going to move this to :myk so we can land his approach. We could
> revisit main thread only approach if it turned out to be not feasible.

PS if we know the approach can be uplift to 56 that would be great.
Here is a profile where macOS took 2.5sec to give us the font list back.

https://perfht.ml/2v3tkfT

It happens when I try to launch about:preferences right away when Firefox launches, e.g. |mach run about:preferences|.
(Assignee)

Comment 21

2 years ago
Here's a cleaned-up patch to add asynchronous font enumeration to about:preferences via EnumerateFontsAsync and EnumerateAllFontsAsync methods on nsIFontEnumerator.

I've reviewed gfxPlatformFontList::GetFontList, and I don't see any obvious thread-safety issues with running it off the main thread, but a review from a Graphics peer would be helpful.

Jonathan: you're a peer of the Graphics module, and you reviewed a recent change to nsThebesFontEnumerator.cpp.  Perhaps you can review this one too?
Attachment #8891238 - Attachment is obsolete: true
Attachment #8891238 - Flags: feedback?(peterv)
Attachment #8897523 - Flags: review?(jfkthame)
gfxPlatformFontList::GetFontList iterates over the mFontFamilies hashtable, which I don't believe is thread-safe, is it? While we'd probably get away with this most of the time -- the hashtable generally gets populated once when we initialize the font list, and then doesn't change -- there's a non-zero possibility of it changing (being cleared and rebuilt) during the session. If that happened while an off-main-thread GetFontList call is busy iterating, I suspect we'd have a very unhappy browser....
(Assignee)

Comment 23

2 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #22)
> gfxPlatformFontList::GetFontList iterates over the mFontFamilies hashtable,
> which I don't believe is thread-safe, is it? While we'd probably get away
> with this most of the time -- the hashtable generally gets populated once
> when we initialize the font list, and then doesn't change -- there's a
> non-zero possibility of it changing (being cleared and rebuilt) during the
> session. If that happened while an off-main-thread GetFontList call is busy
> iterating, I suspect we'd have a very unhappy browser....

Ah, indeed.  It shouldn't be possible to get into that unhappy state.  In this version of the patch, I've added a mutex to protect mFontFamilies.  Now I'm troubleshooting an issue identified by the asan builds on tryserver, after which I'll re-request review.
Attachment #8897523 - Attachment is obsolete: true
Attachment #8897523 - Flags: review?(jfkthame)
(Assignee)

Comment 24

2 years ago
This version fixes the ASAN issues, but there's still one issue remaining on macOS debug builds (like https://treeherder.mozilla.org/logviewer.html#?job_id=126188547&repo=try&lineNumber=24449): Assertion failure: sInServoTraversal || NS_IsMainThread(), at /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ServoStyleSet.h:91

It looks like font enumeration on macOS eventually calls into gfxMacFontFamily::FindStyleVariations, which calls GetWeightOverride, which calls Preferences::GetInt, which calls Preferences::InitStaticMembers, which asserts |NS_IsMainThread() || mozilla::ServoStyleSet::IsInServoTraversal()|, which calls ServoStyleSet::IsInServoTraversal(), which makes the |sInServoTraversal || NS_IsMainThread()| that causes the failure.

I'm looking into what to do about that.
Attachment #8899511 - Attachment is obsolete: true
(Assignee)

Comment 25

2 years ago
(In reply to Myk Melez [:myk] [@mykmelez] from comment #24)
> It looks like font enumeration on macOS eventually calls into
> gfxMacFontFamily::FindStyleVariations, which calls GetWeightOverride, which
> calls Preferences::GetInt, which calls Preferences::InitStaticMembers, which
> asserts |NS_IsMainThread() || mozilla::ServoStyleSet::IsInServoTraversal()|,
> which calls ServoStyleSet::IsInServoTraversal(), which makes the
> |sInServoTraversal || NS_IsMainThread()| that causes the failure.

I read through bug 931426 (which introduced the calls to Preferences::GetInt in GetWeightOverride) and the functions on the stack leading up to that call.  But it still isn't clear to me how to resolve this assertion on Mac.

gfxPlatformFontList::GetFontList doesn't care about font weights, so gfxMacFontFamily::FindStyleVariations wouldn't need to call GetWeightOverride when called on the font enumeration thread.

However, FindStyleVariations caches the gfxFontEntry instances it creates, whose mWeight values might then be incorrect for future callers who do want that information.

We could compute and cache MacOSFontEntry::mWeight lazily, calling GetWeightOverride the first time it's needed.  But that would require significant refactoring of gfxFontEntry::mWeight, which affects other platforms and consumers.

Alternately, we could hardcode the font weight overrides, if they weren't actually intended to be configurable (and thus don't need to be stored as preferences).  But that would presumably break some indeterminate number of users who have configured them anyway.

jfkthame: do you have another suggestion for how to resolve this assertion?
Flags: needinfo?(jfkthame)
Ugh, this is all very sad... so many weird complications for various platform-specific cases in font code.

I don't think computing mWeight lazily is good, because that means we'd have to use a (virtual) accessor for mWeight everywhere; and font matching/selection is a surprisingly hot code path. I would be prepared to consider hard-coding the overrides, though there is some risk that there could be users who have added/customized such prefs.

Stepping back, though, I wonder if we can take a higher-level approach here. The list that about:preferences wants to display is not a list of font faces but of families, which means that in principle it shouldn't require the family entries (gfxFontFamily or platform-specific subclasses) to have called FindStyleVariations and populated their list of faces at all; it only needs the family names.

AFAICS, the reason it ends up calling FindStyleVariations for each family (if that hasn't already been done -- which in practice means if the browser is recently launched, as we populate all these shortly after startup) is that it wants to query a few properties of a gfxFontEntry (an individual font face) to decide whether to include this family in the list:

        fontEntry->IsSymbolFont()
        fontEntry->SupportsLangGroup(aLangGroup)
        fontEntry->MatchesGenericFamily(aGenericFamily)

These are assumed to be consistent across all faces in a family. Moreover, we don't actually implement them meaningfully in all cases: e.g. MatchesGenericFamily returns a hardcoded 'true' on all platforms except Windows/GDI, and SupportsLangGroup does the same on all except Windows/GDI and Fontconfig. And IsSymbolFont returns the mSymbolFont flag, which AFAICT is only set by the Windows and Mac code.

I wonder if we can move these three methods and make them virtual methods on gfxFontFamily (with trivial default implementations), and then provide overrides only for the cases we actually care about. Specifically, in gfxMacFontFamily we wouldn't need either SupportsLangGroup or MatchesGenericFamily to retain existing behavior; and I think we could implement IsSymbolFont without reading all the faces and processing the weight overrides.

I'm on PTO for a couple of days but would be willing to take a stab at this at the end of the week if you don't want to tackle it before then. (I'd suggest spinning off a separate bug for this refactoring of font attributes, which would make sense to do in its own right.)
Flags: needinfo?(jfkthame)
(Assignee)

Comment 27

2 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #26)
> I wonder if we can move these three methods and make them virtual methods on
> gfxFontFamily (with trivial default implementations), and then provide
> overrides only for the cases we actually care about. Specifically, in
> gfxMacFontFamily we wouldn't need either SupportsLangGroup or
> MatchesGenericFamily to retain existing behavior; and I think we could
> implement IsSymbolFont without reading all the faces and processing the
> weight overrides.

That makes sense and seems like a better solution than the two that I suggested.


> I'm on PTO for a couple of days but would be willing to take a stab at this
> at the end of the week if you don't want to tackle it before then. (I'd
> suggest spinning off a separate bug for this refactoring of font attributes,
> which would make sense to do in its own right.)

In my eagerness to make progress on this bug, I tackled this today and got through SupportsLangGroup and MatchesGenericFamily.  IsSymbolFont was harder, though, and I didn't manage to figure it out.  Perhaps you can pick up that one?  I filed bug 1395061 on refactoring these methods and attached my patch there.
Depends on: 1395061
(Assignee)

Comment 28

2 years ago
Here's an updated patch that applies cleanly to the latest mozilla-central.

jfkthame: with the fixes from bug 1395061, this no longer crashes on Mac debug builds, so it should be ready to review.

Here's a tryserver run that includes the first two patches in bug 1395061:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8985aad2b68377a8df77200734918cb28868ca94
Attachment #8901707 - Attachment is obsolete: true
Attachment #8904671 - Flags: review?(jfkthame)
(Assignee)

Comment 29

2 years ago
The tryserver run in comment #28 failed on Windows due to merge bustage (missed the gfxFontEntry::Clone method declaration when merging the fix for bug #835204).  Here's a tryserver run for just Windows with the bustage fixed:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=33d7510e04f77c2b56b4a60217d6696e8bebdb62
(Assignee)

Comment 30

2 years ago
Posted patch promise-enumerate-fonts.diff (obsolete) — Splinter Review
Ah, we can still hit that assertion on Mac via:

#01: mozilla::Preferences::GetInt(char const*, int*) [modules/libpref/Preferences.cpp:1629]
#02: gfxMacFontFamily::FindStyleVariations(FontInfoData*) [modules/libpref/Preferences.h:130]
#03: gfxFontFamily::ReadOtherFamilyNames(gfxPlatformFontList*) [gfx/thebes/gfxFontEntry.cpp:1620]
#04: gfxFontFamily::HasOtherFamilyNames() [gfx/thebes/gfxFontEntry.cpp:1092]
#05: gfxMacFontFamily::LocalizedName(nsAString&) [gfx/thebes/gfxMacPlatformFontList.mm:511]
#06: gfxPlatformFontList::GetFontList(nsIAtom*, nsACString const&, nsTArray<nsString>&) [gfx/thebes/gfxPlatformFontList.cpp:493]

Here's an example:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8985aad2b68377a8df77200734918cb28868ca94&selectedJob=128630308

jfkthame: In gfxMacFontFamily::LocalizedName, the call to HasOtherFamilyNames looks like an optimization.  Regardless of whether we make it or not, LocalizedName will either return the localized name returned by NSFontManager::localizedNameForFamily:face: or the canonical name mName.

So this version of the patch makes gfxMacFontFamily::LocalizedName call HasOtherFamilyNames() only if NS_IsMainThread().  Here's a tryserver run with that change:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=936d6b2b6d98c538d9d02b265ed080d75f34ac67
Attachment #8904671 - Attachment is obsolete: true
Attachment #8904671 - Flags: review?(jfkthame)
Attachment #8904932 - Flags: review?(jfkthame)
Comment on attachment 8904932 [details] [diff] [review]
promise-enumerate-fonts.diff

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

I'm going to mark this feedback+ from the C++ font-code point of view; seems OK, modulo some minor style nits. On the front-end/JS promises etc side of things, I don't really feel competent to review -- that's all a foreign country to me. So I'd be more comfortable if you also get a review from someone who knows that aspect of things.

::: gfx/src/nsThebesFontEnumerator.cpp
@@ +78,5 @@
>  }
>  
> +struct EnumerateFontsPromise final
> +{
> +    explicit EnumerateFontsPromise(mozilla::dom::Promise *aPromise)

Attach '*' to the type rather than the parameter.

@@ +110,5 @@
> +
> +        if (NS_FAILED(mRv))
> +            mEnumerateFontsPromise->mPromise->MaybeReject(mRv);
> +        else
> +            mEnumerateFontsPromise->mPromise->MaybeResolve(mFontList);

Please add braces as per gecko style guide.

@@ +127,5 @@
> +
> +class EnumerateFontsTask final : public Runnable
> +{
> +public:
> +    EnumerateFontsTask(nsIAtom *aLangGroupAtom,

Attach '*' to the type rather than the parameter.

@@ +128,5 @@
> +class EnumerateFontsTask final : public Runnable
> +{
> +public:
> +    EnumerateFontsTask(nsIAtom *aLangGroupAtom,
> +                       nsAutoCString aGeneric,

Pass the string by const reference, e.g. `const nsACString& aGeneric`.

@@ +155,5 @@
> +    }
> +
> +private:
> +    nsCOMPtr<nsIAtom> mLangGroupAtom;
> +    nsAutoCString mGeneric;

Now that we have templated-length autostring types (bug 1386103), we can save space here by using `nsAutoCStringN<16>` (which should be more than sufficient for any of the CSS generics).

@@ +168,5 @@
> +}
> +
> +NS_IMETHODIMP
> +nsThebesFontEnumerator::EnumerateFontsAsync(const char *aLangGroup,
> +                                            const char *aGeneric,

Attach '*' to the type rather than the parameter.

@@ +189,5 @@
> +
> +    nsCOMPtr<nsIThread> thread;
> +    nsresult rv = NS_NewNamedThread("FontEnumThread", getter_AddRefs(thread));
> +    if (NS_FAILED(rv)) {
> +        return NS_ERROR_FAILURE;

Why not return the actual failure `rv` here?

@@ +194,5 @@
> +    }
> +
> +    nsCOMPtr<nsIAtom> langGroupAtom;
> +    if (aLangGroup) {
> +        nsAutoCString lowered;

This could also be `nsAutoCStringN<16>`, to keep the stack frame smaller.

@@ +204,5 @@
> +    nsAutoCString generic;
> +    if (aGeneric)
> +        generic.Assign(aGeneric);
> +    else
> +        generic.SetIsVoid(true);

braces

(Is the 'else' clause needed here at all? 'generic' will be an empty string anyway if we just don't assign to it, and that's all we care about.)

Hmm. So actually, a lot of this is derived from the existing nsThebesFontEnumerator::EnumerateFonts method (which is the origin of a bunch of the style nits). I wish we could do this with less duplication somehow...

::: gfx/thebes/gfxMacPlatformFontList.mm
@@ +503,5 @@
>  gfxMacFontFamily::LocalizedName(nsAString& aLocalizedName)
>  {
>      nsAutoreleasePool localPool;
>  
> +    if (NS_IsMainThread() && !HasOtherFamilyNames()) {

Please add a comment noting why it is unsafe to call HasOtherFamilyNames off-main-thread, to remind us when reading this code in a few years' time...

::: gfx/thebes/gfxPlatformFontList.h
@@ +517,5 @@
>      virtual gfxFontFamily*
>      GetDefaultFontForPlatform(const gfxFontStyle* aStyle) = 0;
>  
> +    // Protects mFontFamilies.
> +    mozilla::Mutex mMutex;

I'd prefer a less generic name than 'mMutex' here, to indicate its specific purpose -- e.g. mFontFamiliesMutex.
Attachment #8904932 - Flags: review?(jfkthame) → feedback+
(Assignee)

Comment 32

2 years ago
(In reply to Jonathan Kew (:jfkthame) from comment #31)

> I'm going to mark this feedback+ from the C++ font-code point of view; seems
> OK, modulo some minor style nits. On the front-end/JS promises etc side of
> things, I don't really feel competent to review -- that's all a foreign
> country to me. So I'd be more comfortable if you also get a review from
> someone who knows that aspect of things.

nhnt11 has previously given me feedback on the JavaScript parts of this patch in comment #14, so I've requested review from him on this patch.

He said he isn't qualified to review the C++ code, however, so perhaps you can grant review on that part?  (That might be the implication of your feedback+, but I don't want to presume!)

nhnt11: note my responses to your earlier feedback in comment #17.  This patch includes the fix for the #defaultFont menulist changing its size once the menu is populated.


> ::: gfx/src/nsThebesFontEnumerator.cpp
> @@ +78,5 @@
> >  }
> >  
> > +struct EnumerateFontsPromise final
> > +{
> > +    explicit EnumerateFontsPromise(mozilla::dom::Promise *aPromise)
> 
> Attach '*' to the type rather than the parameter.

Done for this and the other similar nits.


> @@ +110,5 @@
> > +
> > +        if (NS_FAILED(mRv))
> > +            mEnumerateFontsPromise->mPromise->MaybeReject(mRv);
> > +        else
> > +            mEnumerateFontsPromise->mPromise->MaybeResolve(mFontList);
> 
> Please add braces as per gecko style guide.

Done for this and the other similar nit.


> @@ +128,5 @@
> > +class EnumerateFontsTask final : public Runnable
> > +{
> > +public:
> > +    EnumerateFontsTask(nsIAtom *aLangGroupAtom,
> > +                       nsAutoCString aGeneric,
> 
> Pass the string by const reference, e.g. `const nsACString& aGeneric`.

Done.


> @@ +155,5 @@
> > +    }
> > +
> > +private:
> > +    nsCOMPtr<nsIAtom> mLangGroupAtom;
> > +    nsAutoCString mGeneric;
> 
> Now that we have templated-length autostring types (bug 1386103), we can
> save space here by using `nsAutoCStringN<16>` (which should be more than
> sufficient for any of the CSS generics).

Got it, I've made this change.


> @@ +189,5 @@
> > +
> > +    nsCOMPtr<nsIThread> thread;
> > +    nsresult rv = NS_NewNamedThread("FontEnumThread", getter_AddRefs(thread));
> > +    if (NS_FAILED(rv)) {
> > +        return NS_ERROR_FAILURE;
> 
> Why not return the actual failure `rv` here?

Indeed, that's a better approach, so I've replaced this conditional return with:

    NS_ENSURE_SUCCESS(rv, rv);


> @@ +194,5 @@
> > +    }
> > +
> > +    nsCOMPtr<nsIAtom> langGroupAtom;
> > +    if (aLangGroup) {
> > +        nsAutoCString lowered;
> 
> This could also be `nsAutoCStringN<16>`, to keep the stack frame smaller.

Indeed, I've made this change.


> (Is the 'else' clause needed here at all? 'generic' will be an empty string
> anyway if we just don't assign to it, and that's all we care about.)

The semantics of SetIsVoid aren't entirely clear to me, so I'm unsure if this else clause is really needed for our use case.  I've left it as-is for now, but I can research it further if you think it's significant.


> Hmm. So actually, a lot of this is derived from the existing
> nsThebesFontEnumerator::EnumerateFonts method (which is the origin of a
> bunch of the style nits). I wish we could do this with less duplication
> somehow...

Right.  I've considered how to refactor the code to reduce this duplication, but I haven't yet identified a satisfactory approach.  I'll continue thinking about it, and in any case I can also fix the style nits in the original code as well, if you'd prefer.


> ::: gfx/thebes/gfxMacPlatformFontList.mm
> @@ +503,5 @@
> >  gfxMacFontFamily::LocalizedName(nsAString& aLocalizedName)
> >  {
> >      nsAutoreleasePool localPool;
> >  
> > +    if (NS_IsMainThread() && !HasOtherFamilyNames()) {
> 
> Please add a comment noting why it is unsafe to call HasOtherFamilyNames
> off-main-thread, to remind us when reading this code in a few years' time...

Ok, I've added this comment:

    // It's unsafe to call HasOtherFamilyNames off the main thread because
    // it entrains FindStyleVariations, which calls GetWeightOverride, which
    // retrieves prefs.  And the pref names can change (via user overrides),
    // so we can't use gfxPrefs to access them.


> ::: gfx/thebes/gfxPlatformFontList.h
> @@ +517,5 @@
> >      virtual gfxFontFamily*
> >      GetDefaultFontForPlatform(const gfxFontStyle* aStyle) = 0;
> >  
> > +    // Protects mFontFamilies.
> > +    mozilla::Mutex mMutex;
> 
> I'd prefer a less generic name than 'mMutex' here, to indicate its specific
> purpose -- e.g. mFontFamiliesMutex.

Roger that, I've renamed it to mFontFamiliesMutex.


https://treeherder.mozilla.org/#/jobs?repo=try&revision=72406a4bea346b18228754bc55e75e57fd54d81f
Attachment #8904932 - Attachment is obsolete: true
Attachment #8905285 - Flags: review?(nhnt11)
Attachment #8905285 - Flags: review?(jfkthame)
Comment on attachment 8905285 [details] [diff] [review]
enumerate fonts asynchronously

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

Thanks for the tidying-up; r=me on the C++ side of this.
Attachment #8905285 - Flags: review?(jfkthame) → review+
(Reporter)

Comment 34

2 years ago
Comment on attachment 8905285 [details] [diff] [review]
enumerate fonts asynchronously

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

Thanks for the patch! I have a couple of comments, but I won't let them block r+.

::: browser/components/preferences/fonts.js
@@ +69,5 @@
>    },
>  
>    readFontLanguageGroup() {
>      var languagePref = document.getElementById("font.language.group");
> +    this._selectLanguageGroupPromise =

Can we make this "safety promise" self-contained? What I mean is, consumers of _selectLanguageGroup shouldn't have to worry about setting this promise. It would be more ideal to:

a) Assign the promise within _selectLanguageGroup. The call to buildFontList is what we actually want to wait on, so we can do something like:

_selectLanguageGroup(aLanguageGroup) {
  // Comment about avoiding overlapping
  await this._selectLanguageGroupPromise;
  .
  .
  .
      if (prefs[i].fonttype) {
        this._selectLanguageGroupPromise =
          FontBuilder.buildFontList(...)
          .catch(Components.utils.reportError);
        await this._selectLanguageGroupPromise;
      }
      preference.setElementValue(element);
    }
  }
},

and even call the promise something like _buildFontListPromise.

or b) Add a helper function and call that from readFontLanguageGroup, i.e.:

_safelySelectLanguageGroup(aLanguageGroup) {
  this._selectLanguageGroupPromise =
    this._selectLanguageGroup(aLanguageGroup)
    .catch(Components.utils.reportError);
}



What do you think? Admittedly, there's currently only one caller of _selectLanguageGroup in the file so I don't feel very strongly about this and won't let it block r+.

::: browser/components/preferences/in-content/main.js
@@ +996,5 @@
>      preferences.hidden = false;
>      // Force flush:
>      preferences.clientHeight;
>      var langGroupPref = document.getElementById("font.language.group");
> +    this._selectDefaultLanguageGroupPromise =

Please see what I said above about making this safety promise self-contained.

::: browser/components/preferences/in-content/main.xul
@@ +456,5 @@
>        <hbox align="center" flex="1">
>          <label control="defaultFont" accesskey="&defaultFont2.accesskey;">&defaultFont2.label;</label>
>          <!-- Please don't remove the wrapping hbox/vbox/box for these elements. It's used to properly compute the search tooltip position. -->
>          <hbox flex="1">
> +          <menulist id="defaultFont" flex="1" style="width: 0px;" delayprefsave="true" onsyncfrompreference="return FontBuilder.readFontSelection(this);"/>

Please put this CSS rule in browser/themes/shared/incontentprefs/preferences.inc.css

::: browser/components/preferences/in-content/tests/browser_basic_rebuild_fonts_test.js
@@ +51,5 @@
> +  // Await rebuilding of the font lists, which happens asynchronously in
> +  // gFontsDialog._selectLanguageGroup.  Testing code needs to call this
> +  // function and await its resolution after changing langGroupElement's value
> +  // (or doing anything else that triggers a call to _selectLanguageGroup).
> +  async function rebuildFontLists() {

Since we have to await on calls to this function anyway, we can just make it a normal (not async) function that returns win.gFontsDialog._selectLanguageGroupPromise. Also, this function doesn't actively do anything, so let's call it something like fontListsRebuilt or promiseFontListsRebuilt.

I want to say that we can get rid of this function altogether and just be verbose in all the usages below (await win.gFontsDialog._selectLanguageGroupPromise), but I admit that the convenience function makes the comment much more discoverable to readers of this code. I'll leave it up to you!
Attachment #8905285 - Flags: review?(nhnt11) → review+
(Reporter)

Comment 35

2 years ago
(In reply to Nihanth Subramanya [:nhnt11] from comment #34)
> Comment on attachment 8905285 [details] [diff] [review]
> enumerate fonts asynchronously
> 
> Review of attachment 8905285 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> a) Assign the promise within _selectLanguageGroup. The call to buildFontList
> is what we actually want to wait on, so we can do something like:
> 
> _selectLanguageGroup(aLanguageGroup) {
>   // Comment about avoiding overlapping
>   await this._selectLanguageGroupPromise;
>   .
>   .
>   .
>       if (prefs[i].fonttype) {
>         this._selectLanguageGroupPromise =
>           FontBuilder.buildFontList(...)
>           .catch(Components.utils.reportError);
>         await this._selectLanguageGroupPromise;
>       }
>       preference.setElementValue(element);
>     }
>   }
> },

I meant async _selectLanguageGroup(aLanguageGroup) here, of course.
(Assignee)

Comment 36

2 years ago
(In reply to Nihanth Subramanya [:nhnt11] from comment #34)

> Can we make this "safety promise" self-contained? What I mean is, consumers
> of _selectLanguageGroup shouldn't have to worry about setting this promise.
> It would be more ideal to:
> 
> a) Assign the promise within _selectLanguageGroup. The call to buildFontList
> is what we actually want to wait on, so we can do something like:
> 
> _selectLanguageGroup(aLanguageGroup) {
>   // Comment about avoiding overlapping
>   await this._selectLanguageGroupPromise;
>   .
>   .
>   .
>       if (prefs[i].fonttype) {
>         this._selectLanguageGroupPromise =
>           FontBuilder.buildFontList(...)
>           .catch(Components.utils.reportError);
>         await this._selectLanguageGroupPromise;
>       }
>       preference.setElementValue(element);
>     }
>   }
> },

This won't work, because _selectLanguageGroup calls FontBuilder.buildFontList multiple times (once per pref with an associated "element"), and we need to block re-entrance until all those calls complete (i.e. until _selectLanguageGroup returns) to avoid interleaving _selectLanguageGroup calls.


> or b) Add a helper function and call that from readFontLanguageGroup, i.e.:
> 
> _safelySelectLanguageGroup(aLanguageGroup) {
>   this._selectLanguageGroupPromise =
>     this._selectLanguageGroup(aLanguageGroup)
>     .catch(Components.utils.reportError);
> }

This will work, and I've done it in this updated patch.  I also indented the catch call another two spaces for consistency with another such call (in main.js).


> What do you think? Admittedly, there's currently only one caller of
> _selectLanguageGroup in the file so I don't feel very strongly about this
> and won't let it block r+.

My original thinking was that _selectLanguageGroup is an internal method of gFontsDialog, so any additional callers should be aware of its semantics.  Nevertheless, that'll be more likely now that we've refactored the call into _safelySelectLanguageGroup, which makes the semantics clearer.


> ::: browser/components/preferences/in-content/main.js
> @@ +996,5 @@
> >      preferences.hidden = false;
> >      // Force flush:
> >      preferences.clientHeight;
> >      var langGroupPref = document.getElementById("font.language.group");
> > +    this._selectDefaultLanguageGroupPromise =
> 
> Please see what I said above about making this safety promise self-contained.

This call has the same limitation as the other one, so I've refactored it into a new _safelySelectDefaultLanguageGroup method.  I also factored out the expression that determines if the default font type is "serif" so that the call to _selectDefaultLanguageGroup would be shorter and more readable.


> ::: browser/components/preferences/in-content/main.xul
> @@ +456,5 @@
> >        <hbox align="center" flex="1">
> >          <label control="defaultFont" accesskey="&defaultFont2.accesskey;">&defaultFont2.label;</label>
> >          <!-- Please don't remove the wrapping hbox/vbox/box for these elements. It's used to properly compute the search tooltip position. -->
> >          <hbox flex="1">
> > +          <menulist id="defaultFont" flex="1" style="width: 0px;" delayprefsave="true" onsyncfrompreference="return FontBuilder.readFontSelection(this);"/>
> 
> Please put this CSS rule in
> browser/themes/shared/incontentprefs/preferences.inc.css

Good idea, done.  I also made it |width: 0;|, since the unit is irrelevant when the value is 0.


> :::
> browser/components/preferences/in-content/tests/
> browser_basic_rebuild_fonts_test.js
> @@ +51,5 @@
> > +  // Await rebuilding of the font lists, which happens asynchronously in
> > +  // gFontsDialog._selectLanguageGroup.  Testing code needs to call this
> > +  // function and await its resolution after changing langGroupElement's value
> > +  // (or doing anything else that triggers a call to _selectLanguageGroup).
> > +  async function rebuildFontLists() {
> 
> Since we have to await on calls to this function anyway, we can just make it
> a normal (not async) function that returns
> win.gFontsDialog._selectLanguageGroupPromise. Also, this function doesn't
> actively do anything, so let's call it something like fontListsRebuilt or
> promiseFontListsRebuilt.

Good points, I've renamed it to fontListsRebuilt and made it a non-async function that simply returns win.gFontsDialog._selectLanguageGroupPromise.


> I want to say that we can get rid of this function altogether and just be
> verbose in all the usages below (await
> win.gFontsDialog._selectLanguageGroupPromise), but I admit that the
> convenience function makes the comment much more discoverable to readers of
> this code. I'll leave it up to you!

The function doesn't reduce code size, but it does make behavior clearer at the numerous call sites, and it reduces the number of places where the test accesses an internal method of the gFontsDialog object, which seems more robust; so I've left it in place.

Now that blocker bug 1395061 has been resolved, I'll land this patch after one last tryserver run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4329bbea5004a2d09f3653029f313df65e0690a5
(Reporter)

Comment 37

2 years ago
Thanks! I didn't see the for loop (In reply to Myk Melez [:myk] [@mykmelez] from comment #36)
> Created attachment 8906840 [details] [diff] [review]
> enumerate fonts asynchronously
> 
> (In reply to Nihanth Subramanya [:nhnt11] from comment #34)
> 
> > Can we make this "safety promise" self-contained? What I mean is, consumers
> > of _selectLanguageGroup shouldn't have to worry about setting this promise.
> > It would be more ideal to:
> > 
> > a) Assign the promise within _selectLanguageGroup. The call to buildFontList
> > is what we actually want to wait on, so we can do something like:
> > 
> > _selectLanguageGroup(aLanguageGroup) {
> >   // Comment about avoiding overlapping
> >   await this._selectLanguageGroupPromise;
> >   .
> >   .
> >   .
> >       if (prefs[i].fonttype) {
> >         this._selectLanguageGroupPromise =
> >           FontBuilder.buildFontList(...)
> >           .catch(Components.utils.reportError);
> >         await this._selectLanguageGroupPromise;
> >       }
> >       preference.setElementValue(element);
> >     }
> >   }
> > },
> 
> This won't work, because _selectLanguageGroup calls
> FontBuilder.buildFontList multiple times (once per pref with an associated
> "element"), and we need to block re-entrance until all those calls complete
> (i.e. until _selectLanguageGroup returns) to avoid interleaving
> _selectLanguageGroup calls.

Ah, oops! I only looked at the code in Splinter review and missed the for loop.

> > or b) Add a helper function and call that from readFontLanguageGroup, i.e.:
> > 
> > _safelySelectLanguageGroup(aLanguageGroup) {
> >   this._selectLanguageGroupPromise =
> >     this._selectLanguageGroup(aLanguageGroup)
> >     .catch(Components.utils.reportError);
> > }
> 
> This will work, and I've done it in this updated patch.  I also indented the
> catch call another two spaces for consistency with another such call (in
> main.js).

Cool, WFM, thanks! FWIW, the extra function can be compressed into the original one if you do something like this:

async _selectLanguageGroup(aLanguageGroup, _aWaitForPreviousCall=true) {
  // Comment about avoiding overlapping
  if (_aWaitForPreviousCall) {
    await this._selectLanguageGroupPromise;
    this._selectLanguageGroupPromise =
      this._selectLanguageGroup(aLanguageGroup, false)
        .catch(Components.utils.reportError);
    return;
  }
  .
  .
  .
}

At this point I'm not sure if this is actually better (readability!) but the self-contained safety promise problem interests me. :)
(Reporter)

Comment 38

2 years ago
(In reply to Nihanth Subramanya [:nhnt11] from comment #37)
> async _selectLanguageGroup(aLanguageGroup, _aWaitForPreviousCall=true) {
>   // Comment about avoiding overlapping
>   if (_aWaitForPreviousCall) {
>     await this._selectLanguageGroupPromise;
>     this._selectLanguageGroupPromise =
>       this._selectLanguageGroup(aLanguageGroup, false)
>         .catch(Components.utils.reportError);
>     return;
>   }
>   .
>   .
>   .
> }

This might work in practice but is theoretically incorrect, because successive calls to _selectLanguageGroup will not queue properly. The solution is to use then() instead of awaiting the promise. Here's some code you can run in scratchpad that demonstrates this:


var p = Promise.resolve();;
var count = 0;

async function foo(arg, _wait=true) {
  if (_wait) {
    console.log("queuing " + arg);
    p = p.then(() => foo(arg, false));
    return;
  }
  console.log("executing " + arg);
  await new Promise((resolve, reject) => {
    setTimeout(() => {
      count++;
      setTimeout(() => {
        console.log("count: " + count);
        console.log("resolving " + arg);
        resolve();
      }, 500);
    }, 500);
  });
}

foo("a");
foo("b");
foo("c");
----------------------------------------
Output:

queuing a
queuing b
queuing c
executing a
count: 1
resolving a
executing b
count: 2
resolving b
executing c
count: 3
resolving c


Notice that all three calls to foo() get queued immediately, but they execute one after another. If you use await instead of then (below), it does not work, because the next call to foo() happens before we reassign p:

  if (_wait) {
    console.log("queuing " + arg);
    await p;
    p = foo(arg, false);
    return;
  }

Output:

queuing a
queuing b
queuing c
executing a
executing b
executing c
count: 3
resolving a
count: 3
resolving b
count: 3
resolving c


--------------------------------------------------------------------------------

Basically, the code I suggested is bad; this is good:

async _selectLanguageGroup(aLanguageGroup, _aWaitForPreviousCall=true) {
  // Comment about avoiding overlapping
  if (_aWaitForPreviousCall) {
    this._selectLanguageGroupPromise =
      this._selectLanguageGroupPromise.then(() =>
        this._selectLanguageGroup(aLanguageGroup, false)
          .catch(Components.utils.reportError)
      );
    return;
  }
  .
  .
  .
}

Unfortunately, the indentation gets a bit intense :(
Is this can address performance regression for bug 1398597?
Flags: needinfo?(nhnt11)
(Reporter)

Comment 40

2 years ago
(In reply to Ricky Chien [:rickychien] from comment #39)
> Is this can address performance regression for bug 1398597?

Yes, this should significantly improve the situation.
Flags: needinfo?(nhnt11)
(Assignee)

Updated

2 years ago
Blocks: 1399202
(Assignee)

Comment 41

2 years ago
(In reply to Nihanth Subramanya [:nhnt11] from comment #38)

> async _selectLanguageGroup(aLanguageGroup, _aWaitForPreviousCall=true) {
>   // Comment about avoiding overlapping
>   if (_aWaitForPreviousCall) {
>     this._selectLanguageGroupPromise =
>       this._selectLanguageGroupPromise.then(() =>
>         this._selectLanguageGroup(aLanguageGroup, false)
>           .catch(Components.utils.reportError)
>       );
>     return;
>   }
>   .
>   .
>   .
> }
> 
> Unfortunately, the indentation gets a bit intense :(

Indeed.  It's still a worthy goal to better encapsulate the language group selection methods, however.  I've filed bug 1399202 to do that.

Comment 42

2 years ago
Pushed by myk@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f255ec4e8c36
enumerate fonts asynchronously; r=jfkthame,nhnt11

Updated

2 years ago
Blocks: 1399206
https://hg.mozilla.org/mozilla-central/rev/f255ec4e8c36
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57

Updated

2 years ago
Blocks: 1399410
No longer blocks: 1398597

Updated

2 years ago
Depends on: 1399963

Updated

2 years ago
Depends on: 1404665
No longer depends on: 1404665

Updated

2 years ago
Depends on: 1409662
Depends on: 1426357
Depends on: 1463884
You need to log in before you can comment on or make changes to this bug.