1. Put computer under heavy load 2. Load the testcase 3. Quit Result: Assertion failure: IsIdle(oldState), at xpcom/glue/PLDHashTable.h:132 ...sometimes.

Partial stack: * frame #0: XUL`Checker::StartWriteOp frame #1: XUL`AutoWriteOp::AutoWriteOp frame #2: XUL`AutoWriteOp::AutoWriteOp frame #3: XUL`PLDHashTable::Add frame #4: XUL`PLDHashTable::Add frame #5: XUL`nsTHashtable<nsStringHashKey>::PutEntry frame #6: XUL`gfxPlatformFontList::FindAndAddFamilies frame #7: XUL`gfxMacPlatformFontList::FindAndAddFamilies frame #8: XUL`gfxPlatformFontList::FindFamily frame #9: XUL`gfxPlatformFontList::CleanupLoader frame #10: XUL`gfxFontInfoLoader::CancelLoader frame #11: XUL`gfxFontInfoLoader::ShutdownObserver::Observe frame #12: XUL`nsObserverList::NotifyObservers frame #13: XUL`nsObserverService::NotifyObservers frame #14: XUL`nsAppStartup::Quit frame 9, gfxPlatformFontList::CleanupLoader, is iterating over mOtherNamesMissed frame 6, gfxPlatformFontList::FindAndAddFamilies, adds an entry to mOtherNamesMissed. Modifying a PLDHashTable while iterating over it is unsafe. In debug builds, it aborts. In release builds, IIRC, it can lead to the use of dangling pointers, especially if the hash table is resized.

I'm guessing the time-based heuristics in gfxPlatformFontList.cpp are responsible for the intermittency :/

(In reply to Jesse Ruderman from comment #1) >... > > frame 9, gfxPlatformFontList::CleanupLoader, is iterating over > mOtherNamesMissed > > frame 6, gfxPlatformFontList::FindAndAddFamilies, adds an entry to > mOtherNamesMissed. > > Modifying a PLDHashTable while iterating over it is unsafe. In debug builds, > it aborts. In release builds, IIRC, it can lead to the use of dangling > pointers, especially if the hash table is resized. Didn't think it'd be a problem as we break out of the iteration loop if FindAndAddFamilies adds an entry, but I guess it is.

Comment on attachment 8923591 [details] [diff] [review] When iterating for deletion, do not add more elements. r=jfkthame https://treeherder.mozilla.org/#/jobs?repo=try&revision=db8c0765a28e8360d367d188270731ecbd08e1f2

::CleanupLoader does appear to be the only place that needs this different behaviour (don't create if you don't find it), calling through FindFamily() into FindAndAddFamilies().

Comment on attachment 8923591 [details] [diff] [review] When iterating for deletion, do not add more elements. r=jfkthame Review of attachment 8923591 [details] [diff] [review]: ----------------------------------------------------------------- Looks reasonable, except for one small detail... r=me with that fixed. ::: gfx/thebes/gfxPlatformFontList.h @@ +152,5 @@ > // family name for a styled variant. > + eNoSearchForLegacyFamilyNames = 1 << 1, > + > + // If set, FindAndAddFamilies will not add a missing entry to mOtherNamesMissed > + eNoAddToNamesMissedWhenSearching = 1 << 1, I think you meant "1 << 2" here?

(In reply to Jonathan Kew (:jfkthame) from comment #7) > ... > > Looks reasonable, except for one small detail Kind of you calling it a small detail that makes the code not work properly :)

I can't recall if sec-moderate needs a security review or not, so here's a request anyway. [Security approval request comment] How easily could an exploit be constructed based on the patch? Not easily. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Not really. Which older supported branches are affected by this flaw? All of them, but this is sec-moderate so I'm not sure how far back we need to go. If not all supported branches, which bug introduced the flaw? Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Backports should be easy. How likely is this patch to cause regressions; how much testing does it need? Not likely, will receive testing through regular Firefox usage.

https://wiki.mozilla.org/Security/Bug_Approval_Process says you can land a sec-moderate directly; only high or critical are supposed to wait for approval.

Comment on attachment 8924174 [details] [diff] [review] When iterating for deletion, do not add more elements. Carry r=jfkthame Yes, as a sec-moderate, this doesn't need sec-approval to land on trunk.

https://hg.mozilla.org/mozilla-central/rev/a9930291f639 I'm going to assume this is something we don't need to worry about backporting to ESR52, but feel free to set the status to affected and nominate it for approval if you feel strongly otherwise. Same for 57.