Closed
Bug 1028588
Opened 10 years ago
Closed 10 years ago
Add dangerous public destructor detection to much of the rest of nsISupportsImpl.h (still not covering NS_IMPL_ADDREF_INHERITED)
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: bjacob, Assigned: bjacob)
References
Details
Attachments
(39 files, 2 obsolete files)
1.01 MB,
patch
|
Details | Diff | Splinter Review | |
10.00 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
41.52 KB,
patch
|
froydnj
:
review+
froydnj
:
review+
benjamin
:
review+
|
Details | Diff | Splinter Review |
164.93 KB,
patch
|
mcmanus
:
review+
|
Details | Diff | Splinter Review |
13.29 KB,
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
24.81 KB,
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
6.76 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
24.74 KB,
patch
|
bholley
:
review+
|
Details | Diff | Splinter Review |
6.76 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
162.00 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
21.52 KB,
patch
|
jrmuizel
:
review+
|
Details | Diff | Splinter Review |
34.09 KB,
patch
|
cpearce
:
review+
smaug
:
review+
|
Details | Diff | Splinter Review |
29.31 KB,
patch
|
heycam
:
review+
|
Details | Diff | Splinter Review |
111.66 KB,
patch
|
khuey
:
review+
|
Details | Diff | Splinter Review |
23.70 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
20.39 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
6.39 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
70.19 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
23.41 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
17.52 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
37.54 KB,
patch
|
briansmith
:
review+
|
Details | Diff | Splinter Review |
10.94 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
9.22 KB,
patch
|
tbsaunde
:
review+
|
Details | Diff | Splinter Review |
22.36 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
11.29 KB,
patch
|
wchen
:
review+
|
Details | Diff | Splinter Review |
53.92 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
7.46 KB,
patch
|
enndeakin
:
review+
|
Details | Diff | Splinter Review |
22.92 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
6.11 KB,
patch
|
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
620 bytes,
patch
|
bent.mozilla
:
review+
|
Details | Diff | Splinter Review |
9.21 KB,
patch
|
bugzilla
:
review+
|
Details | Diff | Splinter Review |
4.99 KB,
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
7.79 KB,
patch
|
jesup
:
review+
|
Details | Diff | Splinter Review |
9.29 KB,
patch
|
abr
:
review+
|
Details | Diff | Splinter Review |
9.62 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
6.34 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
1.15 KB,
patch
|
tbsaunde
:
review+
|
Details | Diff | Splinter Review |
18.17 KB,
patch
|
mstange
:
review+
|
Details | Diff | Splinter Review |
2.87 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
Bug 1027251 added detection of dangerous public destructors for refcounted classes using any of NS_INLINE_DECL_REFCOUNTING NS_INLINE_DECL_THREADSAFE_REFCOUNTING but missed classes using any of NS_INLINE_DECL_CYCLE_COLLECTING_NATIVE_REFCOUNTING NS_IMPL* The latter case likely covers most nsISupports child classes.
Assignee | ||
Comment 1•10 years ago
|
||
Thanks to khuey for noticing this.
Assignee | ||
Comment 2•10 years ago
|
||
Wow. I didn't realize how much we were still using for-real XPCOM (nsISupports) as opposed to XPCOM-as-a-repo-of-C++-helopers (e.g. these NS_DECL_INLINE... macros). This patch fixes it all for me on linux, and it's 1.1 M. To make it, I edited the static assertion so it prints the faulty class name as part of the assertion message, and I made a command parsing compiler errors and automatically telling me the faulty class name and opening a text editor at the location of the error... with that, I could iterate at roughly 10 seconds per faulty class in average... still took me a good chunk of the weekend. Here was my command: ./mach build binaries 2>&1 | grep 'Reference-counted class' | sed 's/.*\(\/hack\/[^\:]*\)\:\([0-9]*\)\:.*Reference\-counted\ class\ \([^\ ]*\).*/echo \3 \&\& kate \1 -l \2/g' > script.sh && sh script.sh *REVIEWERS NOTE*: please be very nice with style nits. This is a huge patch and it bitrots very fast. Also, it allows us to statically verify that things that should be private, are private. So please don't nitpick too **** implicit-private-at-start-of-class versus explicit-private. I understand that that's a valid style conversation to have. But the value of landing this before it bitrots is IMHO higher!
Assignee | ||
Comment 3•10 years ago
|
||
There are 3 things in this patch: - extends the static checking of destructor non-publicness to all XPCOM refcounting - moves the checking code up in this file, as it's now used earlier in this file than it used to be - changes the static_assert message to now say which class is at fault. Very useful for fixing occurrences, see comment 0.
Attachment #8444500 -
Flags: review?(khuey)
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8444506 -
Flags: review?(nfroyd)
Attachment #8444506 -
Flags: review?(khuey)
Comment on attachment 8444500 [details] [diff] [review] Last patch to land: nsISupportsImpl.h changes to extend the static checking to much of XPCOM refcounting Review of attachment 8444500 [details] [diff] [review]: ----------------------------------------------------------------- This still doesn't cover things that use NS_IMPL_ADDREF_INHERITED, does it?
Attachment #8444500 -
Flags: review?(khuey) → review+
Assignee | ||
Updated•10 years ago
|
Attachment #8444506 -
Flags: review?(benjamin)
Assignee | ||
Comment 6•10 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #5) > Comment on attachment 8444500 [details] [diff] [review] > Last patch to land: nsISupportsImpl.h changes to extend the static checking > to all XPCOM refcounting > > Review of attachment 8444500 [details] [diff] [review]: > ----------------------------------------------------------------- > > This still doesn't cover things that use NS_IMPL_ADDREF_INHERITED, does it? You're right AFAICS I missed it! Renaming the bug accordingly.
Assignee | ||
Updated•10 years ago
|
Summary: Add dangerous public destructor detection to the rest of nsISupportsImpl.h → Add dangerous public destructor detection to much of the rest of nsISupportsImpl.h (still not covering NS_IMPL_ADDREF_INHERITED)
Assignee | ||
Updated•10 years ago
|
Attachment #8444500 -
Attachment description: Last patch to land: nsISupportsImpl.h changes to extend the static checking to all XPCOM refcounting → Last patch to land: nsISupportsImpl.h changes to extend the static checking to much of XPCOM refcounting
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8444514 -
Flags: review?(mcmanus)
Assignee | ||
Comment 8•10 years ago
|
||
Attachment #8444515 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8444517 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #8444521 -
Flags: review?(sphink)
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8444523 -
Flags: review?(bobbyholley)
Assignee | ||
Comment 12•10 years ago
|
||
The Codegen.py part of this patch makes the code generator not generate public destructors.
Attachment #8444525 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 13•10 years ago
|
||
There's a ton of stuff there and I'm feeling too lazy to go hunt the right reviewer for each subdir... Ehsan, is it OK to ask to you review that?
Attachment #8444528 -
Flags: review?(ehsan)
Comment 14•10 years ago
|
||
Comment on attachment 8444506 [details] [diff] [review] xpcom/ fixes Review of attachment 8444506 [details] [diff] [review]: ----------------------------------------------------------------- Re-r+'ing for khuey, since his r+ got lost. r+, I suppose; I have a slight preference for explicitly marking the destructor as |private:|, rather than relying on the default visibility of |class| members, but I don't think it's worth redoing this patch over. Benjamin can disagree if he likes.
Attachment #8444506 -
Flags: review?(nfroyd)
Attachment #8444506 -
Flags: review?(khuey)
Attachment #8444506 -
Flags: review+
Comment 15•10 years ago
|
||
(In reply to Nathan Froyd (:froydnj) from comment #14) > Re-r+'ing for khuey, since his r+ got lost. Whoops, maybe not. Anyway, Kyle indicated he's OK with r+ here.
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8444534 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 17•10 years ago
|
||
Attachment #8444535 -
Flags: review?(cpearce)
Assignee | ||
Comment 18•10 years ago
|
||
Attachment #8444536 -
Flags: review?(cam)
Assignee | ||
Comment 19•10 years ago
|
||
...and the winner of the uncategorized content/ bulk is again... Ehsan!!
Attachment #8444538 -
Flags: review?(ehsan)
Assignee | ||
Comment 20•10 years ago
|
||
It was deeply unfair to ask Ehsan to review all that stuff that he didn't necessarily have any particular affinity for. To make up for it, here's to you -- your personal favorite toplevel directory.
Attachment #8444540 -
Flags: review?(ehsan)
Assignee | ||
Comment 21•10 years ago
|
||
Attachment #8444541 -
Flags: review?(benjamin)
Assignee | ||
Comment 22•10 years ago
|
||
Attachment #8444543 -
Flags: review?(bugs)
Assignee | ||
Comment 23•10 years ago
|
||
Attachment #8444544 -
Flags: review?(dbaron)
Assignee | ||
Comment 24•10 years ago
|
||
Note that this is only what I found while building on Linux. Obviously, building on other platforms with the static checking patch, would turn up more cases. But this should be good to take even if it's not all of what needs fixing.
Attachment #8444545 -
Flags: review?(roc)
Assignee | ||
Comment 25•10 years ago
|
||
Attachment #8444549 -
Flags: review?(bugs)
Assignee | ||
Comment 26•10 years ago
|
||
Attachment #8444550 -
Flags: review?(brian)
Assignee | ||
Comment 27•10 years ago
|
||
Attachment #8444551 -
Flags: review?(benjamin)
Updated•10 years ago
|
Attachment #8444523 -
Flags: review?(bobbyholley) → review+
Updated•10 years ago
|
Attachment #8444515 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 28•10 years ago
|
||
Attachment #8444552 -
Flags: review?(trev.saunders)
Assignee | ||
Comment 29•10 years ago
|
||
Who is reviewing this toplevel dir? hg log says: Ehsan!
Attachment #8444553 -
Flags: review?(ehsan)
Updated•10 years ago
|
Attachment #8444517 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 30•10 years ago
|
||
Attachment #8444553 -
Attachment is obsolete: true
Attachment #8444553 -
Flags: review?(ehsan)
Attachment #8444555 -
Flags: review?(ehsan)
Assignee | ||
Comment 31•10 years ago
|
||
Attachment #8444557 -
Flags: review?(hsivonen)
Assignee | ||
Comment 32•10 years ago
|
||
I have no idea who should review this or event how this would split. Ehsan, your name turned up the most frequently in hg logs... do feel free to reassign review.
Attachment #8444562 -
Flags: review?(ehsan)
Assignee | ||
Comment 33•10 years ago
|
||
Attachment #8444567 -
Flags: review?(enndeakin)
Comment 34•10 years ago
|
||
Comment on attachment 8444525 [details] [diff] [review] dom/bindings/ fixes > # We need a public virtual destructor our subclasses can use Fix this comment to say "protected" instead of "public", or just "a virtual destructor"? r=me with that
Attachment #8444525 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 35•10 years ago
|
||
Attachment #8444571 -
Flags: review?(ehsan)
Assignee | ||
Comment 36•10 years ago
|
||
Attachment #8444573 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 37•10 years ago
|
||
Attachment #8444574 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 38•10 years ago
|
||
Attachment #8444577 -
Flags: review?(aklotz)
Assignee | ||
Comment 39•10 years ago
|
||
Attachment #8444578 -
Flags: review?(benjamin)
Comment 40•10 years ago
|
||
Comment on attachment 8444543 [details] [diff] [review] docshell/ fixes > class nsDownloadHistory MOZ_FINAL : public nsIDownloadHistory > { >+ ~nsDownloadHistory() {} explicit private: section after public: please.
Attachment #8444543 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 41•10 years ago
|
||
Attachment #8444581 -
Flags: review?(rjesup)
Assignee | ||
Comment 42•10 years ago
|
||
Attachment #8444582 -
Flags: review?(adam)
Updated•10 years ago
|
Attachment #8444549 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 43•10 years ago
|
||
Attachment #8444585 -
Flags: review?(ehsan)
Comment 44•10 years ago
|
||
Comment on attachment 8444552 [details] [diff] [review] accessible/ fixes I'd prefer empty dtors be defined inline, but whatever
Attachment #8444552 -
Flags: review?(trev.saunders) → review+
Updated•10 years ago
|
Attachment #8444528 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 45•10 years ago
|
||
Landed some already r+'d patches: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/2c62d4b7b055 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/4976dc6ee72a remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/3d6b2a02b254 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/477fcc7c9f60 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/4ccc78ad6d2a remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/31aa508e8c6d
Assignee: nobody → bjacob
Whiteboard: [leave open]
Comment 46•10 years ago
|
||
Comment on attachment 8444521 [details] [diff] [review] jsd/ fixes Review of attachment 8444521 [details] [diff] [review]: ----------------------------------------------------------------- Style is a little weird. And I don't care in the least, because JSD will soon be dead dead dead.
Attachment #8444521 -
Flags: review?(sphink) → review+
Comment 47•10 years ago
|
||
Comment on attachment 8444577 [details] [diff] [review] libjar/ fixes Review of attachment 8444577 [details] [diff] [review]: ----------------------------------------------------------------- ::: modules/libjar/nsZipArchive.h @@ +94,5 @@ > class nsZipArchive > { > friend class nsZipFind; > > + /** destructing the object closes the archive */ explicit private: please ::: modules/libjar/zipwriter/src/nsZipHeader.h @@ +23,5 @@ > // Combine file type attributes with unix style permissions > #define ZIP_ATTRS(p, a) ((p & 0xfff) << 16) | a > > class nsZipHeader MOZ_FINAL : public nsIZipEntry > { explicit private: please
Attachment #8444577 -
Flags: review?(aklotz) → review+
Assignee | ||
Comment 48•10 years ago
|
||
Landed dom/, accessible/, js/jsd/. remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/1679e9d569c3 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/967e19ae406c remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/72d774431043
Updated•10 years ago
|
Attachment #8444574 -
Flags: review?(bent.mozilla) → review+
Comment 49•10 years ago
|
||
Comment on attachment 8444550 [details] [diff] [review] security/ fixes Review of attachment 8444550 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/manager/pki/src/nsFormSigningDialog.h @@ +13,5 @@ > { 0x8d, 0x5a, 0x31, 0x11, 0xa6, 0xd1, 0x7e, 0xc7 }} > > class nsFormSigningDialog MOZ_FINAL : public nsIFormSigningDialog > { > + ~nsFormSigningDialog(); Please use an explicit "private:" section. ::: security/manager/ssl/src/nsCertTree.h @@ +37,5 @@ > }; > > class nsCertAddonInfo MOZ_FINAL : public nsISupports > { > + ~nsCertAddonInfo() {} Please use an explicit "private:" section. ::: security/manager/ssl/src/nsCrypto.cpp @@ +197,5 @@ > +struct HasDangerousPublicDestructor<nsCryptoRunnable> > +{ > + static const bool value = true; > +}; > +} Please file a follow-up bug for fixing whatever forced you to use this, as this seems like a bug. ::: security/manager/ssl/src/nsNSSCertificate.h @@ +79,5 @@ > +struct HasDangerousPublicDestructor<nsNSSCertificate> > +{ > + static const bool value = true; > +}; > +} Please file a follow-up bug for fixing whatever forced you to use this, as this seems like a bug. ::: security/manager/ssl/src/nsRandomGenerator.h @@ +15,5 @@ > "@mozilla.org/security/random-generator;1" > > class nsRandomGenerator MOZ_FINAL : public nsIRandomGenerator > { > + ~nsRandomGenerator() {} Please use an explicit "private:" section.
Comment 50•10 years ago
|
||
Why did you use "protected:" in some places and "private:" in others?
Flags: needinfo?(bjacob)
Comment on attachment 8444573 [details] [diff] [review] storage/ fixes Review of attachment 8444573 [details] [diff] [review]: ----------------------------------------------------------------- r=me with these fixed: ::: storage/src/mozStorageArgValueArray.h @@ +23,5 @@ > NS_DECL_ISUPPORTS > NS_DECL_MOZISTORAGEVALUEARRAY > > private: > + ~ArgValueArray() {} Nit: Add an extra line after this? ::: storage/src/mozStorageAsyncStatementParams.h @@ +33,5 @@ > NS_DECL_MOZISTORAGESTATEMENTPARAMS > NS_DECL_NSIXPCSCRIPTABLE > > protected: > + ~AsyncStatementParams() {} Nit: Mark as virtual ::: storage/src/mozStorageError.h @@ +22,5 @@ > > Error(int aResult, const char *aMessage); > > private: > + ~Error() {} Nit: Newline after this.
Attachment #8444573 -
Flags: review?(bent.mozilla) → review+
Assignee | ||
Comment 52•10 years ago
|
||
(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #50) > Why did you use "protected:" in some places and "private:" in others? Great question :-) My approach there evolved over time :-) Initially I just put private: everywhere, thinking that I could always go back to change that to protected: for the minority of classes where that would be needed (for subclasses). Eventually it turned out that that minority case was still important enough that I was wasting a lot of time going back to change private: to protected:. That's largely because while I had a very efficient workflow (described in comment 0) to handle the initial error case of bad public destructors, anything departing from this still required more manual work (looking at detailed errors, etc). So it was very important that I found a generic approach that would work nearly everywhere. So I decided to default to protected: everywhere, except when the class was MOZ_FINAL or when it was otherwise completely obvious that it wouldn't be subclassed. I could probably have been more aggressive in deciding that a class couldn't be subclassed, but that would have required me to spend more time thinking. For example, when a class was defined in a .cpp file, I could have inspected the rest of that .cpp file to check if there was any subclass, but even that was already a large amount of work on top of my automated workflow.
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(bjacob)
Comment on attachment 8444544 [details] [diff] [review] layout/ fixes Please use explicit "private:" in TouchCaret. Same for nsComboButtenListener. In nsSelection.cpp, could you fix the indentation and bracing of ~nsAutoScrollTimer given that you're moving it? Is there a bug filed on nsGlyphTableList? (It's not hard to fix.) Please use explicit private: for MouseListener in nsMathMLmactionFrame.h Is there a bug filed on nsComputedDOMStyle? (It's probably best fixed with a friend declaration.) Same for nsDOMCSSValueList and nsROCSSPrimitiveValue? (nsComputedDOMStyle::GetImageRectString looks easily fixable; I didn't look at what the nsROCSSPrimitiveValue cases are, but they should be too.) Please use explicit private: for ~HTMLColorRule() and ~LangRule() and ~nsEmptyStyleRule and ~nsInitialStyleRule and ~nsDisableTextZoomStyleRule (followed by the currently missing explicit "public:" on the last 3). Is there a bug filed on SVGTextFrame::MutationObserver? That looks a little harder. Please use explicit private: for ~nsListScrollSmoother() and ~nsTreeColumns. r=dbaron with that
Attachment #8444544 -
Flags: review?(dbaron) → review+
Updated•10 years ago
|
Attachment #8444540 -
Flags: review?(ehsan) → review+
Updated•10 years ago
|
Attachment #8444555 -
Flags: review?(ehsan) → review+
Updated•10 years ago
|
Attachment #8444562 -
Flags: review?(ehsan) → review+
Updated•10 years ago
|
Attachment #8444585 -
Flags: review?(ehsan) → review+
Updated•10 years ago
|
Attachment #8444571 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 54•10 years ago
|
||
(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #53) > Comment on attachment 8444544 [details] [diff] [review] > layout/ fixes Thanks for the careful review. I've addressed all your comments in the patch to be landed soon, and filed these bugs about the HasDangerousPublicDestructor specializations: Bug 1029138 - Remove dangerous public destructor of nsGlyphTableList Bug 1029137 - Remove dangerous public destructor of nsComputedDOMStyle Bug 1029139 - Remove dangerous public destructor of nsDOMCSSValueList Bug 1029140 - Remove dangerous public destructor of nsROCSSPrimitiveValue Bug 1029141 - Remove dangerous public destructor of SVGTextFrame::MutationObserver To be clear I do intend to file bugs for all whitelist entries, just not necessarily at the same time as I land patches, unless explicitly asked for.
Assignee | ||
Comment 55•10 years ago
|
||
(In reply to Brian Smith (:briansmith, was :bsmith; NEEDINFO? for response) from comment #49) > Comment on attachment 8444550 [details] [diff] [review] > security/ fixes Thanks. I've addressed your review comments and filed: Bug 1029150 - Remove dangerous public destructor of nsCryptoRunnable Bug 1029151 - Remove dangerous public destructor of nsNSSCertificate Please check that I got the bugzilla components right, I wasn't too sure.
Comment 56•10 years ago
|
||
Comment on attachment 8444535 [details] [diff] [review] content/media fixes Review of attachment 8444535 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine. Olli should r+ the content/media/webspeech/* changes. ::: content/media/TextTrackCueList.h @@ +54,5 @@ > void RemoveAll(); > void GetArray(nsTArray<nsRefPtr<TextTrackCue> >& aCues); > > private: > + ~TextTrackCueList(); I'd be happy with just "~TextTrackCueList() {}" here, unless we actually need to define the dtor in the .cpp for some other reason.
Attachment #8444535 -
Flags: review?(cpearce)
Attachment #8444535 -
Flags: review?(bugs)
Attachment #8444535 -
Flags: review+
Comment 57•10 years ago
|
||
Comment on attachment 8444535 [details] [diff] [review] content/media fixes > class SpeechGrammar MOZ_FINAL : public nsISupports, > public nsWrapperCache > { >+ ~SpeechGrammar(); >+ to a private: after public: stuff > class SpeechGrammarList MOZ_FINAL : public nsISupports, > public nsWrapperCache > { >+ ~SpeechGrammarList(); >+ ditto >+++ b/content/media/webspeech/recognition/SpeechRecognition.h >@@ -180,32 +180,34 @@ private: > public: > NS_DECL_ISUPPORTS > NS_DECL_NSIDOMGETUSERMEDIASUCCESSCALLBACK > > GetUserMediaSuccessCallback(SpeechRecognition* aRecognition) > : mRecognition(aRecognition) > {} > >+ protected: > virtual ~GetUserMediaSuccessCallback() {} Not sure why this needs to be protected, and not private > >+ protected: > virtual ~GetUserMediaErrorCallback() {} ditto
Attachment #8444535 -
Flags: review?(bugs) → review+
Comment 58•10 years ago
|
||
Comment on attachment 8444514 [details] [diff] [review] netwerk/ fixes Review of attachment 8444514 [details] [diff] [review]: ----------------------------------------------------------------- thanks - better.
Attachment #8444514 -
Flags: review?(mcmanus) → review+
Updated•10 years ago
|
Attachment #8444550 -
Flags: review?(brian) → review+
Comment 59•10 years ago
|
||
(In reply to ben turner from comment #51) > (From update of attachment 8444573 [details] [diff] [review]) > ::: storage/src/mozStorageAsyncStatementParams.h > @@ +33,5 @@ > > NS_DECL_MOZISTORAGESTATEMENTPARAMS > > NS_DECL_NSIXPCSCRIPTABLE > > > > protected: > > + ~AsyncStatementParams() {} > > Nit: Mark as virtual Why? The class is already final, so it can't be subclassed. (In reply to Olli Pettay from comment #57) > (From update of attachment 8444535 [details] [diff] [review]) > >+ protected: > > virtual ~GetUserMediaSuccessCallback() {} > Not sure why this needs to be protected, and not private > > > > >+ protected: > > virtual ~GetUserMediaErrorCallback() {} > ditto Out of interest, does it make sense to have a private virtual destructor?
Comment 60•10 years ago
|
||
Comment on attachment 8444582 [details] [diff] [review] media/mtransport/ fixes Review of attachment 8444582 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. r+, assuming the nits are addressed. ::: media/mtransport/nr_socket_prsock.h @@ +127,5 @@ > }; > > class NrSocket : public NrSocketBase, > public nsASocketHandler { > +private: private at end of the class, please. ::: media/mtransport/stun_udp_socket_filter.h @@ +11,5 @@ > { 0xa3, 0x4e, 0x62, 0xd9, 0xe4, 0xc9, 0xf9, 0x93 } }; > > class nsStunUDPSocketFilterHandler : public nsIUDPSocketFilterHandler { > +private: > + virtual ~nsStunUDPSocketFilterHandler() {} private: at end of the class, please. ::: media/mtransport/transportflow.h @@ +138,5 @@ > ScopedDeletePtr<std::deque<TransportLayer *> > layers_; > nsCOMPtr<nsIEventTarget> target_; > }; > > +template<> Please include a bug number here for the bug that tracks fixing this class.
Attachment #8444582 -
Flags: review?(adam) → review+
Assignee | ||
Comment 61•10 years ago
|
||
more landing more better remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/2d4b9bc5d6f5 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/438751141e7c remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/97477a6da3b1 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/461ad7d4f1ef remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/12985d7bd06a remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/fdb81eed1397 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c928b9f75e7f remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/8f8f2b86ae01 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/fc66d632e573
Attachment #8444545 -
Flags: review?(roc) → review+
Updated•10 years ago
|
Attachment #8444567 -
Flags: review?(enndeakin) → review+
Updated•10 years ago
|
Attachment #8444536 -
Flags: review?(cam) → review+
Assignee | ||
Comment 62•10 years ago
|
||
(In reply to Chris Pearce (:cpearce) from comment #56) > ::: content/media/TextTrackCueList.h > @@ +54,5 @@ > > void RemoveAll(); > > void GetArray(nsTArray<nsRefPtr<TextTrackCue> >& aCues); > > > > private: > > + ~TextTrackCueList(); > > I'd be happy with just "~TextTrackCueList() {}" here, unless we actually > need to define the dtor in the .cpp for some other reason. IIRC we do need to; I started with what you suggest and got compilation errors as TextTrackCue is only forward-declared above and is thus an incomplete type, so the nsTArray< nsRefPtr<TextTrackCue> > mList; , which requires a definition of TextTrackCue to compile the destructor, failed to compile.
Assignee | ||
Comment 63•10 years ago
|
||
(In reply to Adam Roach [:abr] from comment #60) > Comment on attachment 8444582 [details] [diff] [review] > media/mtransport/ fixes Thanks for the review; addressed your comments in the patch to be landed soon; filed Bug 1029478 - Remove dangerous public destructor of TransportFlow and referenced it in a code comment.
Assignee | ||
Comment 64•10 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #57) > Comment on attachment 8444535 [details] [diff] [review] > content/media fixes Thanks, applied your comments. > >+ protected: > > virtual ~GetUserMediaSuccessCallback() {} > Not sure why this needs to be protected, and not private I just defaulted to protected: for non-MOZ_FINAL classes. Changed to private:.
Comment 65•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2c62d4b7b055 https://hg.mozilla.org/mozilla-central/rev/4976dc6ee72a https://hg.mozilla.org/mozilla-central/rev/3d6b2a02b254 https://hg.mozilla.org/mozilla-central/rev/477fcc7c9f60 https://hg.mozilla.org/mozilla-central/rev/4ccc78ad6d2a https://hg.mozilla.org/mozilla-central/rev/31aa508e8c6d https://hg.mozilla.org/mozilla-central/rev/1679e9d569c3 https://hg.mozilla.org/mozilla-central/rev/967e19ae406c https://hg.mozilla.org/mozilla-central/rev/72d774431043 https://hg.mozilla.org/mozilla-central/rev/2d4b9bc5d6f5 https://hg.mozilla.org/mozilla-central/rev/438751141e7c https://hg.mozilla.org/mozilla-central/rev/97477a6da3b1 https://hg.mozilla.org/mozilla-central/rev/461ad7d4f1ef https://hg.mozilla.org/mozilla-central/rev/12985d7bd06a https://hg.mozilla.org/mozilla-central/rev/fdb81eed1397 https://hg.mozilla.org/mozilla-central/rev/c928b9f75e7f https://hg.mozilla.org/mozilla-central/rev/8f8f2b86ae01 https://hg.mozilla.org/mozilla-central/rev/fc66d632e573
Assignee | ||
Comment 66•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/236e0f562470 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ef6cb0f76224 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/831eb6e6ce07 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/6a0a566bc003 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/20428bf68d89 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/5307410e3a3b
Comment on attachment 8444538 [details] [diff] [review] zz-content Review of attachment 8444538 [details] [diff] [review]: ----------------------------------------------------------------- I have no interest in actually reviewing this.
Attachment #8444538 -
Flags: review?(ehsan) → review+
Updated•10 years ago
|
Attachment #8444581 -
Flags: review?(rjesup) → review+
https://hg.mozilla.org/mozilla-central/rev/236e0f562470 https://hg.mozilla.org/mozilla-central/rev/ef6cb0f76224 https://hg.mozilla.org/mozilla-central/rev/831eb6e6ce07 https://hg.mozilla.org/mozilla-central/rev/6a0a566bc003 https://hg.mozilla.org/mozilla-central/rev/20428bf68d89 https://hg.mozilla.org/mozilla-central/rev/5307410e3a3b
Assignee | ||
Comment 70•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/c055d3168355 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e12602a71d8f
Comment 71•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c055d3168355 https://hg.mozilla.org/mozilla-central/rev/e12602a71d8f
Updated•10 years ago
|
Attachment #8444534 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 72•10 years ago
|
||
Comment on attachment 8444557 [details] [diff] [review] parser/ fixes Reassigning review because Henri is currently away.
Attachment #8444557 -
Flags: review?(hsivonen) → review?(wchen)
Assignee | ||
Comment 73•10 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/16244b1384ab http://hg.mozilla.org/integration/mozilla-inbound/rev/d9d6555b5a1b
Updated•10 years ago
|
Attachment #8444557 -
Flags: review?(wchen) → review+
Comment 74•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/16244b1384ab https://hg.mozilla.org/mozilla-central/rev/d9d6555b5a1b
Assignee | ||
Comment 75•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/a588fb7df4a3
Updated•10 years ago
|
Attachment #8444506 -
Flags: review?(benjamin) → review+
Updated•10 years ago
|
Attachment #8444541 -
Flags: review?(benjamin) → review+
Updated•10 years ago
|
Attachment #8444551 -
Flags: review?(benjamin) → review+
Updated•10 years ago
|
Attachment #8444578 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 77•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/5942ad3859b8 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/d5fae80054de remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/19a19833f1d6 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/13a8bae671ca
I had to back this out in https://hg.mozilla.org/integration/mozilla-inbound/rev/9b33a6d30532 for build bustage on every platform: https://tbpl.mozilla.org/php/getParsedLog.php?id=42800030&tree=Mozilla-Inbound
Flags: needinfo?(bjacob)
Assignee | ||
Comment 79•10 years ago
|
||
Was due to unintentionally commenting out a NS_IMPL_ line... my text editor has a keyboard shortcut for commenting out the current line, that seems to be a bad idea.
Flags: needinfo?(bjacob)
Assignee | ||
Comment 80•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f537dd9a7986 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/682c3cd67254 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/80a31ed2b975 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/bb961917a75c
Comment 81•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f537dd9a7986 https://hg.mozilla.org/mozilla-central/rev/682c3cd67254 https://hg.mozilla.org/mozilla-central/rev/80a31ed2b975 https://hg.mozilla.org/mozilla-central/rev/bb961917a75c
Assignee | ||
Comment 82•10 years ago
|
||
That's basically how fast we add new bad public destructors in the tree :-)
Attachment #8450899 -
Flags: review?(ehsan)
Assignee | ||
Comment 83•10 years ago
|
||
Attachment #8450903 -
Flags: review?(mstange)
Assignee | ||
Comment 84•10 years ago
|
||
Turns out that part of my accessible/ fixes were to a file in my object directory. This is the corresponding fixes to the code generator in the tree.
Attachment #8450904 -
Flags: review?(trev.saunders)
Assignee | ||
Comment 85•10 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=bc618df71722
Assignee | ||
Comment 86•10 years ago
|
||
Err, https://tbpl.mozilla.org/?tree=Try&rev=048902a2553c
Updated•10 years ago
|
Attachment #8450903 -
Flags: review?(mstange) → review+
Updated•10 years ago
|
Attachment #8450899 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 87•10 years ago
|
||
Sorry, there were further fixes needed. This is green on tryserver now.
Attachment #8450903 -
Attachment is obsolete: true
Attachment #8451300 -
Flags: review?(mstange)
Assignee | ||
Comment 88•10 years ago
|
||
Found by tryserver MacOSX b2g desktop build (uses Clang).
Attachment #8451301 -
Flags: review?(jib)
Assignee | ||
Comment 89•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b6e4d0527c3
Assignee | ||
Updated•10 years ago
|
Updated•10 years ago
|
Attachment #8451300 -
Flags: review?(mstange) → review+
Assignee | ||
Comment 90•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f27f634ed58
Comment 91•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8b6e4d0527c3 https://hg.mozilla.org/mozilla-central/rev/5f27f634ed58
Updated•10 years ago
|
Attachment #8450904 -
Flags: review?(trev.saunders) → review+
Updated•10 years ago
|
Attachment #8451301 -
Flags: review?(jib) → review+
Assignee | ||
Comment 92•10 years ago
|
||
...And the last patches are landed, ending with the static_assert that ensures we don't regress this. remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/4d276769e9dd remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/eac85d5073ef remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/261d818388d2
Whiteboard: [leave open]
Comment 93•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4d276769e9dd https://hg.mozilla.org/mozilla-central/rev/eac85d5073ef https://hg.mozilla.org/mozilla-central/rev/261d818388d2
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Assignee | ||
Comment 94•10 years ago
|
||
Work continues on bug 1035394.
You need to log in
before you can comment on or make changes to this bug.
Description
•