Closed
Bug 1035394
Opened 10 years ago
Closed 10 years ago
Add dangerous public destructor detection to _INHERITED refcounting macros
Categories
(Core :: XPCOM, defect)
Core
XPCOM
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.akhgari
:
review+
|
Details | Diff | Splinter Review |
15.82 KB,
patch
|
ehsan.akhgari
:
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.akhgari
:
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.
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8451843 -
Flags: review?(khuey)
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8451846 -
Flags: review?(surkov.alexander)
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8451848 -
Flags: review?(khuey)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8451849 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8451850 -
Flags: review?(karlt)
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8451851 -
Flags: review?(cpearce)
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8451853 -
Flags: review?(paul)
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8451854 -
Flags: review?(bugs)
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8451855 -
Flags: review?(cpearce)
Comment 10•10 years ago
|
||
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+
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8451856 -
Flags: review?(cam)
Assignee | ||
Comment 12•10 years ago
|
||
Attachment #8451858 -
Flags: review?(bugs)
Updated•10 years ago
|
Attachment #8451851 -
Flags: review?(cpearce) → review+
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8451865 -
Flags: review?(mrbkap)
Assignee | ||
Comment 14•10 years ago
|
||
Attachment #8451867 -
Flags: review?(bugs)
Assignee | ||
Comment 15•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8451868 -
Attachment is patch: true
Attachment #8451868 -
Flags: review?(ehsan)
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8451870 -
Flags: review?(ehsan)
Assignee | ||
Comment 17•10 years ago
|
||
Attachment #8451871 -
Flags: review?(dbaron)
Assignee | ||
Comment 18•10 years ago
|
||
Attachment #8451872 -
Flags: review?(mcmanus)
Assignee | ||
Comment 19•10 years ago
|
||
Attachment #8451873 -
Flags: review?(wchen)
Updated•10 years ago
|
Attachment #8451855 -
Flags: review?(cpearce) → review+
Assignee | ||
Comment 20•10 years ago
|
||
Attachment #8451874 -
Flags: review?(brian)
Assignee | ||
Comment 21•10 years ago
|
||
Attachment #8451875 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 22•10 years ago
|
||
Attachment #8451876 -
Flags: review?(ehsan)
Assignee | ||
Comment 23•10 years ago
|
||
Attachment #8451877 -
Flags: review?(bgirard)
Assignee | ||
Comment 24•10 years ago
|
||
Attachment #8451880 -
Flags: review?(bugs)
Assignee | ||
Comment 25•10 years ago
|
||
Attachment #8451881 -
Flags: review?(roc)
Assignee | ||
Comment 26•10 years ago
|
||
Attachment #8451882 -
Flags: review?(khuey)
Updated•10 years ago
|
Attachment #8451877 -
Flags: review?(bgirard) → review+
Comment 27•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8451865 -
Flags: review?(mrbkap) → review+
Updated•10 years ago
|
Attachment #8451872 -
Flags: review?(mcmanus) → review+
Updated•10 years ago
|
Attachment #8451875 -
Flags: review?(bent.mozilla) → review+
Updated•10 years ago
|
Attachment #8451858 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8451880 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8451854 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8451867 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8451850 -
Flags: review?(karlt) → review+
Updated•10 years ago
|
Attachment #8451856 -
Flags: review?(cam) → review+
Attachment #8451882 -
Flags: review?(khuey) → review+
Updated•10 years ago
|
Attachment #8451874 -
Flags: review?(brian) → review+
Attachment #8451871 -
Flags: review?(dbaron) → review+
Updated•10 years ago
|
Attachment #8451873 -
Flags: review?(wchen) → review+
Attachment #8451881 -
Flags: review?(roc) → review+
Updated•10 years ago
|
Attachment #8451853 -
Flags: review?(paul) → review+
Updated•10 years ago
|
Attachment #8451868 -
Flags: review?(ehsan) → review+
Updated•10 years ago
|
Attachment #8451870 -
Flags: review?(ehsan) → review+
Updated•10 years ago
|
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+
Attachment #8451843 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 29•10 years ago
|
||
(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.
Assignee | ||
Comment 30•10 years ago
|
||
(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.
Assignee | ||
Comment 31•10 years ago
|
||
Landed everything: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=558231b7c91c Green try push (aside from Git server error): https://tbpl.mozilla.org/?tree=Try&rev=f53b52969d45
Assignee | ||
Comment 32•10 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #28) > virtual please Done (x3).
Assignee | ||
Comment 33•10 years ago
|
||
Follow-up bugs to fix whitelisted dangerous destructors have been file; there were only 3. The tracking bug is bug 1028132.
Comment 34•10 years ago
|
||
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: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Comment 35•10 years ago
|
||
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.
Description
•