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

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Eric Rahm [:erahm]]

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

Comment 1

[Jinank Jain[:jj]]

I would like to work on this bug could please give me a head start :)

Comment 2

[Eric Rahm [:erahm]]

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!

Comment 3

[Jinank Jain[:jj]]

Attachment: BugNo1142816.diff

Could someone please review my patch ?

Comment 4

[Eric Rahm [:erahm]]

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?

Comment 5

[Eric Rahm [:erahm]]

While your at it, you might also be interested in bug 1142820. Thank you for your contributions!

Comment 6

[Terrence Cole [:terrence]]

Comment on attachment 8605487 [details] [diff] [review]
BugNo1142816.diff

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

Thanks!

Comment 7

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3902a6225c3

Comment 8

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/acf9c5ff8fdc

Comment 9

[Jinank Jain[:jj]]

Are these changes deployed ?

Comment 10

[Eric Rahm [:erahm]]

(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.

Comment 11

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/148424ff93f9

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Eric Rahm [:erahm]]

(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

[Steve Fink [:sfink] [:s:]]

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!

Comment 15

[Eric Rahm [:erahm]]

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

Comment 16

[Jinank Jain[:jj]]

Now this is for all the previous patches too ?

Comment 17

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/148424ff93f9

Comment 18

[Eric Rahm [:erahm]]

(This was backed out previously)

Comment 19

[Eric Rahm [:erahm]]

(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.

Comment 20

[Jinank Jain[:jj]]

Do I have to use mozilla::ScopedDeletePtr or mozilla::Scoped ?

Comment 21

[Eric Rahm [:erahm]]

(In reply to Jinank Jain from comment #20)
> Do I have to use mozilla::ScopedDeletePtr or mozilla::Scoped ?

|mozilla::ScopedDeletePtr| is the easiest option.

Comment 22

[Jinank Jain[:jj]]

Attachment: patch3.diff

Comment 23

[Eric Rahm [:erahm]]

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

Comment 24

[Jinank Jain[:jj]]

Could you help me out generating that private key file ? I was not able to figure out which file is that

Comment 25

[Eric Rahm [:erahm]]

(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.

Comment 26

[Jinank Jain[:jj]]

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!

Comment 27

[Eric Rahm [:erahm]]

(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 [e-mail]
>   IdentityFile ~/.ssh/id_rsa

[1] https://wiki.mozilla.org/ReleaseEngineering/TryServer#.7E.2F.ssh.2Fconfig

Comment 28

[Jinank Jain[:jj]]

https://hg.mozilla.org/try/rev/c8aea49396a3

I was able to do it today and that was the link for the build I guess.

Comment 29

[Eric Rahm [:erahm]]

(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'.

Comment 30

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e602cb65a030

Comment 31

[Ryan VanderMeulen [:RyanVM]]

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

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7e851e4c2d2

Comment 33

[Jinank Jain[:jj]]

Eric what next to do ?

Comment 34

[Eric Rahm [:erahm]]

(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

[Ryan VanderMeulen [:RyanVM]]

Add a browser build too so you get the jsapi tests.
try: -b o -p linux,linux64-br-haz -u none -t none

Comment 36

[Jinank Jain[:jj]]

Attachment: patch4.diff

Comment 37

[Jinank Jain[:jj]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d5ccd4a6357

Comment 38

[Jinank Jain[:jj]]

This time it was successful and no error reports were there :)

Comment 39

[Ryan VanderMeulen [:RyanVM]]

Thanks for sticking with this, I'll land it shortly :)

Comment 40

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/42b94ddc1495

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/42b94ddc1495