Closed Bug 1142826 Opened 5 years ago Closed 5 years ago

js/src/jsapi-tests/testMutedErrors.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 1286445][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 |chars| in |eval| [1] is leaked if any of the |CHECK|s fail.

[1] https://hg.mozilla.org/mozilla-central/annotate/0190a1d17294/js/src/jsapi-tests/testMutedErrors.cpp#l45
Mentor: erahm
Whiteboard: [CID 1286445] → [CID 1286445][lang=c++][good first bug]
hi.i am a newbie in this field.i know c/c++ and i would like to work on this bug.
(In reply to shubham_sinha from comment #1)
> hi.i am a newbie in this field.i know c/c++ and i would like to work on this
> bug.

Thank you for your interest! For this bug the simplest solution is probably to wrap |chars| in a |mozilla::UniquePtr| [1]. Please let me know if you need any help along the way! We also have some documentation [2] that might be helpful for a newbie :)

[1] https://hg.mozilla.org/mozilla-central/annotate/617dbce26726/mfbt/UniquePtr.h#l122
[2] https://developer.mozilla.org/en-US/docs/Introduction
Simply wraping chars around UniquePtr creates chaos as they are passed in the JS::Evaluate function which expects char* instead of UniquePtr so I need to ask that could it be typecast.
(In reply to Jinank Jain from comment #3)
> Simply wraping chars around UniquePtr creates chaos as they are passed in
> the JS::Evaluate function which expects char* instead of UniquePtr so I need
> to ask that could it be typecast.

Good point! You can use |UniquePtr::get()| to retrieve the raw pointer. For example:
> char* chars = ptr.get();
One more thing could you please tell me how to run test for this whether that thing worked or not ?
Attached patch Bug1142826.diff (obsolete) — Splinter Review
(In reply to Jinank Jain from comment #5)
> One more thing could you please tell me how to run test for this whether
> that thing worked or not ?

To run just 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 the test 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.
Could you please review my patch ?
Comment on attachment 8604767 [details] [diff] [review]
Bug1142826.diff

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

This looks good, I just have two minor style comments (in this case 'nit' just means a very minor request). We'll need to get a r+ from one of the JS peers, Luke do you mind taking a look?

::: js/src/jsapi-tests/testMutedErrors.cpp
@@ +4,5 @@
>  
>  #include "jsfriendapi.h"
>  #include "jsapi-tests/tests.h"
>  
> +using namespace mozilla;

nit: It looks like you shouldn't need this.

@@ +61,5 @@
>      JS::CompileOptions options(cx);
>      options.setMutedErrors(mutedErrors)
>             .setFileAndLine("", 0);
>  
> +    bool ok = JS::Evaluate(cx, options, chars.get(), len, rval);

nit: We can just return here instead of storing the result. ie:
    return JS::Evaluate(cx, options, chars.get(), len, rval);
Attachment #8604767 - Flags: review?(luke)
Attachment #8604767 - Flags: feedback+
Comment on attachment 8604767 [details] [diff] [review]
Bug1142826.diff

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

Great, thanks!

::: js/src/jsapi-tests/testMutedErrors.cpp
@@ +4,5 @@
>  
>  #include "jsfriendapi.h"
>  #include "jsapi-tests/tests.h"
>  
> +using namespace mozilla;

That's correct, we never 'using namespace mozilla;' (it actually causes all sorts of unified build bustage).  We do however commonly have explicit 'using mozilla::UniquePtr;' (and whatever else you need) so that the uses look nicer.  I'd suggest doing that here.
Attachment #8604767 - Flags: review?(luke) → review+
I got bit confused now should I use that using namespace mozilla or not ?
Comment on attachment 8604767 [details] [diff] [review]
Bug1142826.diff

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

I've add some specific recommendations. Also when you update the patch can you update the title to include the reviewer? In this case it should be:
  "Bug 1142826 - js/src/jsapi-tests/testMutedErrors.cpp has leaks. r=luke"

::: js/src/jsapi-tests/testMutedErrors.cpp
@@ +4,5 @@
>  
>  #include "jsfriendapi.h"
>  #include "jsapi-tests/tests.h"
>  
> +using namespace mozilla;

Per Luke's request: Lets change this to an explicit |using namespace mozilla::UniquePtr;|

@@ +46,5 @@
>  bool
>  eval(const char* asciiChars, bool mutedErrors, JS::MutableHandleValue rval)
>  {
>      size_t len = strlen(asciiChars);
> +    mozilla::UniquePtr<char16_t[]> chars(new char16_t[len+1]);

...and here we can just make this: |UniquePtr<char16_t[]> chars(new char16_t[len+1]);|

@@ +61,5 @@
>      JS::CompileOptions options(cx);
>      options.setMutedErrors(mutedErrors)
>             .setFileAndLine("", 0);
>  
> +    bool ok = JS::Evaluate(cx, options, chars.get(), len, rval);

...and then just clean this part up.
Attached patch BugNo1142826.diff (obsolete) — Splinter Review
I had attached the new patch
Attachment #8604767 - Attachment is obsolete: true
Attachment #8605476 - Attachment is obsolete: true
:luke Could you review it ?
Attachment #8605482 - Flags: review?(luke)
Comment on attachment 8605482 [details] [diff] [review]
BugNo1142826.diff

Thanks again!
Attachment #8605482 - Flags: review?(luke) → review+
Any progress ?
(In reply to Jinank Jain from comment #18)
> Any progress ?

I'll get this landed shortly.

At this point I'd be happy to vouch to get you try access (Level 1) [1,2]. With try access you'll be able to push your changes to our try server in order to show they build and pass tests. Once your patch passes review and looks good on try you will be able to add the 'checkin-needed' flag and our sheriffs will take care of the rest, no more waiting for me :)

For reference I had to make the same request in bug 965009.

[1] https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/
[2] https://www.mozilla.org/en-US/about/governance/policies/commit/
[3]
For getting your vouch what I had do ?
I have filled a bug as mentioned in the guidelines
(In reply to Jinank Jain from comment #23)
> I have filled a bug as mentioned in the guidelines

I've vouched for you. It might take a few days to get processed.
https://hg.mozilla.org/mozilla-central/rev/4b4e3bb1097c
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.