1082145 - |js::WatchGuts| can leak |wpmap|

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Eric Rahm [:erahm]]

If |wpmap| is successfully allocated in |js::WatchGuts| [1] but fails to init [2] we will leak wpmap.

[1] http://hg.mozilla.org/mozilla-central/annotate/71edd80236b2/js/src/jsobj.cpp#l3581
[2] http://hg.mozilla.org/mozilla-central/annotate/71edd80236b2/js/src/jsobj.cpp#l3582

Comment 1

[Jinank Jain[:jj]]

I would like to work on this bug :)

Comment 2

[Eric Rahm [:erahm]]

Excellent, please do!

Comment 3

[Jinank Jain[:jj]]

Hi,

Could you tell me which branch this code is on because I could not find a sync between link and codebase which I have

Comment 4

[Andrew McCreight [:mccr8]]

The code in question is in the file js/src/jsobj.cpp in the function js::WatchGuts:

    WatchpointMap* wpmap = cx->compartment()->watchpointMap;
    if (!wpmap) {
        wpmap = cx->runtime()->new_<WatchpointMap>();
        if (!wpmap || !wpmap->init()) {
            ReportOutOfMemory(cx);
            return false;
        }

The wpmap allocation will leak if wpmap->init() returns false.

Comment 5

[Jinank Jain[:jj]]

Could fix that my wrapping WatchpointMap pointer around UniquePtr ?

Comment 6

[Andrew McCreight [:mccr8]]

Possibly.  You'd need to make sure to use it with some kind of DeletePolicy that deletes it using  cx->runtime() rather than just calling delete directly.  Also, we've had some problems recently with our JS rooting analysis not understanding UniquePointer but I don't know if that's a problem for WatchpointMap.

Comment 7

[Eric Rahm [:erahm]]

(In reply to Andrew McCreight [:mccr8] from comment #6)
> Possibly.  You'd need to make sure to use it with some kind of DeletePolicy
> that deletes it using  cx->runtime() rather than just calling delete
> directly.  Also, we've had some problems recently with our JS rooting
> analysis not understanding UniquePointer but I don't know if that's a
> problem for WatchpointMap.

As Andrew said, it's possible but a bit more complicated. An alternative would be to just use |js_delete| directly in the early return path.

Comment 8

[Jinank Jain[:jj]]

What test I have to run to see that it is not leaking any memory?

Comment 9

[Andrew McCreight [:mccr8]]

(In reply to Jinank Jain from comment #8)
> What test I have to run to see that it is not leaking any memory?

I appreciate your desire to run a test, but unfortunately this particular leak will only happen when we run out of memory in a very specific place, so it will be hard to write a test.  This particular bug was identified by a static analysis tool called Coverity.  When the patch lands and Coverity is run again, it should be possible for Eric or I to confirm that the bug has been fixed.

Comment 10

[Jinank Jain[:jj]]

Wrapping wpmap around ScopedDeletePtr could fix the leaking instead of UniquePtr or should I explicity delete that before returning from the function?

Comment 11

[Eric Rahm [:erahm]]

(In reply to Jinank Jain from comment #10)
> Wrapping wpmap around ScopedDeletePtr could fix the leaking instead of
> UniquePtr or should I explicity delete that before returning from the
> function?

We're trying to avoid using |ScopedDeletePtr| anymore (unless we have to). Lets just explicitly delete using |js_delete| in this case.

Comment 12

[Jinank Jain[:jj]]

(In reply to Eric Rahm from comment #11)

Could please send me link where I could find the documentation regarding js_delete function.

Comment 13

[Jinank Jain[:jj]]

Attachment: patchv1.diff

Comment 14

[Eric Rahm [:erahm]]

Comment on attachment 8608623 [details] [diff] [review]
patchv1.diff

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

Looks good, r=me. Can you update the patch title to use the |r=erahm| format? And then can you do a try push to build against a few platforms? Something like the following should work:
   |try: -b do -p linux64,macosx64,win32 -u none -t none|

That will build on linux64, maxosx64, and win32, it should also trigger the JS tests.

Comment 15

[Jinank Jain[:jj]]

Attachment: patchv1.diff

Comment 16

[Jinank Jain[:jj]]

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

Comment 17

[Andrew McCreight [:mccr8]]

(In reply to Jinank Jain from comment #16)
> https://hg.mozilla.org/try/rev/b9110f32cbf8

You should post the tree herder link for your try push, which will show the results of the tests, rather than directly to the hg commit.

Comment 18

[Jinank Jain[:jj]]

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

Comment 19

[Jinank Jain[:jj]]

I guess there is some problem with Windows XP build

Comment 20

[Eric Rahm [:erahm]]

(In reply to Jinank Jain from comment #19)
> I guess there is some problem with Windows XP build

This might be an infrastructure problem, I'll follow up w/ the build folks.

Comment 21

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

(In reply to Andrew McCreight [:mccr8] from comment #6)
> Possibly.  You'd need to make sure to use it with some kind of DeletePolicy
> that deletes it using  cx->runtime() rather than just calling delete
> directly.  Also, we've had some problems recently with our JS rooting
> analysis not understanding UniquePointer but I don't know if that's a
> problem for WatchpointMap.

For the record, the problems with UniquePtr are rare. In fact, the only one I know of is arguably not a bug in the analysis -- it's in a test that is intentionally creating a hazardous situation.

In this case, the analysis would be fine with using a UniquePtr here, and in fact it would be a little bit safer to have the analysis double-check what's going on because it's more subtle than it appears. That said, the js_delete seems like a fine solution, and is simpler than setting up a UniquePtr with a custom deleter.

Details:

The analysis believes that the contents of memory held in a UniquePtr are unstable during GCs. So it knows that any GCPointer stored in a UniquePtr can become invalidated if you GC, because the GC may move the GCThing to a different address and it is very unlikely that anybody is going to bother to trace stuff in a UniquePtr in order to update the pointer. Who would do it? UniquePtr claims it is the sole owner.

Now, in this case none of this matters, because we're storing a WatchpointMap and it doesn't directly contain any GCPointers. So none of this matters. But even if it did, the analysis still wouldn't complain, because nothing can GC here. ReportOutOfMemory *almost* could, and it would almost make sense to move the js_delete before the ReportOutOfMemory just because it looks a little scary, but it turns out that it is internally prevented from GCing even though it calls a callback function pointer. Continuing our hypotheticals, if WatchpointMap held a GCPointer and ReportOutOfMemory could GC but js_delete was called first, the analysis would *still* be happy because wpmap would be dead before the GC call. (As in, in the only path that can GC, which is when wpmap.init() fails, wpmap is never used after the GC call so it doesn't matter if all of its GCPointers are completely bogus.)

tl;dr: the analysis understands UniquePtr, but there is no hazard here for multiple reasons. The analysis correctly figures that out.

Comment 22

[Eric Rahm [:erahm]]

(In reply to Eric Rahm [:erahm] from comment #20)
> (In reply to Jinank Jain from comment #19)
> > I guess there is some problem with Windows XP build
> 
> This might be an infrastructure problem, I'll follow up w/ the build folks.

This was bug 1167648, I'll trigger a few retries to verify everything is good to go.

Comment 23

[Jinank Jain[:jj]]

Have you checked the error Eric ?

Comment 24

[Eric Rahm [:erahm]]

I think we're good, lets mark it checkin-needed.

Comment 25

[Eric Rahm [:erahm]]

Comment on attachment 8608936 [details] [diff] [review]
patchv1.diff

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

Carrying forward r+

Comment 26

[Pulsebot]

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

Comment 27

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

https://hg.mozilla.org/mozilla-central/rev/e840013600aa