Closed Bug 1036367 Opened 10 years ago Closed 10 years ago

Fix dangerous public destructors in purple/ and im/

Categories

(Chat Core :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

Details

Attachments

(4 files, 2 obsolete files)

Bug 1028588 added an assertion for public destructors.
Attached patch destructors.diff (obsolete) — Splinter Review
Is there a reasonable way to avoid the delete purpleTimeout calls?
Attachment #8453012 - Flags: review?(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.
Attachment #8453080 - Flags: review?(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:"
Attachment #8453080 - Flags: review?(florian) → review+
Attachment #8453012 - Flags: review?(florian)
Pushed with nits fixed. I also verified the purpleTimeout destructor was still getting called after the change to nsCOMArray.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.6
Attachment #8453304 - Flags: review?(aleth)
Attachment #8453304 - Flags: review?(aleth) → review+
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.
Attachment #8453012 - Attachment is obsolete: true
(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
(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>>.)
This fixes the issue during compiling. Should be an easy and quick review for whoever gets to it first :)
Attachment #8454675 - Flags: review?(florian)
Attachment #8454675 - Flags: review?(clokep)
Attachment #8454675 - Flags: review?(aleth)
(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 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.)
(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 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!)
Attachment #8454675 - Flags: review?(florian)
Attachment #8454675 - Flags: review?(clokep)
Attachment #8454675 - Flags: review?(aleth)
Attachment #8454675 - Flags: review-
Removed NS_ADDREF.
Attachment #8454675 - Attachment is obsolete: true
Attachment #8454698 - Flags: review?(aleth)
Comment on attachment 8454698 [details] [diff] [review]
Fix issue in TrayIconImpl (inside im/) (v2)

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

Thanks!
Attachment #8454698 - Flags: review?(aleth) → review+
Attachment #8454883 - Flags: review?(florian)
Attachment #8454883 - Flags: review?(florian) → review+
Summary: Fix dangerous public destructors in purple/ → Fix dangerous public destructors in purple/ and im/
You need to log in before you can comment on or make changes to this bug.