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)

Other Branch
defect
Not set
normal

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.
Thanks to khuey for noticing this.
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!
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)
Attached patch xpcom/ fixesSplinter Review
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+
Attachment #8444506 - Flags: review?(benjamin)
(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.
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)
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
Attached patch netwerk/ fixesSplinter Review
Attachment #8444514 - Flags: review?(mcmanus)
Attached patch gfx/ fixesSplinter Review
Attachment #8444515 - Flags: review?(jmuizelaar)
Attached patch image/ fixesSplinter Review
Attachment #8444517 - Flags: review?(jmuizelaar)
Attached patch jsd/ fixesSplinter Review
Attachment #8444521 - Flags: review?(sphink)
Attached patch XPConnect fixesSplinter Review
Attachment #8444523 - Flags: review?(bobbyholley)
The Codegen.py part of this patch makes the code generator not generate public destructors.
Attachment #8444525 - Flags: review?(bzbarsky)
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 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+
(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.
Attachment #8444534 - Flags: review?(jmuizelaar)
Attachment #8444535 - Flags: review?(cpearce)
Attachment #8444536 - Flags: review?(cam)
Attached patch zz-contentSplinter Review
...and the winner of the uncategorized content/ bulk is again... Ehsan!!
Attachment #8444538 - Flags: review?(ehsan)
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)
Attached patch embedding/ fixesSplinter Review
Attachment #8444541 - Flags: review?(benjamin)
Attached patch docshell/ fixesSplinter Review
Attachment #8444543 - Flags: review?(bugs)
Attached patch layout/ fixesSplinter Review
Attachment #8444544 - Flags: review?(dbaron)
Attached patch widget/ fixesSplinter Review
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)
Attached patch uriloader/ fixesSplinter Review
Attachment #8444549 - Flags: review?(bugs)
Attached patch security/ fixesSplinter Review
Attachment #8444550 - Flags: review?(brian)
Attached patch rdf/ fixesSplinter Review
Attachment #8444551 - Flags: review?(benjamin)
Attachment #8444523 - Flags: review?(bobbyholley) → review+
Attachment #8444515 - Flags: review?(jmuizelaar) → review+
Attachment #8444552 - Flags: review?(trev.saunders)
Attached patch extensions/ fixes (obsolete) — Splinter Review
Who is reviewing this toplevel dir? hg log says: Ehsan!
Attachment #8444553 - Flags: review?(ehsan)
Attachment #8444517 - Flags: review?(jmuizelaar) → review+
Attachment #8444553 - Attachment is obsolete: true
Attachment #8444553 - Flags: review?(ehsan)
Attachment #8444555 - Flags: review?(ehsan)
Attached patch parser/ fixesSplinter Review
Attachment #8444557 - Flags: review?(hsivonen)
Attached patch toolkit/ fixesSplinter Review
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)
Attached patch xpfe/ fixesSplinter Review
Attachment #8444567 - Flags: review?(enndeakin)
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+
Attached patch intl/ fixesSplinter Review
Attachment #8444571 - Flags: review?(ehsan)
Attached patch storage/ fixesSplinter Review
Attachment #8444573 - Flags: review?(bent.mozilla)
Attached patch ipc/ fixSplinter Review
Attachment #8444574 - Flags: review?(bent.mozilla)
Attached patch libjar/ fixesSplinter Review
Attachment #8444577 - Flags: review?(aklotz)
Attached patch libpref/ fixesSplinter Review
Attachment #8444578 - Flags: review?(benjamin)
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+
Attachment #8444581 - Flags: review?(rjesup)
Attachment #8444582 - Flags: review?(adam)
Attachment #8444549 - Flags: review?(bugs) → review+
Attachment #8444585 - Flags: review?(ehsan)
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+
Attachment #8444528 - Flags: review?(ehsan) → review+
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 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+
Attachment #8444574 - Flags: review?(bent.mozilla) → review+
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.
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+
(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.
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+
Attachment #8444540 - Flags: review?(ehsan) → review+
Attachment #8444555 - Flags: review?(ehsan) → review+
Attachment #8444562 - Flags: review?(ehsan) → review+
Attachment #8444585 - Flags: review?(ehsan) → review+
Attachment #8444571 - Flags: review?(ehsan) → review+
Blocks: 1029137
Blocks: 1029138
Blocks: 1029139
Blocks: 1029140
Blocks: 1029141
(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.
Blocks: 1029150
Blocks: 1029151
(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 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 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 on attachment 8444514 [details] [diff] [review]
netwerk/ fixes

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

thanks - better.
Attachment #8444514 - Flags: review?(mcmanus) → review+
Attachment #8444550 - Flags: review?(brian) → review+
(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 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+
Attachment #8444567 - Flags: review?(enndeakin) → review+
Attachment #8444536 - Flags: review?(cam) → review+
(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.
Blocks: 1029478
(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.
(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 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+
Attachment #8444581 - Flags: review?(rjesup) → review+
Attachment #8444534 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8444557 [details] [diff] [review]
parser/ fixes

Reassigning review because Henri is currently away.
Attachment #8444557 - Flags: review?(hsivonen) → review?(wchen)
Attachment #8444557 - Flags: review?(wchen) → review+
Attachment #8444506 - Flags: review?(benjamin) → review+
Attachment #8444541 - Flags: review?(benjamin) → review+
Attachment #8444551 - Flags: review?(benjamin) → review+
Attachment #8444578 - Flags: review?(benjamin) → review+
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)
That's basically how fast we add new bad public destructors in the tree :-)
Attachment #8450899 - Flags: review?(ehsan)
Attached patch Mac-specific fixes (obsolete) — Splinter Review
Attachment #8450903 - Flags: review?(mstange)
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)
Attachment #8450903 - Flags: review?(mstange) → review+
Attachment #8450899 - Flags: review?(ehsan) → review+
Sorry, there were further fixes needed. This is green on tryserver now.
Attachment #8450903 - Attachment is obsolete: true
Attachment #8451300 - Flags: review?(mstange)
Found by tryserver MacOSX b2g desktop build (uses Clang).
Attachment #8451301 - Flags: review?(jib)
Attachment #8451300 - Flags: review?(mstange) → review+
Attachment #8450904 - Flags: review?(trev.saunders) → review+
Attachment #8451301 - Flags: review?(jib) → review+
...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]
Blocks: 1035394
Depends on: 1035747
Work continues on bug 1035394.
Depends on: 1060787
Depends on: 1060984
You need to log in before you can comment on or make changes to this bug.