Closed
Bug 1142816
Opened 10 years ago
Closed 10 years ago
js/src/jsapi-tests/testGCHeapPostBarriers.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 1286003][lang=c++][good first bug])
Attachments
(1 file, 2 obsolete files)
2.22 KB,
patch
|
Details | Diff | Splinter Review |
Note: this is only in test code, set priority as you see fit.
Coverity indicates that |heapData| in |TestHeapPostBarriers| [1] is leaked if any of the |CHECK|s fail.
[1] https://hg.mozilla.org/mozilla-central/annotate/0190a1d17294/js/src/jsapi-tests/testGCHeapPostBarriers.cpp#l41
Reporter | ||
Updated•10 years ago
|
Mentor: erahm
Whiteboard: [CID 1286003] → [CID 1286003][lang=c++][good first bug]
Assignee | ||
Comment 1•10 years ago
|
||
I would like to work on this bug could please give me a head start :)
Reporter | ||
Comment 2•10 years ago
|
||
Jinank Jain, thank you for your interest! Much like bug 1142826, comment 2 a |UniquePtr| would be easiest here. To access the pointer wrapped by |UniquePtr| you can use |UniquePtr::get()|.
A short example:
> mozilla::UniquePtr<char[]> wrapper = new char[10];
> char* chars = wrapper.get();
> aFunctionThatTakesCharStar(chars);
> // or you can pass in directly
> aFunctionThatTakesCharStart(wrapper.get());
Please let me know if you have any further questions!
Assignee | ||
Comment 3•10 years ago
|
||
Could someone please review my patch ?
Reporter | ||
Comment 4•10 years ago
|
||
Comment on attachment 8605487 [details] [diff] [review]
BugNo1142816.diff
Review of attachment 8605487 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good, built and tested locally. Terrence would you mind giving a sign off on this?
Attachment #8605487 -
Flags: review?(terrence)
Attachment #8605487 -
Flags: feedback+
Reporter | ||
Comment 5•10 years ago
|
||
While your at it, you might also be interested in bug 1142820. Thank you for your contributions!
Comment 6•10 years ago
|
||
Comment on attachment 8605487 [details] [diff] [review]
BugNo1142816.diff
Review of attachment 8605487 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks!
Attachment #8605487 -
Flags: review?(terrence) → review+
Reporter | ||
Comment 7•10 years ago
|
||
Reporter | ||
Comment 8•10 years ago
|
||
Assignee | ||
Comment 9•10 years ago
|
||
Are these changes deployed ?
Reporter | ||
Comment 10•10 years ago
|
||
(In reply to Jinank Jain from comment #9)
> Are these changes deployed ?
Getting there! I need to reland w/ a proper r=terrence in the title. Then it will be on mozilla-inbound, and on the next merge to mozilla-central it will be officially deployed.
Reporter | ||
Comment 11•10 years ago
|
||
Comment 12•10 years ago
|
||
Backed out for introducing new rooting hazards.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c266af06956b
https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-linux64-br-haz/20150515112844/hazards.txt.gz
Reporter | ||
Comment 13•10 years ago
|
||
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #12)
> Backed out for introducing new rooting hazards.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/c266af06956b
>
> https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-
> inbound-linux64-br-haz/20150515112844/hazards.txt.gz
:sfink thinks this is ok (perhaps intentional?), we're going to work out a way around it.
Comment 14•10 years ago
|
||
Although that code change is totally correct and fixes a leak, it is within testing code that is doing the exact sort of unsafe things that the hazard analysis is intended to catch. The previous version of the code happened to do things in a way that the analysis *can't* catch, and sadly making the code more correct also makes it easier for the analysis to understand and detect that it's doing something wrong.
Specifically, the analysis knows that storing GC pointers inside a UniquePtr is unsafe, because it knows that nothing will trace that memory to keep it alive if the GC runs. It only works in this test because we know that it starts out in the nursery and gets post-barriered (the purpose of the test is to verify this), which ends up tracing it as a side effect. If you ran a compacting GC right after the minorGC, it *wouldn't* get traced during that second GC! So this really is an unsafe construct in general, it just happens that this test can't trigger the problematic behavior.
As erahm suggested on IRC, probably the easiest way to fix this is to use mozilla::Scoped instead of UniquePtr to make sure it gets freed. It would be pretty tricky to make the analysis understand Scoped enough to complain about it too, so you should be safe!
Reporter | ||
Comment 15•10 years ago
|
||
Quick summary: we can't use |UniquePtr|, so we'll need to use |mozilla::Scoped| [1] instead. There's already a specialization that we can use: |ScopedDeletePtr|.
All that we need to do is update the patch to use |mozilla::ScopedDeletePtr| instead of |mozilla::UniquePtr|.
[1] https://hg.mozilla.org/mozilla-central/annotate/1a8343f8ed83/mfbt/Scoped.h#l80
Assignee | ||
Comment 16•10 years ago
|
||
Now this is for all the previous patches too ?
Comment 17•10 years ago
|
||
Assignee: nobody → jinank94
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Reporter | ||
Comment 18•10 years ago
|
||
(This was backed out previously)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter | ||
Comment 19•10 years ago
|
||
(In reply to Jinank Jain from comment #16)
> Now this is for all the previous patches too ?
We probably only need to use |Scoped| in this bug.
Assignee | ||
Comment 20•10 years ago
|
||
Do I have to use mozilla::ScopedDeletePtr or mozilla::Scoped ?
Reporter | ||
Comment 21•10 years ago
|
||
(In reply to Jinank Jain from comment #20)
> Do I have to use mozilla::ScopedDeletePtr or mozilla::Scoped ?
|mozilla::ScopedDeletePtr| is the easiest option.
Assignee | ||
Comment 22•10 years ago
|
||
Attachment #8605487 -
Attachment is obsolete: true
Reporter | ||
Comment 23•10 years ago
|
||
Comment on attachment 8607125 [details] [diff] [review]
patch3.diff
Review of attachment 8607125 [details] [diff] [review]:
-----------------------------------------------------------------
r=me. Can you do a try push [1] now that you have access to make sure this really fixes the hazard problem?
The format to test this is: 'try: -b do -p linux64-sh-haz'
I suggest configuring your hgrc and .ssh/config before attempting to push as documented [2] on the wiki.
[1] https://wiki.mozilla.org/ReleaseEngineering/TryServer#How_to_push_to_try
[2] https://wiki.mozilla.org/ReleaseEngineering/TryServer#Creating_an_alias
Attachment #8607125 -
Flags: review+
Assignee | ||
Comment 24•10 years ago
|
||
Could you help me out generating that private key file ? I was not able to figure out which file is that
Reporter | ||
Comment 25•10 years ago
|
||
(In reply to Jinank Jain from comment #24)
> Could you help me out generating that private key file ? I was not able to
> figure out which file is that
This should be the private part of the file you attached to your commit level 1 request in bug 1165625. Normally this would be |~/.ssh/id_rsa| on linux.
Assignee | ||
Comment 26•10 years ago
|
||
I got this error while pushing my code could you help me out in that:
pushing to ssh://hg.mozilla.org/try
The authenticity of host 'hg.mozilla.org (63.245.215.25)' can't be established.
RSA key fingerprint is fa:e8:5a:cc:de:02:e0:c7:6e:ff:14:92:90:b9:12:be.
Are you sure you want to continue connecting (yes/no)? yes
remote: Warning: Permanently added 'hg.mozilla.org,63.245.215.25' (RSA) to the list of known hosts.
remote: Permission denied (publickey).
abort: no suitable response from remote hg!
Reporter | ||
Comment 27•10 years ago
|
||
(In reply to Jinank Jain from comment #26)
> I got this error while pushing my code could you help me out in that:
>
> pushing to ssh://hg.mozilla.org/try
> The authenticity of host 'hg.mozilla.org (63.245.215.25)' can't be
> established.
> RSA key fingerprint is fa:e8:5a:cc:de:02:e0:c7:6e:ff:14:92:90:b9:12:be.
> Are you sure you want to continue connecting (yes/no)? yes
> remote: Warning: Permanently added 'hg.mozilla.org,63.245.215.25' (RSA) to
> the list of known hosts.
> remote: Permission denied (publickey).
> abort: no suitable response from remote hg!
Did you update your ~/.ssh/config as explained on the wiki [1]?
Mine has an entry that looks like:
> Host hg.mozilla.org
> User erahm@mozilla.com
> IdentityFile ~/.ssh/id_rsa
[1] https://wiki.mozilla.org/ReleaseEngineering/TryServer#.7E.2F.ssh.2Fconfig
Assignee | ||
Comment 28•10 years ago
|
||
https://hg.mozilla.org/try/rev/c8aea49396a3
I was able to do it today and that was the link for the build I guess.
Reporter | ||
Comment 29•10 years ago
|
||
(In reply to Jinank Jain from comment #28)
> https://hg.mozilla.org/try/rev/c8aea49396a3
>
> I was able to do it today and that was the link for the build I guess.
Excellent, here's the relevant link: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8aea49396a3
There we can see the build passed and the hazard analysis passed.
Now that you have an r+ and a good try build we can flag this 'checkin-needed'.
Keywords: checkin-needed
Comment 30•10 years ago
|
||
Keywords: checkin-needed
Comment 31•10 years ago
|
||
Backed out for jsapi test segfaults.
http://ftp.mozilla.org/pub/mozilla.org/spidermonkey/tinderbox-builds/mozilla-inbound-linux-debug/mozilla-inbound_linux-debug_spidermonkey-arm-sim-bm94-build1-build171.txt.gz
/builds/slave/m-in_lx-d_sm-arm-sim-000000000/src/js/src/devtools/automation/autospider.sh: line 191: 19658 Segmentation fault (core dumped) $COMMAND_PREFIX $OBJDIR/dist/bin/jsapi-tests
Also, FWIW, SM(H) builds are not what run in production (as noted on Trychooser). You want the browser hazard builds.
Comment 32•10 years ago
|
||
Assignee | ||
Comment 33•10 years ago
|
||
Eric what next to do ?
Reporter | ||
Comment 34•10 years ago
|
||
(In reply to Jinank Jain from comment #33)
> Eric what next to do ?
It looks like the latest version of your didn't remove the |delete heapData;| at the end of the function. Can you update the patch to remove that?
And then as RyanVM suggested lets do a try push against a browser hazard build:
|try: -b o -p linux64-br-haz -u none -t none|
Comment 35•10 years ago
|
||
Add a browser build too so you get the jsapi tests.
try: -b o -p linux,linux64-br-haz -u none -t none
Assignee | ||
Comment 36•10 years ago
|
||
Assignee | ||
Comment 37•10 years ago
|
||
Assignee | ||
Comment 38•10 years ago
|
||
This time it was successful and no error reports were there :)
Updated•10 years ago
|
Attachment #8607125 -
Attachment is obsolete: true
Comment 39•10 years ago
|
||
Thanks for sticking with this, I'll land it shortly :)
Comment 40•10 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Updated•7 years ago
|
Blocks: coverity-analysis
You need to log in
before you can comment on or make changes to this bug.
Description
•