Closed Bug 1142820 Opened 5 years ago Closed 5 years ago

js/src/jsapi-tests/testPersistentRooted.cpp has leaks

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: erahm, Assigned: jj, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: [CID 1286374][CID 1286451][CID 1286509][lang=c++][good first bug])

Attachments

(1 file, 2 obsolete files)

Note: this is only in test code, set priority as you see fit.

Coverity indicates that there are several potential leaks in testPersistentRooted.cpp if |CHECK|s fail.

- |test_PersistentRootedCopy| can leak |kennel| and |newKennel|
- |test_PersistentRootedAssign| can leak |kennel| and |kennel2|
- |test_PersistentRooted| can leak |kennel|

[1] https://hg.mozilla.org/mozilla-central/annotate/0190a1d17294/js/src/jsapi-tests/testPersistentRooted.cpp#l115
[2] https://hg.mozilla.org/mozilla-central/annotate/0190a1d17294/js/src/jsapi-tests/testPersistentRooted.cpp#l145
[3] https://hg.mozilla.org/mozilla-central/annotate/0190a1d17294/js/src/jsapi-tests/testPersistentRooted.cpp#l79
[3]
Mentor: erahm
Whiteboard: [CID 1286374][CID 1286451][CID 1286509] → [CID 1286374][CID 1286451][CID 1286509][lang=c++][good first bug]
But there is a thing with Allocate function which return a char* type pointer which cannot be assigned to UniquePtr
(In reply to Jinank Jain from comment #1)
> But there is a thing with Allocate function which return a char* type
> pointer which cannot be assigned to UniquePtr

In these cases |Allocate| returns a |Kennel*|. You can use:
> UniquePtr<Kennel> kennel = Allocate(cx);
Attached patch BugNo1142820.diff (obsolete) — Splinter Review
That thing generated errors so I created a wrapper
Comment on attachment 8605517 [details] [diff] [review]
BugNo1142820.diff

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

This is a good start, but you shouldn't need to allocate a new Kennel, and then assign over it (that will actually cause a new leak). I've described what to do in the first function, the same applies to the rest.

::: js/src/jsapi-tests/testPersistentRooted.cpp
@@ +83,5 @@
>      BarkWhenTracedClass::reset();
>  
> +    UniquePtr<Kennel> kennel_wrapper(new Kennel);
> +    Kennel* kennel = kennel_wrapper.get();
> +    kennel = Allocate(cx);

Ah sorry! |UniquePtr| is a bit picky. The proper form is:
  UniquePtr<Kennel> kennel(Allocate(cx));

@@ -86,5 @@
>  
>      // GC should be able to find our barker.
>      CHECK(GCFinalizesNBarkers(cx, 0));
>  
> -    delete(kennel);

This was actually intentional to check if the next part works. To do the same w/ a UniquePtr all you have to do is clear the reference:

  kennel = nullptr;
Attachment #8605517 - Flags: feedback+
Attached patch BugNo1142820.diff (obsolete) — Splinter Review
I have made the changes as suggested by you :)
Attachment #8605517 - Attachment is obsolete: true
Comment on attachment 8605548 [details] [diff] [review]
BugNo1142820.diff

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

So this certainly fixes the potential leaks and builds, but now the tests fail. Jim, do you think you could help guide us through the rest?
Attachment #8605548 - Flags: feedback?(jimb)
I did not get that build was successful, then which tests failed ?
Comment on attachment 8605548 [details] [diff] [review]
BugNo1142820.diff

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

::: js/src/jsapi-tests/testPersistentRooted.cpp
@@ +123,5 @@
>  
>      CHECK(GCFinalizesNBarkers(cx, 0));
>  
>      // Copy construction! AMAZING!
> +    UniquePtr<Kennel> newKennel(new Kennel);

This doesn't call the copy constructor any more - we need |new Kennel(*kennel)| like before.

@@ +153,5 @@
>  
>      CHECK(GCFinalizesNBarkers(cx, 0));
>  
>      // Allocate a new, empty kennel.
> +    UniquePtr<Kennel> kennel2(Allocate(cx));

This needs to be |new Kennel(cx)| as it was originally, not |Allocate(cx)|.

@@ +168,5 @@
>      CHECK(GCFinalizesNBarkers(cx, 0));
>  
>      // Allocate a second barker.
> +    kennel2 = UniquePtr<Kennel>(Allocate(cx));
> +    CHECK(kennel.get());

There's a pre-existing bug here - this should check kennel2 not kennel.
Attachment #8605548 - Flags: feedback?(jimb) → feedback+
Changes made
Attachment #8605548 - Attachment is obsolete: true
Comment on attachment 8605822 [details] [diff] [review]
BugNo1142820.diff

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

Tested locally, this looks good. Thanks for the guidance John, would you mind giving this final patch a stamp of approval?
Attachment #8605822 - Flags: review?(jcoppeard)
Attachment #8605822 - Flags: feedback+
Comment on attachment 8605822 [details] [diff] [review]
BugNo1142820.diff

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

Looks good, thanks for fixing this!
Attachment #8605822 - Flags: review?(jcoppeard) → review+
https://hg.mozilla.org/mozilla-central/rev/25966a721114
Assignee: nobody → jinank94
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.