Closed Bug 1029138 Opened 6 years ago Closed 5 years ago

Remove dangerous public destructor of nsGlyphTableList

Categories

(Core :: Layout, defect)

Other Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: bjacob, Assigned: anujagarwal464)

References

Details

Attachments

(1 file, 2 obsolete files)

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?
Flags: needinfo?(anujagarwal464)
Assignee: nobody → anujagarwal464
Flags: needinfo?(anujagarwal464)
Attached patch bug1029138.diff (obsolete) — Splinter Review
Attachment #8445843 - Flags: review?(nfroyd)
Attachment #8445843 - Flags: review?(bjacob)
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)
Attachment #8445843 - Flags: review?(nfroyd) → feedback+
Attached patch bug1029138.diff (obsolete) — Splinter Review
Attachment #8445843 - Attachment is obsolete: true
Attachment #8445843 - Flags: review?(bjacob)
Attachment #8445862 - Flags: feedback?(nfroyd)
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.
Attachment #8445862 - Flags: feedback?(nfroyd) → feedback+
Attached patch bug1029138.diffSplinter Review
@Nathan Thanks!
Attachment #8445862 - Attachment is obsolete: true
Attachment #8445910 - Flags: feedback?(nfroyd)
Comment on attachment 8445910 [details] [diff] [review]
bug1029138.diff

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

Looks good.
Attachment #8445910 - Flags: feedback?(nfroyd) → feedback+
No longer depends on: 1028588
https://hg.mozilla.org/mozilla-central/rev/99bd46e93517
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.