In bug 1028588 we removed dangerous public destructors of XPCOM-refcounted classes outside of a finite whitelist, see HasDangerousPublicDestructor. Now we are going over the entries in this whitelist. One of them is: nsGlyphTableList

Anuj, does this bug look like something you'd be interested in?

Comment on attachment 8445843 [details] [diff] [review] bug1029138.diff Review of attachment 8445843 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for working on this. This is one of those not-quite-straightforward cases of removing the public destructor. ::: layout/mathml/nsMathMLChar.cpp @@ +600,5 @@ > nsGlyphTable* > GetGlyphTableFor(const nsAString& aFamily); > > private: > + virtual ~nsGlyphTableList() Can you remove the |virtual| while you're here? There's no reason for it to be here in the first place. @@ -717,5 @@ > if (gGlyphTableList) { > rv = gGlyphTableList->Initialize(); > } > if (NS_FAILED(rv)) { > - delete gGlyphTableList; Removing this means that we leak gGlyphTableList in the failure case, which is undesirable. The easiest way I can think of to solve this is: 1. Change this function to use a local variable: nsRefPtr<nsGlyphTableList> glyphTableList = new nsGlyphTableList(); and fix all the uses of gGlyphTableList in this function. 2. At some appropriate point in the function, do: glyphTableList.forget(&gGlyphTableList); to transfer ownership to the global variable. 3. Inside nsGlyphTableList::Finalize(), call: NS_IF_RELEASE(gGlyphTableList)

Thanks Nathan for taking this. I hadn't started looking at this. I assume I don't need to now :-)

Comment on attachment 8445862 [details] [diff] [review] bug1029138.diff Review of attachment 8445862 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/mathml/nsMathMLChar.cpp @@ +712,4 @@ > } > if (NS_FAILED(rv)) { > + glyphTableList.forget(&gGlyphTableList); > + glyphTableList = nullptr; You should just delete the original code here. @@ +727,4 @@ > #endif > ) { > rv = NS_ERROR_OUT_OF_MEMORY; > } ...and move the .forget() call to here.

@Nathan Thanks!

Comment on attachment 8445910 [details] [diff] [review] bug1029138.diff Review of attachment 8445910 [details] [diff] [review]: ----------------------------------------------------------------- Looks good.

try run: https://tbpl.mozilla.org/?tree=Try&rev=4295186114cb