Note: There are a few cases of duplicates in user autocompletion which are being worked on.

ExecutablePool::release can mask leaky references

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bhackett, Assigned: njn)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
If an executable pool is in the m_smallPools list on ExecutableAllocator destruction, then release is called with willDestroy == true.  The JS_ASSERT_IF sets the refcount to one rather than testing it against one, so leaky references are not uncovered.

    void release(bool willDestroy = false)
    {
        JS_ASSERT(m_refCount != 0);
        JS_ASSERT_IF(willDestroy, m_refCount = 1);
        if (--m_refCount == 0) {
            js::UnwantedForeground::delete_(this);
        }
    }
(Assignee)

Comment 1

6 years ago
Oh geez, =/== fail.  My bad.

How did you find this -- code inspection, or something else?
(Assignee)

Comment 2

6 years ago
This is why I prefer the "const == var" form for comparisons, BTW... but it's not standard Mozilla/SpiderMonkey practice :(
(Reporter)

Comment 3

6 years ago
(In reply to comment #1)
> Oh geez, =/== fail.  My bad.
> 
> How did you find this -- code inspection, or something else?

I was working on bug 656753 (reference leak in the TI branch) and trying to figure out why a testcase I wrote exposing the bug didn't assert.  Took some printfs and gdb watching before I noticed this, very sneaky assignment.

Don't know how often we have problems like these, but it would be nifty to have a pseudo-policy of no assignments at all in conditionals, then a tool could pick up violations super easy.
(Assignee)

Updated

6 years ago
Depends on: 654820
(Assignee)

Comment 4

6 years ago
Created attachment 532544 [details] [diff] [review]
patch

This fixes the =/== confusion and also disables the failing assertion, so that the tinderbox won't go orange.  It can be re-enabled when bug 654820 is fixed.
Assignee: general → nnethercote
Attachment #532544 - Flags: review?(bhackett1024)
(Reporter)

Updated

6 years ago
Attachment #532544 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 5

6 years ago
http://hg.mozilla.org/tracemonkey/rev/cc65c061a9b5
Whiteboard: fixed-in-tracemonkey
(Assignee)

Comment 6

6 years ago
Follow up patch that disables the assertion because it's triggering in xpcshell tests:

http://hg.mozilla.org/tracemonkey/rev/0cf1acdb20b1

I've added the details of the failure to bug 654820 which is almost certainly related.
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/cc65c061a9b5
http://hg.mozilla.org/mozilla-central/rev/0cf1acdb20b1
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.