[moved from bug 752394 so that this can land separately] I'm breaking out the async font loader implementation into a separate bug so that I can land it first before landing other patches that fix related font chrome hang bugs. The implementation here simply duplicates the existing functionality of the current font loader code which runs on the main thread in intervals until it completes. Current font loader sequence: * platform font list initialization (main thread) ==> StartLoader - starts a timer to delay font loading until after startup ==> InitLoader - initialize list of gfxFontFamily objects * timer fires after delay (main thread) * timer fires on an interval (main thread) ==> RunLoader - iterates over a subset of gfxFontFamily objects, initializing whatever is needed on a given platform (cmaps, facenames, localized family names) * RunLoader returns true (main thread) ==> FinishLoader - clean up Async font loader sequence: * platform font list initialization (main thread) ==> StartLoader - starts a timer to delay font loading until after startup ==> InitLoader - initialize list of font family name strings * timer fires after delay (main thread) ==> StartLoader - creates a platform-specific FontInfoData object, then creates a new thread and dispatches an AsyncFontInfoLoader runnable to the new thread * AsyncFontInfoLoader::Run (off-main-thread) ==> FontInfoData::Load - iterates over font families ==> LoadFontFamilyData - platform-specific routine to which initializes whatever is needed on a given platform (cmaps, facenames, localized family names) dispatches FontInfoLoadCompleteEvent runnable to main thread * FontInfoLoadCompleteEvent::Run (main thread) ==> FinalizeLoader - iterates over gfxFontFamily objects, transfering data loaded on async thread. calls LoadFontInfo and starts interval timer if can't complete within set timeframe * timer fires on an interval (main thread) ==> LoadFontInfo - additional calls to LoadFontInfo * LoadFontInfo returns true (main thread) ==> CleanupLoader - clean up After the loader completes, the FontInfoData object is dumped, leaving only the data that was explicitly transfered to the platform font list and gfxFontFamily objects. Loader infrastructure base class and async runnables are in gfxFontInfoLoader.[h/cpp]. gfxPlatformFontList::LoadFontInfo contains the code that transfers the loaded font data to the platform font list. Platform-specific loading code is contained in LoadFontFamilyData methods for each platform font list (OSX, DirectWrite, GDI). The gfxFontFamily methods that initialize cmaps or name data are now passed a FontInfoData object containing font data loaded on an async thread. The LoadFontFamilyData methods are run on the async thread and so must use only static methods or thread-safe platform API's, they explicitly don't touch any object accessed by text code running on the main thread, nor do they access any harfbuzz method. The interval timer to transfer font data back to the platform font list generally isn't needed; since the transfer is basically just swizzling around data structures, the transfer will usually occur within the initial allotted time slice. For systems with huge numbers of fonts (i.e. >3000 fonts) or in cases where simply enumerating fonts takes a long time (due to overzealous virus-checker code for example) it may take longer and the interval timer will fire multiple times. Note: loader code doesn't run on Android or Linux.

Switch to using CTFontManager instead of AppKit since it's not clear AppKit methods are thread-safe (Apple docs don't indicate either way).

Move existing methods used in reading font tables out to static methods which can be used off-main-thread.

Move the gfxFontInfoLoader class out from gfxFontUtils.[h/cpp] into a separate file before making changes to it.

This is the bulk of the implementation of the async font loader: - loader now uses async runnable - platform-specific code for loading font data under OSX - code for transferring data to platform font list after async loader completes

Font data loading for DirectWrite platform font list.

Font data loading for GDI platform font list.

EnumFontFamiliesExW calls callback one per style-charset rather than simply per style, so skip subsequent calls with the same style.

Parts 1,2,3 are refactorings and moving code into a new file. Part 4 is the key piece that needs careful review since it's multi-threaded. Parts 5 & 6 are primarily reviews of Windows DirectWrite/GDI usage.

Bas, roc is on holiday and jfkthame is off for a bit, would you be able to tackle reviewing all these?

Comment on attachment 8363477 [details] [diff] [review] part 1, use CTFontManager API to enumerate font families in OSX Review of attachment 8363477 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxMacPlatformFontList.mm @@ +681,2 @@ > > + CFArrayRef familyNames = CTFontManagerCopyAvailableFontFamilyNames(); https://developer.apple.com/library/mac/documentation/Carbon/Reference/CoreText_FontManager_Ref/Reference/reference.html#//apple_ref/c/func/CTFontManagerCopyAvailableFontFamilyNames states: This function returns a retained reference to a CFArray of CFStringRef objects representing the visible font family names of the available fonts, or NULL on error. The caller is responsible for releasing the array. I don't see this being released :). I believe you'll need a simple CFRelease from what I can tell.

Comment on attachment 8363478 [details] [diff] [review] part 2 - create static helper methods for fontinfo loading Review of attachment 8363478 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxFont.cpp @@ +1157,2 @@ > const gfxFontUtils::NameHeader *nameHeader = > + reinterpret_cast<const gfxFontUtils::NameHeader*>(aNameData); Personally I'd prefer to see this happen with a typesafe helper rather than a reinterpret cast :). @@ +1208,5 @@ > + ReadOtherFamilyNamesForFace(mName, nameData, dataLength, > + otherFamilyNames, useFullName); > + > + uint32_t i, n = otherFamilyNames.Length(); > + for (i = 0; i < n; i++) { nit: Can we just declare in the for like we do everywhere else? This is just me but I was a little bit confused by the syntax. @@ +1355,5 @@ > + } > + mFamilyCharacterMap.Compact(); > + mFamilyCharacterMapInitialized = true; > +} > + nit: one less newline

Comment on attachment 8363482 [details] [diff] [review] part 4 - set up async loader intrastructure with osx implementation Review of attachment 8363482 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxFontInfoLoader.cpp @@ +145,5 @@ > + } > + > + nsCOMPtr<nsIRunnable> loadEvent = new AsyncFontInfoLoader(mFontInfo); > + > + mFontLoaderThread->Dispatch(loadEvent, NS_DISPATCH_NORMAL); I'm probably missing something but do we shutdown this thread somewhere when this is not cancelled but done? ::: gfx/thebes/gfxMacPlatformFontList.mm @@ +1041,5 @@ > +MacFontInfo::LoadFontFamilyData(const nsAString& aFamilyName) > +{ > + // family name ==> CTFontDescriptor > + NSString *famName = GetNSStringForString(aFamilyName); > + CFStringRef family = CFStringRef(famName); Don't see us ever releasing this ref. @@ +1045,5 @@ > + CFStringRef family = CFStringRef(famName); > + > + CFMutableDictionaryRef attr = > + CFDictionaryCreateMutable(NULL, 0, &kCFTypeDictionaryKeyCallBacks, > + &kCFTypeDictionaryValueCallBacks); idem @@ +1047,5 @@ > + CFMutableDictionaryRef attr = > + CFDictionaryCreateMutable(NULL, 0, &kCFTypeDictionaryKeyCallBacks, > + &kCFTypeDictionaryValueCallBacks); > + CFDictionaryAddValue(attr, kCTFontFamilyNameAttribute, family); > + CTFontDescriptorRef fd = CTFontDescriptorCreateWithAttributes(attr); idem @@ +1049,5 @@ > + &kCFTypeDictionaryValueCallBacks); > + CFDictionaryAddValue(attr, kCTFontFamilyNameAttribute, family); > + CTFontDescriptorRef fd = CTFontDescriptorCreateWithAttributes(attr); > + CFArrayRef matchingFonts = > + CTFontDescriptorCreateMatchingFontDescriptors(fd, NULL); idem @@ +1063,5 @@ > + for (f = 0; f < numFaces; f++) { > + mLoadStats.fonts++; > + > + CTFontDescriptorRef faceDesc = > + (CTFontDescriptorRef)CFArrayGetValueAtIndex(matchingFonts, f); idem

Comment on attachment 8363482 [details] [diff] [review] part 4 - set up async loader intrastructure with osx implementation Review of attachment 8363482 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxFont.h @@ +492,5 @@ > // caller is responsible to do any sanitization/validation necessary. > hb_blob_t* GetTableFromFontData(const void* aFontData, uint32_t aTableTag); > > + // lookup the cmap in cached font data > + virtual gfxCharacterMap* GetCMAPFromFontInfo(FontInfoData *aFontInfoData, Should this be already_AddRefed? ::: gfx/thebes/gfxFontInfoLoader.h @@ +194,5 @@ > }; > > + // CreateFontInfo - create platform-specific object used > + // to load system-wide font info > + virtual FontInfoData* CreateFontInfoData() { This should be an already_AddRefed<> since FontInfoData is refcounted.

Comment on attachment 8363483 [details] [diff] [review] part 5 - font info loading implementation for DirectWrite Review of attachment 8363483 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxDWriteFontList.cpp @@ +1725,5 @@ > + } > + } > +} > + > +FontInfoData* This needs to be already_AddRefed as well then.

Comment on attachment 8364210 [details] [diff] [review] part 6 - font info loading for GDI Review of attachment 8364210 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxFont.cpp @@ +1369,5 @@ > nameTable, gfxFontUtils::NAME_ID_POSTSCRIPT, psname) == NS_OK) > { > aPlatformFontList->AddPostscriptName(fe, psname); > } > + } nit: probably not the best patch to have this change in, although I don't feel strongly ::: gfx/thebes/gfxGDIFontList.cpp @@ +1087,5 @@ > + mLoadStats.othernames += data.mOtherFamilyNames.Length(); > + } > +} > + > +FontInfoData* again, already_AddRefed

(In reply to Bas Schouten (:bas.schouten) from comment #12) > Comment on attachment 8363482 [details] [diff] [review] > part 4 - set up async loader intrastructure with osx implementation > > Review of attachment 8363482 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/thebes/gfxFontInfoLoader.cpp > @@ +145,5 @@ > > + } > > + > > + nsCOMPtr<nsIRunnable> loadEvent = new AsyncFontInfoLoader(mFontInfo); > > + > > + mFontLoaderThread->Dispatch(loadEvent, NS_DISPATCH_NORMAL); > > I'm probably missing something but do we shutdown this thread somewhere when > this is not cancelled but done? Threads are automatically shutdown when no more events exist and there are no longer any references. I verified that the thread goes away after the font loader completes. > ::: gfx/thebes/gfxMacPlatformFontList.mm > @@ +1041,5 @@ > > +MacFontInfo::LoadFontFamilyData(const nsAString& aFamilyName) > > +{ > > + // family name ==> CTFontDescriptor > > + NSString *famName = GetNSStringForString(aFamilyName); > > + CFStringRef family = CFStringRef(famName); > > Don't see us ever releasing this ref. This string is created via NSString so it's handled by the NSAutoReleasePool. > @@ +1063,5 @@ > > + for (f = 0; f < numFaces; f++) { > > + mLoadStats.fonts++; > > + > > + CTFontDescriptorRef faceDesc = > > + (CTFontDescriptorRef)CFArrayGetValueAtIndex(matchingFonts, f); > > idem The array holds the reference, so there's no need to explicitly release faceDesc.

Updated based on review comments.

Updated based on review comments.

This sounds cool. (In reply to John Daggett (:jtd) from comment #0) > The interval timer to transfer font data back to the platform font > list generally isn't needed; since the transfer is basically just > swizzling around data structures, the transfer will usually occur > within the initial allotted time slice. For systems with huge numbers > of fonts (i.e. >3000 fonts) or in cases where simply enumerating fonts > takes a long time (due to overzealous virus-checker code for example) > it may take longer and the interval timer will fire multiple times. If this hardly ever runs, how are we going to test it? Should we set the timer value to some especially low value when running tests to ensure this code is tested?

Comment on attachment 8364815 [details] [diff] [review] part 1 v2, use CTFontManager API to enumerate font families in OSX Review of attachment 8364815 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/thebes/gfxMacPlatformFontList.mm @@ +686,4 @@ > > + numFamilies = CFArrayGetCount(familyNames); > + for (i = 0; i < numFamilies; i++) { > + CFStringRef family = (CFStringRef)CFArrayGetValueAtIndex(familyNames, i); I think you're correctly not releasing this.. but I wish I could find documentation proving that.

(In reply to Bas Schouten (:bas.schouten) from comment #20) > Comment on attachment 8364815 [details] [diff] [review] > part 1 v2, use CTFontManager API to enumerate font families in OSX > > Review of attachment 8364815 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/thebes/gfxMacPlatformFontList.mm > @@ +686,4 @@ > > > > + numFamilies = CFArrayGetCount(familyNames); > > + for (i = 0; i < numFamilies; i++) { > > + CFStringRef family = (CFStringRef)CFArrayGetValueAtIndex(familyNames, i); > > I think you're correctly not releasing this.. but I wish I could find > documentation proving that. It's standard Core Foundation ownership policy: "Create" and "Copy" APIs give you a retained (owning) reference that you have to release; "Get" APIs give you a non-retained reference and therefore must *not* be released by the caller (unless you explicitly Retain the object after Getting it). https://developer.apple.com/library/mac/documentation/CoreFOundation/Conceptual/CFMemoryMgmt/Concepts/Ownership.html#//apple_ref/doc/uid/20001148-SW1

Comment on attachment 8364816 [details] [diff] [review] part 4 v2 - set up async loader intrastructure with osx implementation Review of attachment 8364816 [details] [diff] [review]: ----------------------------------------------------------------- I may not have done the most thorough job reviewing this because I'm not -that- familiar with the code, but it looks pretty good now to me. ::: gfx/thebes/gfxMacPlatformFontList.mm @@ +240,5 @@ > } else { > + uint32_t kCMAP = TRUETYPE_TAG('c','m','a','p'); > + charmap = new gfxCharacterMap(); > + AutoTable cmapTable(this, kCMAP); > + nit: whitespace @@ +1056,5 @@ > + CTFontDescriptorCreateMatchingFontDescriptors(fd, NULL); > + if (!matchingFonts) { > + return; > + } > + CFRelease(fd); move this above the if clause so it gets executed even if !matchingFonts @@ +1067,5 @@ > + for (f = 0; f < numFaces; f++) { > + mLoadStats.fonts++; > + > + CTFontDescriptorRef faceDesc = > + (CTFontDescriptorRef)CFArrayGetValueAtIndex(matchingFonts, f); same as in the last review comment :s

(In reply to Bas Schouten (:bas.schouten) from comment #22) > Comment on attachment 8364816 [details] [diff] [review] > part 4 v2 - set up async loader intrastructure with osx implementation > > Review of attachment 8364816 [details] [diff] [review]: > ----------------------------------------------------------------- > > I may not have done the most thorough job reviewing this because I'm not > -that- familiar with the code, but it looks pretty good now to me. Thanks so much! > @@ +1067,5 @@ > > + for (f = 0; f < numFaces; f++) { > > + mLoadStats.fonts++; > > + > > + CTFontDescriptorRef faceDesc = > > + (CTFontDescriptorRef)CFArrayGetValueAtIndex(matchingFonts, f); > > same as in the last review comment :s These are not explicitly released, see comment 21.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #19) > This sounds cool. > > (In reply to John Daggett (:jtd) from comment #0) > > The interval timer to transfer font data back to the platform font > > list generally isn't needed; since the transfer is basically just > > swizzling around data structures, the transfer will usually occur > > within the initial allotted time slice. For systems with huge numbers > > of fonts (i.e. >3000 fonts) or in cases where simply enumerating fonts > > takes a long time (due to overzealous virus-checker code for example) > > it may take longer and the interval timer will fire multiple times. > > If this hardly ever runs, how are we going to test it? Should we set the > timer value to some especially low value when running tests to ensure this > code is tested? Under OSX the interval timer runs anytime the disabled-by-default pref gfx.font_rendering.fallback.always_use_cmaps pref is enabled, since this forces cmap loading for all fonts. Under OSX there's still a bunch of stuff we do in this case on the main thread to deal with support for complex scripts, so in that case the interval timer will fire. On older systems running XP, I imagine this will be hit in cold startup situations where simply enumerating fonts takes longer than usual. Not sure setting the timer low is such a great idea unless you mean strictly limited to some sort of environment in which tests run (e.g. for reftest/mochitest runs for example). The interval is set by gfx.font_loader.interval so that's easy enough to do.

Pushed to inbound, including changes based on review comments https://hg.mozilla.org/integration/mozilla-inbound/rev/aa98ff644f89 https://hg.mozilla.org/integration/mozilla-inbound/rev/579ca52244a1 https://hg.mozilla.org/integration/mozilla-inbound/rev/3ee447cb0b75 https://hg.mozilla.org/integration/mozilla-inbound/rev/71c900e16cf9 https://hg.mozilla.org/integration/mozilla-inbound/rev/a79047a5654f https://hg.mozilla.org/integration/mozilla-inbound/rev/3cbec86b2482

https://hg.mozilla.org/mozilla-central/rev/aa98ff644f89 https://hg.mozilla.org/mozilla-central/rev/579ca52244a1 https://hg.mozilla.org/mozilla-central/rev/3ee447cb0b75 https://hg.mozilla.org/mozilla-central/rev/71c900e16cf9 https://hg.mozilla.org/mozilla-central/rev/a79047a5654f https://hg.mozilla.org/mozilla-central/rev/3cbec86b2482

I landed a trivial wchar_t/char16_t mismatch fixup for mingw: https://hg.mozilla.org/integration/mozilla-inbound/rev/eb348e535e65