597212 - hold a reference to language nsIAtom from gfxFontStyle

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Karl Tomlinson (:karlt)]

gfxFontStyle doesn't keep references to the language nsIAtom.
I don't know anything that ensures the pointer remains valid.

nsRuleNode::ComputeVisibilityData ref-counts the result of do_GetAtom.

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: patch

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: remove unnecessary extra nsILanguageAtomService::LookupLanguage

cleanup

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: remove unnecessary extra nsILanguageAtomService::LookupLanguage

Forgot to remove another header.

LookupLanguage is done in gfxFontGroup::ForEachFontInternal, anyway.

Comment 4

[Jonathan Kew [:jfkthame]]

(In reply to comment #0)
> gfxFontStyle doesn't keep references to the language nsIAtom.
> I don't know anything that ensures the pointer remains valid.

Hmm, my memory of this is a bit hazy but don't the language (or langGroup) atoms come from the language atom service's LookupLanguage() method, which ensures that all the atoms it returns will remain valid (because it owns references to them in its hashtable) until the service itself is shut down? And therefore the rest of the code can rely on these atoms being "permanent" once created.

Comment 5

[Karl Tomlinson (:karlt)]

(In reply to comment #4)
> ... don't the language (or langGroup)
> atoms come from the language atom service's LookupLanguage() method

LookupLanguage only returns langGroups, so real languages don't come from here.

Comment 6

[Karl Tomlinson (:karlt)]

nsLanguageAtomService::GetLanguageGroup uses languages as keys to 
nsInterfaceHashtable<nsISupportsHashKey, nsIAtom> mLangToGroup,
but nsISupportsHashKey does not keep a reference.

Comment 7

[Jonathan Kew [:jfkthame]]

Comment on attachment 476037 [details] [diff] [review]
patch

For now I suppose we'd better do this. If we could guarantee that language atoms are always persistent once they're created, we'd be able to avoid the overhead of refcount management here and simply pass the raw atom pointers around.

Comment 8

[Karl Tomlinson (:karlt)]

(In reply to comment #6)
> nsLanguageAtomService::GetLanguageGroup uses languages as keys to 
> nsInterfaceHashtable<nsISupportsHashKey, nsIAtom> mLangToGroup,
> but nsISupportsHashKey does not keep a reference.

Oh, sorry.  nsISupportsHashKey does keep a reference.

However, I don't think we have any guarantee that GetLanguageGroup will be called for all gfxFontStyles.

Comment 9

[Karl Tomlinson (:karlt)]

Comment on attachment 476037 [details] [diff] [review]
patch

Requesting approval2.0 for the sake of attachment 476057 [details] [diff] [review] in bug 597147.

Comment 10

[Karl Tomlinson (:karlt)]

Comment on attachment 476051 [details] [diff] [review]
remove unnecessary extra nsILanguageAtomService::LookupLanguage

Sensible related clean-up.

Comment 11

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/rev/420931108036
http://hg.mozilla.org/mozilla-central/rev/6e65aa67a0b3