1036367 - Fix dangerous public destructors in purple/ and im/

Status: RESOLVED FIXED

(Chat Core :: General, defect)

Description

[aleth [:aleth]]

Bug 1028588 added an assertion for public destructors.

Comment 1

[aleth [:aleth]]

Attachment: destructors.diff

Is there a reasonable way to avoid the delete purpleTimeout calls?

Comment 2

[Florian Quèze [:florian]]

(In reply to aleth [:aleth] from comment #1)
> Created attachment 8453012 [details] [diff] [review]
> destructors.diff
> 
> Is there a reasonable way to avoid the delete purpleTimeout calls?

You could try replacing the nsTArray with a refcounted nsCOMArray, that would remove a reference (and call the private destructor) automatically whenever an element is removed from the array.

Comment 3

[aleth [:aleth]]

Attachment: destructors.diff nsCOMArray version

Comment 4

[Florian Quèze [:florian]]

Comment on attachment 8453080 [details] [diff] [review]
destructors.diff nsCOMArray version

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

Comments are coding style only; thanks for looking into this! :-)

::: purplexpcom/src/purpleDebug.h
@@ +29,5 @@
>  
>   private:
>    static nsresult init();
>    purpleDebug(); // can only be called by init()
> +  ~purpleDebug() {};

No ; here.

::: purplexpcom/src/purpleGetText.h
@@ +31,5 @@
>  
>   private:
>    // can only be called by init();
>    purpleGetText();
> +  ~purpleGetText() {};

No ;

::: purplexpcom/src/purpleInit.cpp
@@ +126,5 @@
>      NS_DECL_NSIOBSERVER
>      prefsObserver(void *aCallback) : callback(aCallback) {}
>  
>    private:
> +    ~prefsObserver() {};

No ;

::: purplexpcom/src/purpleInitAccounts.cpp
@@ +154,5 @@
>      void Cancel();
>  
>  
>    private:
> +    ~purpleAuthorizationRequest() {};

No ;

::: purplexpcom/src/purpleSockets.cpp
@@ +32,5 @@
>  {
>   public:
>    NS_DECL_ISUPPORTS
>    NS_DECL_NSIOBSERVER
> + private:

nit: please add an empty line before this.

@@ +33,5 @@
>   public:
>    NS_DECL_ISUPPORTS
>    NS_DECL_NSIOBSERVER
> + private:
> +  ~purpleSocketNetworkStateObserver() {};

No ;

::: purplexpcom/src/purpleTimer.h
@@ +19,5 @@
>    PRUint32 mId;
>    nsCOMPtr<nsITimer> mTimer;
>    GSourceFunc mFunction;
>    gpointer mData;
> + private: 

Trailing whitespace. Also, I think we usually have an empty line before "private:"

Comment 5

[aleth [:aleth]]

Pushed with nits fixed. I also verified the purpleTimeout destructor was still getting called after the change to nsCOMArray.

Comment 6

[aleth [:aleth]]

http://hg.mozilla.org/users/florian_queze.net/purple/rev/b0681d7c96db

Comment 7

[Patrick Cloke [:clokep]]

Attachment: fix-static-prpls.diff

Comment 8

[Patrick Cloke [:clokep]]

https://hg.mozilla.org/users/florian_queze.net/purple/rev/890a12044644

And as Florian correct me on IRC: this is for dynamic prpls, not static.

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Florian Quèze [:florian] [:flo] from comment #2)
> (In reply to aleth [:aleth] from comment #1)
> > Created attachment 8453012 [details] [diff] [review]
> > destructors.diff
> > 
> > Is there a reasonable way to avoid the delete purpleTimeout calls?
> 
> You could try replacing the nsTArray with a refcounted nsCOMArray, that
> would remove a reference (and call the private destructor) automatically
> whenever an element is removed from the array.

I don't think this is what you want.  nsCOMPtr / nsCOMArray are generally used for interfaces; nsRefPtr is used for concrete classes.  There is no nsRefPtrArray; the equivalent is just nsTArray<nsRefPtr<Whatever> >.

So, this probably wants to use nsTArrray<nsRefPtr<purpleTimeout> >.  (It's a bit ugly, but it's a well-established pattern, per http://mxr.mozilla.org/mozilla-central/search?string=nsTArray%3CnsRefPtr%3C

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #9)
> So, this probably wants to use nsTArrray<nsRefPtr<purpleTimeout> >
(This space is important & needs to be there----------------------^
 Otherwise you'll get an arcane compiler error about operator>>.)

Comment 11

[Saurabh Anand [:sawrubh]]

Attachment: Fix issue in TrayIconImpl (inside im/) (v1)

This fixes the issue during compiling. Should be an easy and quick review for whoever gets to it first :)

Comment 12

[Florian Quèze [:florian]]

(In reply to Daniel Holbert [:dholbert] from comment #9)
> (In reply to Florian Quèze [:florian] [:flo] from comment #2)
> > (In reply to aleth [:aleth] from comment #1)
> > > Created attachment 8453012 [details] [diff] [review]
> > > destructors.diff
> > > 
> > > Is there a reasonable way to avoid the delete purpleTimeout calls?
> > 
> > You could try replacing the nsTArray with a refcounted nsCOMArray, that
> > would remove a reference (and call the private destructor) automatically
> > whenever an element is removed from the array.
> 
> I don't think this is what you want.  nsCOMPtr / nsCOMArray are generally
> used for interfaces; nsRefPtr is used for concrete classes.  There is no
> nsRefPtrArray; the equivalent is just nsTArray<nsRefPtr<Whatever> >.
> 
> So, this probably wants to use nsTArrray<nsRefPtr<purpleTimeout> >.  (It's a
> bit ugly, but it's a well-established pattern, per
> http://mxr.mozilla.org/mozilla-central/search?string=nsTArray%3CnsRefPtr%3C

Thanks!

Comment 13

[Daniel Holbert [:dholbert]]

Comment on attachment 8454675 [details] [diff] [review]
Fix issue in TrayIconImpl (inside im/) (v1)

>-  TrayIconImpl *icon = new TrayIconImpl(this);
>+  nsRefPtr<TrayIconImpl> icon = new TrayIconImpl(this);
>+  if (NS_SUCCEEDED(rv)) {
>     mIcons.AppendObject(icon);
>     if (aResult) {
>-      *aResult = icon;
>+      icon.forget(aResult);
>       NS_ADDREF(*aResult);

This NS_ADDREF ^ should be removed.  (That AddRef is now performed under-the-hood by the nsRefPtr. It normally would also Release to cancel-out the addref (and delete the thing), except that you're calling forget() which prevents it from decrementing. So, the nsRefPtr usage combined with the forget() is equivalent to the explicit NS_ADDREF.)

Comment 14

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #13)
> It normally would also Release to cancel-out
> the addref (and delete the thing), except that you're calling forget()

(clarifying side note: technically in this case, the release() wouldn't *actually* delete the thing (not immediately), because mIcons.AppendObject() does another addref under-the-hood, which will keep the icon alive for as long as mIcons is holding onto it.)

Comment 15

[aleth [:aleth]]

Comment on attachment 8454675 [details] [diff] [review]
Fix issue in TrayIconImpl (inside im/) (v1)

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

See the great feedback from dholbert (Thanks!)

Comment 16

[Saurabh Anand [:sawrubh]]

Attachment: Fix issue in TrayIconImpl (inside im/) (v2)

Removed NS_ADDREF.

Comment 17

[aleth [:aleth]]

Comment on attachment 8454698 [details] [diff] [review]
Fix issue in TrayIconImpl (inside im/) (v2)

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

Thanks!

Comment 18

[aleth [:aleth]]

Attachment: Followup to fix array class

Comment 19

[aleth [:aleth]]

http://hg.mozilla.org/users/florian_queze.net/purple/rev/956f01125ef6

Comment 20

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/589e0d46eeba