Benoit Jacob [:bjacob] (mostly away) Reporter
	Description • 10 years ago

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

	Nathan Froyd [:froydnj]
	Comment 1 • 10 years ago

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

	Nathan Froyd [:froydnj]
	Comment 3 • 10 years ago

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)

	Benoit Jacob [:bjacob] (mostly away) Reporter
	Comment 5 • 10 years ago

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

	Nathan Froyd [:froydnj]
	Comment 6 • 10 years ago

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.

	Anuj Agarwal [:anujagarwal464] Assignee
	Comment 7 • 10 years ago

@Nathan Thanks!

	Nathan Froyd [:froydnj]
	Comment 8 • 10 years ago

Comment on attachment 8445910 [details] [diff] [review]
bug1029138.diff

Review of attachment 8445910 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.

	Andrew McCreight [:mccr8]
	Comment 9 • 10 years ago

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

	Carsten Book [:Tomcat]
	Comment 10 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/99bd46e93517

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/99bd46e93517