bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

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

RESOLVED FIXED in Firefox 41

Status

()

Core
JavaScript Engine
RESOLVED FIXED
3 years ago
9 days ago

People

(Reporter: erahm, Assigned: jj, Mentored)

Tracking

(Blocks: 1 bug, {coverity})

unspecified
mozilla41
x86_64
Linux
coverity
Points:
---

Firefox Tracking Flags

(firefox41 fixed)

Details

(Whiteboard: [CID 1286003][lang=c++][good first bug])

Attachments

(1 attachment, 2 obsolete attachments)

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
Mentor: erahm
Whiteboard: [CID 1286003] → [CID 1286003][lang=c++][good first bug]
(Assignee)

Comment 1

3 years ago
I would like to work on this bug could please give me a head start :)
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

3 years ago
Created attachment 8605487 [details] [diff] [review]
BugNo1142816.diff

Could someone please review my patch ?
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+
While your at it, you might also be interested in bug 1142820. Thank you for your contributions!
Comment on attachment 8605487 [details] [diff] [review]
BugNo1142816.diff

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

Thanks!
Attachment #8605487 - Flags: review?(terrence) → review+
(Assignee)

Comment 9

3 years ago
Are these changes deployed ?
(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.
(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.
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!
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

3 years ago
Now this is for all the previous patches too ?
https://hg.mozilla.org/mozilla-central/rev/148424ff93f9
Assignee: nobody → jinank94
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
(This was backed out previously)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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

3 years ago
Do I have to use mozilla::ScopedDeletePtr or mozilla::Scoped ?
(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

3 years ago
Created attachment 8607125 [details] [diff] [review]
patch3.diff
Attachment #8605487 - Attachment is obsolete: true
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

3 years ago
Could you help me out generating that private key file ? I was not able to figure out which file is that
(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

3 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!
(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

3 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.
(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
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.
(Assignee)

Comment 33

3 years ago
Eric what next to do ?
(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|
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

3 years ago
Created attachment 8611083 [details] [diff] [review]
patch4.diff
(Assignee)

Comment 38

3 years ago
This time it was successful and no error reports were there :)
Attachment #8607125 - Attachment is obsolete: true
Thanks for sticking with this, I'll land it shortly :)
https://hg.mozilla.org/mozilla-central/rev/42b94ddc1495
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.