Fix dangerous public destructors in purple/ and im/

RESOLVED FIXED in 1.6

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: aleth, Assigned: aleth)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

Assignee

Description

5 years ago
Bug 1028588 added an assertion for public destructors.
Assignee

Comment 1

5 years ago
Posted 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.
Assignee

Comment 3

5 years ago
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+
Assignee

Updated

5 years ago
Attachment #8453012 - Flags: review?(florian)
Assignee

Comment 5

5 years ago
Pushed with nits fixed. I also verified the purpleTimeout destructor was still getting called after the change to nsCOMArray.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Assignee

Updated

5 years ago
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.)
Assignee

Comment 15

5 years ago
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)
Assignee

Comment 17

5 years ago
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+
Assignee

Comment 18

5 years ago
Attachment #8454883 - Flags: review?(florian)
Attachment #8454883 - Flags: review?(florian) → review+
Assignee

Updated

5 years ago
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.