Closed Bug 1082145 Opened 5 years ago Closed 5 years ago

|js::WatchGuts| can leak |wpmap|

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: erahm, Assigned: jj, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: [MemShrink:P3][CID 1244587][lang=c++][good first bug])

Attachments

(1 file, 1 obsolete file)

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
Whiteboard: [MemShrink][CID 1244587] → [MemShrink:P3][CID 1244587]
Mentor: erahm
Whiteboard: [MemShrink:P3][CID 1244587] → [MemShrink:P3][CID 1244587][lang=c++][good first bug]
I would like to work on this bug :)
Excellent, please do!
Assignee: nobody → jinank94
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
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.
Could fix that my wrapping WatchpointMap pointer around UniquePtr ?
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.
(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.
What test I have to run to see that it is not leaking any memory?
(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.
Wrapping wpmap around ScopedDeletePtr could fix the leaking instead of UniquePtr or should I explicity delete that before returning from the function?
(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.
(In reply to Eric Rahm from comment #11)

Could please send me link where I could find the documentation regarding js_delete function.
Attached patch patchv1.diff (obsolete) — Splinter Review
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.
Attachment #8608623 - Flags: review+
Attached patch patchv1.diffSplinter Review
(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.
I guess there is some problem with Windows XP build
(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.
(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.
(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.
Have you checked the error Eric ?
I think we're good, lets mark it checkin-needed.
Keywords: checkin-needed
Comment on attachment 8608936 [details] [diff] [review]
patchv1.diff

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

Carrying forward r+
Attachment #8608936 - Flags: review+
Attachment #8608623 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/e840013600aa
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.