Closed
Bug 1029138
Opened 10 years ago
Closed 10 years ago
Remove dangerous public destructor of nsGlyphTableList
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: bjacob, Assigned: anujagarwal464)
References
Details
Attachments
(1 file, 2 obsolete files)
4.78 KB,
patch
|
froydnj
:
feedback+
|
Details | Diff | Splinter Review |
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
Comment 1•10 years ago
|
||
Anuj, does this bug look like something you'd be interested in?
Flags: needinfo?(anujagarwal464)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → anujagarwal464
Flags: needinfo?(anujagarwal464)
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8445843 -
Flags: review?(nfroyd)
Attachment #8445843 -
Flags: review?(bjacob)
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)
Attachment #8445843 -
Flags: review?(nfroyd) → feedback+
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8445843 -
Attachment is obsolete: true
Attachment #8445843 -
Flags: review?(bjacob)
Attachment #8445862 -
Flags: feedback?(nfroyd)
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 :-)
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.
Attachment #8445862 -
Flags: feedback?(nfroyd) → feedback+
Assignee | ||
Comment 7•10 years ago
|
||
@Nathan Thanks!
Attachment #8445862 -
Attachment is obsolete: true
Attachment #8445910 -
Flags: feedback?(nfroyd)
Comment 8•10 years ago
|
||
Comment on attachment 8445910 [details] [diff] [review] bug1029138.diff Review of attachment 8445910 [details] [diff] [review]: ----------------------------------------------------------------- Looks good.
Attachment #8445910 -
Flags: feedback?(nfroyd) → feedback+
Updated•10 years ago
|
Keywords: checkin-needed
Comment 10•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/99bd46e93517
Keywords: checkin-needed
Comment 11•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/99bd46e93517
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in
before you can comment on or make changes to this bug.
Description
•