Closed Bug 1142817 Opened 5 years ago Closed 4 years ago

/js/src/jsapi-tests/testXDR.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: shubham_sinha, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: [CID 1286075][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 |expected| in |testXDR_sourceMap| [1] is leaked.

[1] https://hg.mozilla.org/mozilla-central/annotate/0190a1d17294/js/src/jsapi-tests/testXDR.cpp#l117
Mentor: erahm
Whiteboard: [CID 1286075] → [CID 1286075][lang=c++][good first bug]
hey
i would like to work on this bug
Hi shubham_sinha, good to see you are still interested in contributing! The solution for this bug is similar to bug 1142826. Please feel free to start working on the bug and let me know if you have any questions.
How do you pointer++ in case of Unique Ptr
(In reply to Jinank Jain from comment #3)
> How do you pointer++ in case of Unique Ptr

Good question Jinank Jain!

In this case it would probably be easiest to do something along the lines of:

> UniquePtr<char16_t *> expected_wrapper = js::InflateString(cx, *sm, &len);
> char16_t *expected = expected_wrapper.get();

But let's give shubham_sinha a chance to work on this as they've shown some interest :)
hey eric,
how to know if the patch i have created works correctly or not
(In reply to shubham_sinha from comment #5)
> hey eric,
> how to know if the patch i have created works correctly or not

You can find some pretty good details about the process at: https://developer.mozilla.org/docs/Introduction

First you'll want to test that the patch builds [1], if you've already gone through and set things up this should simply be:
> ./mach build

Next you'll want to run the test to make sure it still passes. To run this specific test you'll need to do a build (you probably already did that) and then run:
> $(OBJ_DIR)/dist/bin/jsapi-tests testMutedErrors

where $(OBJ_DIR) is the where your build output goes. For me this is |obj-x86_64-unknown-linux-gnu|, it will almost always start with 'obj-'. Example of running with success:
> [erahm@mozilla21268 mozilla-central]$ obj-x86_64-unknown-linux-gnu/dist/bin/jsapi-tests testMutedErrors
> testMutedErrors
> TEST-PASS | testMutedErrors | ok
> 
> Passed: ran 1 tests.
Attached patch bug.patch (obsolete) — Splinter Review
is it correct?plz review it for me
Comment on attachment 8606592 [details] [diff] [review]
bug.patch

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

This looks great, we just need to modify it a little bit.

::: js/src/jsapi-tests/testXDR.cpp
@@ +10,5 @@
>  
>  #include "jsapi-tests/tests.h"
>  
>  #include "jsscriptinlines.h"
> +using mozilla::UniquePtr;	

nit: Please remove the trailing spaces.

@@ +128,5 @@
>          CHECK(JS_CompileScript(cx, "", 0, options, &script));
>          CHECK(script);
>  
>          size_t len = strlen(*sm);
> +        UniquePtr<char16_t> expected_wrapper(js::InflateString(cx, *sm, &len));

Looking closer at |js::InflateString| [1] it looks like we'll need to add an extra parameter to |UniquePtr| explaining how to cleanup the memory. Luckily this shouldn't be too bad.

We can use the |JS::FreePolicy| [2] to specify the deleter. A simple change should suffice:
   
>    UniquePtr<char16_t, JS::FreePolicy> expected_wrapper...

All this does is tells |UniquePtr| to use |free| instead of |delete|.

[1] https://hg.mozilla.org/mozilla-central/annotate/0273e9c967ec/js/src/jsstr.cpp#l4573
[2] https://hg.mozilla.org/mozilla-central/annotate/0273e9c967ec/js/public/Utility.h#l333
Attachment #8606592 - Flags: feedback+
Attached patch bug.patch (obsolete) — Splinter Review
Did the changes
Comment on attachment 8606701 [details] [diff] [review]
bug.patch

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

This looks good! I have a few very minor style comments. Also can you recreate your patch? This doesn't seem to be based on what's in mozilla-central (you might have multiple patches in your queue locally).

When you update the patch, please update it with a proper title and add an r=erahm to the end. There are good details on creating patches here: https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

::: js/src/jsapi-tests/testXDR.cpp
@@ -6,5 @@
>  
>  #include "jsfriendapi.h"
>  #include "jsscript.h"
>  #include "jsstr.h"
> -

This empty line doesn't need to be removed.

@@ -11,2 @@
>  #include "jsapi-tests/tests.h"
> -

This empty line doesn't need to be removed.

@@ +11,2 @@
>  #include "jsscriptinlines.h"
> +using mozilla::UniquePtr;	

nit: Please remove the tab at the end of this line and add an empty line above and below it like so:

> #include "jsscriptinlines.h"
> 
> using mozilla::UniquePtr;
> 
> static JSScript*
Attachment #8606701 - Flags: feedback+
Attached patch bug-fix.patchSplinter Review
Comment on attachment 8607329 [details] [diff] [review]
bug-fix.patch

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

Looks good, thank you for your contribution! I'll get this landed shortly.
Attachment #8607329 - Flags: review+
Attachment #8606592 - Attachment is obsolete: true
Attachment #8606701 - Attachment is obsolete: true
what to do after this regarding this bug?
(In reply to shubham_sinha from comment #14)
> what to do after this regarding this bug?

We were having some infrastructure problems yesterday, I'll get this landed today.
https://hg.mozilla.org/mozilla-central/rev/ce80e70b04e2
Assignee: nobody → shubham_sinha
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.