333704 - We leak key in txNodeSorter::addSortElement

Status: RESOLVED FIXED

(Core :: XSLT, defect)

Description

[Jonathan Watt [:jwatt]]

We really can't use the NS_ENSURE_ macros in txNodeSorter::addSortElement because we'll leak the SortKey.

Comment 1

[Axel Hecht]

We should really stick to early returns, but use an nsAutoPtr for the key up
to the point where we actually pass the ownership over to mSortKeys.

Comment 2

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v1

Patch v1. Swaps the |NS_ENSURE_SUCCESS| in return for early returns.

Comment 3

[Peter Van der Beken [:peterv]]

Comment on attachment 242432 [details] [diff] [review]
Patch v1

Make key an nsAutoPtr instead (use forget when adding it to mSortKeys).

Comment 4

[Ryan Jones-Ward [:sciguyryan]]

(In reply to comment #3)
> (From update of attachment 242432 [details] [diff] [review] [edit])
> Make key an nsAutoPtr instead (use forget when adding it to mSortKeys).
> 

I was under the impression that XSLT could be built standalone. Wouldn't a reliance on XPCOM for nsAutoPtr break that?

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

the standalone engine relies on XPCOM, just not the rest of gecko.

Comment 6

[Ryan Jones-Ward [:sciguyryan]]

(In reply to comment #5)
> the standalone engine relies on XPCOM, just not the rest of gecko.
> 

Ah. I see. In that case I'll make a new patch using nsAutoPtr :)

Comment 7

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v2

Patch v2

* Return |rv| from a method call where failure is detected.
* Switch too |nsAutoPtr|.
* Favour early return rather than use of |NS_ENSURE_TRUE|.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 242511 [details] [diff] [review]
Patch v2

>-        NS_ENSURE_SUCCESS(rv, rv);
>+        if (NS_FAILED(rv)) {
>+            return rv;
>+        }

No, don't make this change. The two things mean the same thing, but your version bloats the source more. In release builds the NS_ENSURE_SUCCESS macro expands to the exact same thing.

Comment 9

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v3

(In reply to comment #8)
> No, don't make this change. The two things mean the same thing, but your
> version bloats the source more. In release builds the NS_ENSURE_SUCCESS macro
> expands to the exact same thing.
> 

Updated the patch, no removal of the |NS_ENSURE_TRUE| calls.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 242520 [details] [diff] [review]
Patch v3

>-    mSortKeys.add(key);
>+    // mSortKeys owns key now.
>+    mSortKeys.add(key.forget());

This is wrong in the current code too, but please fix this call too since it too can cause a leak. mSortKeys.add can fail (it returns an nsresult), so only call key.forget() once you've checked the return value of the add-call.

Comment 11

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v3.1

Patched to Jonas Sicking - appear to have missed that one sorry.

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 242531 [details] [diff] [review]
Patch v3.1

>Index: txNodeSorter.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/xslt/src/xslt/txNodeSorter.cpp,v
>retrieving revision 1.21
>diff -u -8 -p -r1.21 txNodeSorter.cpp
>--- txNodeSorter.cpp	1 Apr 2006 02:12:15 -0000	1.21
>+++ txNodeSorter.cpp	17 Oct 2006 18:33:49 -0000
>@@ -65,17 +65,17 @@ txNodeSorter::~txNodeSorter()
>     }
> }
> 
> nsresult
> txNodeSorter::addSortElement(Expr* aSelectExpr, Expr* aLangExpr,
>                              Expr* aDataTypeExpr, Expr* aOrderExpr,
>                              Expr* aCaseOrderExpr, txIEvalContext* aContext)
> {
>-    SortKey* key = new SortKey;
>+    nsAutoPtr<SortKey> key(new SortKey);

Nit: I prefer this to be |nsAutoPtr<SortKey> key = new SortKey;| since that's IMHO more readable.

>-    mSortKeys.add(key);
>+    // mSortKeys owns key now. 
>+    nsresult success = mSortKeys.add(key.get());
>     mNKeys++;
> 
>+    if (NS_SUCCEEDED(success)) {
>+        key.forget();
>+    }
>+
>     return NS_OK;

This looses the error code if one is return, i.e. you want that error to be returned and not just forgotten. So simply do what all other callsites do in this function and use

rv = ...
NS_ENSURE_SUCCESS(rv, rv);

Comment 13

[Ryan Jones-Ward [:sciguyryan]]

Attachment: patch v3.2

Addressing nits!

Comment 14

[Peter Van der Beken [:peterv]]

> >+    nsAutoPtr<SortKey> key(new SortKey);
> 
> Nit: I prefer this to be |nsAutoPtr<SortKey> key = new SortKey;| since that's
> IMHO more readable.

I prefer this too, but I think some compilers won't accept it. So it should be |nsAutoPtr<SortKey> key(new SortKey);| after all.

Comment 15

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v3.3

Changed back too |nsAutoPtr<SortKey> key(new SortKey);| Peter's comment :)

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

should you really increase mNKeys when add failed?

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 242540 [details] [diff] [review]
Patch v3.3

>-    mSortKeys.add(key);
>+    // mSortKeys owns key now. 
>+    nsresult success = mSortKeys.add(key.get());
>     mNKeys++;

You never check the |success| variable. Why not use |rv|? Additionally you should check if the call succeeded before doing anything else. And no need to call .get() expclicitly, that conversion is done automatically.

Comment 18

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v3.4

Hopefully I didn't make any more mistakes!

Moved |mNKeys++| after the |NS_ENSURE_SUCCESS| (stupid error).
Changed |success| too |rv| which is now checked (another stupid error).
Removed the explicit use of |key.get()| in favour of auto-casting.

Comment 19

[Ryan Jones-Ward [:sciguyryan]]

Comment on attachment 242546 [details] [diff] [review]
Patch v3.4

Wrong one sorry.

Comment 20

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Correct patch!

Uploaded the wrong patch last time. Sorry for the bugspam.

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 242548 [details] [diff] [review]
Correct patch!

>-    mSortKeys.add(key);
>+    // mSortKeys owns key now. 
>+    rv = mSortKeys.add(key);
>+
>+    NS_ENSURE_SUCCESS(rv, rv);

Nit: Put the empty line after the NS_ENSURE_SUCCESS, as the ENSURE belongs together with the call.

>     mNKeys++;
>+    key.forget();

Nit: Put the call to forget() before the call to mNKeys++ so that we don't accidentally put more code in later that can cause other early returns.

r/sr=sicking with that.

Comment 22

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Nits-be-gone

Final patch to Jonas' nits! Thanks for all the reviews and comments (and your patience with a n00b).

Comment 23

[:Gavin Sharp [email: [e-mail]]]

mozilla/content/xslt/src/xslt/txNodeSorter.cpp 	1.22