Closed Bug 1035394 Opened 5 years ago Closed 5 years ago

Add dangerous public destructor detection to _INHERITED refcounting macros

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: bjacob, Assigned: bjacob)

References

Details

Attachments

(25 files)

2.09 KB, patch
khuey
: review+
Details | Diff | Splinter Review
34.15 KB, patch
surkov
: review+
Details | Diff | Splinter Review
26.97 KB, patch
khuey
: review+
Details | Diff | Splinter Review
84.26 KB, patch
smaug
: review+
Details | Diff | Splinter Review
820 bytes, patch
karlt
: review+
Details | Diff | Splinter Review
2.46 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
24.24 KB, patch
padenot
: review+
Details | Diff | Splinter Review
2.59 KB, patch
smaug
: review+
Details | Diff | Splinter Review
14.00 KB, patch
cpearce
: review+
Details | Diff | Splinter Review
17.85 KB, patch
heycam
: review+
Details | Diff | Splinter Review
9.11 KB, patch
smaug
: review+
Details | Diff | Splinter Review
3.86 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
16.43 KB, patch
smaug
: review+
Details | Diff | Splinter Review
26.11 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
15.82 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
9.00 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
11.87 KB, patch
mcmanus
: review+
Details | Diff | Splinter Review
767 bytes, patch
wchen
: review+
Details | Diff | Splinter Review
2.53 KB, patch
briansmith
: review+
Details | Diff | Splinter Review
630 bytes, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
6.96 KB, patch
ehsan
: review+
Details | Diff | Splinter Review
809 bytes, patch
BenWa
: review+
Details | Diff | Splinter Review
1012 bytes, patch
smaug
: review+
Details | Diff | Splinter Review
13.79 KB, patch
roc
: review+
Details | Diff | Splinter Review
607 bytes, patch
khuey
: review+
Details | Diff | Splinter Review
This is a follow-up of bug 1028588, which added such detection to much of nsISupportsImpl.h but left out _INHERITED macros.
Attached patch fix accessibleSplinter Review
Attachment #8451846 - Flags: review?(surkov.alexander)
Attached patch fix content/baseSplinter Review
Attachment #8451848 - Flags: review?(khuey)
Attached patch fix content/htmlSplinter Review
Attachment #8451849 - Flags: review?(bzbarsky)
Attachment #8451850 - Flags: review?(karlt)
Attachment #8451851 - Flags: review?(cpearce)
Attachment #8451855 - Flags: review?(cpearce)
Comment on attachment 8451846 [details] [diff] [review]
fix accessible

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

rs=me

::: accessible/xul/XULListboxAccessible.cpp
@@ +742,5 @@
>  }
>  
> +XULListCellAccessible::~XULListCellAccessible()
> +{
> +}

any benefits of moving it into cpp file?
Attachment #8451846 - Flags: review?(surkov.alexander) → review+
Attached patch fix content/svgSplinter Review
Attachment #8451856 - Flags: review?(cam)
Attached patch fix content/xmlSplinter Review
Attachment #8451858 - Flags: review?(bugs)
Attachment #8451851 - Flags: review?(cpearce) → review+
Attached patch fix content/xulSplinter Review
Attachment #8451865 - Flags: review?(mrbkap)
Attachment #8451868 - Attachment is patch: true
Attachment #8451868 - Flags: review?(ehsan)
Attached patch fix editorSplinter Review
Attachment #8451870 - Flags: review?(ehsan)
Attached patch fix layoutSplinter Review
Attachment #8451871 - Flags: review?(dbaron)
Attached patch fix netwerkSplinter Review
Attachment #8451872 - Flags: review?(mcmanus)
Attached patch fix parserSplinter Review
Attachment #8451873 - Flags: review?(wchen)
Attachment #8451855 - Flags: review?(cpearce) → review+
Attached patch fix securitySplinter Review
Attachment #8451874 - Flags: review?(brian)
Attached patch fix storageSplinter Review
Attachment #8451875 - Flags: review?(bent.mozilla)
Attachment #8451876 - Flags: review?(ehsan)
Attachment #8451877 - Flags: review?(bgirard)
Attached patch fix uriloaderSplinter Review
Attachment #8451880 - Flags: review?(bugs)
Attached patch fix widgetSplinter Review
Attachment #8451881 - Flags: review?(roc)
Attached patch fix xpcomSplinter Review
Attachment #8451882 - Flags: review?(khuey)
Attachment #8451877 - Flags: review?(bgirard) → review+
Comment on attachment 8451849 [details] [diff] [review]
fix content/html

r+ except struct HasDangerousPublicDestructor<dom::HTMLFormElement> 
needs an explanation or a fix.
Attachment #8451849 - Flags: review?(bzbarsky) → review+
Attachment #8451865 - Flags: review?(mrbkap) → review+
Attachment #8451872 - Flags: review?(mcmanus) → review+
Attachment #8451875 - Flags: review?(bent.mozilla) → review+
Attachment #8451858 - Flags: review?(bugs) → review+
Attachment #8451880 - Flags: review?(bugs) → review+
Attachment #8451854 - Flags: review?(bugs) → review+
Attachment #8451867 - Flags: review?(bugs) → review+
Attachment #8451850 - Flags: review?(karlt) → review+
Attachment #8451856 - Flags: review?(cam) → review+
Attachment #8451874 - Flags: review?(brian) → review+
Attachment #8451871 - Flags: review?(dbaron) → review+
Attachment #8451873 - Flags: review?(wchen) → review+
Attachment #8451853 - Flags: review?(paul) → review+
Attachment #8451868 - Flags: review?(ehsan) → review+
Attachment #8451870 - Flags: review?(ehsan) → review+
Attachment #8451876 - Flags: review?(ehsan) → review+
Comment on attachment 8451848 [details] [diff] [review]
fix content/base

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

::: content/base/src/DOMQuad.cpp
@@ +137,5 @@
>      *aY2 = y2;
>    }
>  
>  protected:
> +  ~QuadBounds() {}

virtual please

::: content/base/src/nsObjectLoadingContent.cpp
@@ +390,5 @@
>    // nsITimerCallback
>    NS_IMETHOD Notify(nsITimer *timer);
>  
> +protected:
> +  ~nsStopPluginRunnable() {}

virtual please

::: content/base/src/nsXMLHttpRequest.h
@@ +172,5 @@
>      return mListenerManager && mListenerManager->HasListeners();
>    }
> +
> +private:
> +  ~nsXMLHttpRequestUpload() {}

virtual please
Attachment #8451848 - Flags: review?(khuey) → review+
(In reply to Olli Pettay [:smaug] from comment #27)
> Comment on attachment 8451849 [details] [diff] [review]
> fix content/html
> 
> r+ except struct HasDangerousPublicDestructor<dom::HTMLFormElement> 
> needs an explanation or a fix.

So if I make that destructor protected, I get:

 0:09.45 /home/bjacob/hack/mozilla-central/content/html/content/src/HTMLFormElement.cpp:75:12: error: calling a protected destructor of class 'mozilla::dom::HTMLFormElement'
 0:09.46     delete it;
 0:09.46            ^
 
The code in question is

http://dxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLFormElement.cpp#75

It looks very easy to fix. I'll file the followup bug as I land.
(In reply to alexander :surkov from comment #10)
> Comment on attachment 8451846 [details] [diff] [review]
> fix accessible
> 
> Review of attachment 8451846 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> rs=me
> 
> ::: accessible/xul/XULListboxAccessible.cpp
> @@ +742,5 @@
> >  }
> >  
> > +XULListCellAccessible::~XULListCellAccessible()
> > +{
> > +}
> 
> any benefits of moving it into cpp file?

None. That was just my default action to be able to process all the failures quickly. Indeed, that's the safe choice as it doesn't rely on all the definitions to be #included in the header file. But I tried, and in the present case, it works, even in non-unified build. So I'll do that.
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #28)
> virtual please

Done (x3).
Follow-up bugs to fix whitelisted dangerous destructors have been file; there were only 3. The tracking bug is bug 1028132.
https://hg.mozilla.org/mozilla-central/rev/959a72081da6
https://hg.mozilla.org/mozilla-central/rev/d44b04df56ae
https://hg.mozilla.org/mozilla-central/rev/f25ec3d47ca0
https://hg.mozilla.org/mozilla-central/rev/67b022015900
https://hg.mozilla.org/mozilla-central/rev/6af74fb23859
https://hg.mozilla.org/mozilla-central/rev/fcc1ebdf82c0
https://hg.mozilla.org/mozilla-central/rev/3f0f389821f7
https://hg.mozilla.org/mozilla-central/rev/821dd7192068
https://hg.mozilla.org/mozilla-central/rev/18dd3958f2f6
https://hg.mozilla.org/mozilla-central/rev/15ea98d307c5
https://hg.mozilla.org/mozilla-central/rev/e5f8bd650ef1
https://hg.mozilla.org/mozilla-central/rev/ad2e6df50240
https://hg.mozilla.org/mozilla-central/rev/cddfa382a2ba
https://hg.mozilla.org/mozilla-central/rev/f6fd4bafabdf
https://hg.mozilla.org/mozilla-central/rev/53bc1bd562ac
https://hg.mozilla.org/mozilla-central/rev/6e71cf4db1ad
https://hg.mozilla.org/mozilla-central/rev/dcd4f09949ce
https://hg.mozilla.org/mozilla-central/rev/74685fc519f4
https://hg.mozilla.org/mozilla-central/rev/bb3aaa6cdaa7
https://hg.mozilla.org/mozilla-central/rev/d070ae6e63cb
https://hg.mozilla.org/mozilla-central/rev/5821b55a574c
https://hg.mozilla.org/mozilla-central/rev/2f5b2fbae4fb
https://hg.mozilla.org/mozilla-central/rev/cd28b3d3a4bd
https://hg.mozilla.org/mozilla-central/rev/683e40882500
https://hg.mozilla.org/mozilla-central/rev/558231b7c91c
Assignee: nobody → bjacob
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Depends on: 1036619
Accidentally used this bug number in https://hg.mozilla.org/integration/b2g-inbound/rev/86702af059f4 . The intended bug number was bug 1036394.
You need to log in before you can comment on or make changes to this bug.