657164 - ExecutablePool::release can mask leaky references

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

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);
        }
    }

Comment 1

[Nicholas Nethercote [inactive]]

Oh geez, =/== fail.  My bad.

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

Comment 2

[Nicholas Nethercote [inactive]]

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

Comment 3

[Brian Hackett [Laid off!]]

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

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: 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.

Comment 5

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/cc65c061a9b5

Comment 6

[Nicholas Nethercote [inactive]]

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.

Comment 7

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/cc65c061a9b5
http://hg.mozilla.org/mozilla-central/rev/0cf1acdb20b1