1281965 - Hash table re-entrancy during shutdown with non-ASCII font names

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Jesse Ruderman]

Attachment: testcase (intermittent)

1. Put computer under heavy load
2. Load the testcase
3. Quit

Result:

Assertion failure: IsIdle(oldState), at xpcom/glue/PLDHashTable.h:132

...sometimes.

Comment 1

[Jesse Ruderman]

Attachment: stack+

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.

Comment 2

[Jesse Ruderman]

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

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

(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 4

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: When iterating for deletion, do not add more elements. r=jfkthame

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

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

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

::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 7

[Jonathan Kew [:jfkthame]]

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?

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

(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 :)

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: When iterating for deletion, do not add more elements. Carry r=jfkthame

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.

Comment 10

[Jonathan Kew [:jfkthame]]

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 11

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9930291f639145b9a612de69fca1804cfb4d4f3

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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.