Closed
Bug 1142826
Opened 9 years ago
Closed 9 years ago
js/src/jsapi-tests/testMutedErrors.cpp has leaks
Categories
(Core :: JavaScript Engine, defect)
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)
2.30 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•9 years ago
|
Mentor: erahm
Whiteboard: [CID 1286445] → [CID 1286445][lang=c++][good first bug]
Comment 1•9 years ago
|
||
hi.i am a newbie in this field.i know c/c++ and i would like to work on this bug.
Reporter | ||
Comment 2•9 years ago
|
||
(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
Assignee | ||
Comment 3•9 years ago
|
||
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.
Reporter | ||
Comment 4•9 years ago
|
||
(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();
Assignee | ||
Comment 5•9 years ago
|
||
One more thing could you please tell me how to run test for this whether that thing worked or not ?
Assignee | ||
Comment 6•9 years ago
|
||
Reporter | ||
Comment 7•9 years ago
|
||
(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.
Assignee | ||
Comment 8•9 years ago
|
||
Could you please review my patch ?
Reporter | ||
Comment 9•9 years ago
|
||
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 10•9 years ago
|
||
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+
Assignee | ||
Comment 11•9 years ago
|
||
I got bit confused now should I use that using namespace mozilla or not ?
Reporter | ||
Comment 12•9 years ago
|
||
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.
Assignee | ||
Comment 13•9 years ago
|
||
Assignee | ||
Comment 14•9 years ago
|
||
I had attached the new patch
Assignee | ||
Comment 15•9 years ago
|
||
Attachment #8604767 -
Attachment is obsolete: true
Attachment #8605476 -
Attachment is obsolete: true
Assignee | ||
Comment 16•9 years ago
|
||
:luke Could you review it ?
Reporter | ||
Updated•9 years ago
|
Attachment #8605482 -
Flags: review?(luke)
Comment 17•9 years ago
|
||
Comment on attachment 8605482 [details] [diff] [review] BugNo1142826.diff Thanks again!
Attachment #8605482 -
Flags: review?(luke) → review+
Assignee | ||
Comment 18•9 years ago
|
||
Any progress ?
Reporter | ||
Comment 19•9 years ago
|
||
(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]
Reporter | ||
Comment 20•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b4e3bb1097c
Assignee | ||
Comment 21•9 years ago
|
||
For getting your vouch what I had do ?
Assignee | ||
Comment 22•9 years ago
|
||
https://bugzilla.mozilla.org/show_bug.cgi?id=1165625
Assignee | ||
Comment 23•9 years ago
|
||
I have filled a bug as mentioned in the guidelines
Reporter | ||
Comment 24•9 years ago
|
||
(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.
Comment 25•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4b4e3bb1097c
Assignee: nobody → jinank94
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Updated•6 years ago
|
Blocks: coverity-analysis
You need to log in
before you can comment on or make changes to this bug.
Description
•