Last Comment Bug 657164 - ExecutablePool::release can mask leaky references
: ExecutablePool::release can mask leaky references
Status: RESOLVED FIXED
fixed-in-tracemonkey
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Nicholas Nethercote [:njn]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 654820
Blocks: 639743
  Show dependency treegraph
 
Reported: 2011-05-14 13:58 PDT by Brian Hackett (:bhackett)
Modified: 2011-05-23 14:09 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (1.69 KB, patch)
2011-05-15 17:04 PDT, Nicholas Nethercote [:njn]
bhackett1024: review+
Details | Diff | Splinter Review

Description Brian Hackett (:bhackett) 2011-05-14 13:58:48 PDT
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 [:njn] 2011-05-14 14:58:09 PDT
Oh geez, =/== fail.  My bad.

How did you find this -- code inspection, or something else?
Comment 2 Nicholas Nethercote [:njn] 2011-05-14 16:00:46 PDT
This is why I prefer the "const == var" form for comparisons, BTW... but it's not standard Mozilla/SpiderMonkey practice :(
Comment 3 Brian Hackett (:bhackett) 2011-05-14 18:47:05 PDT
(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 [:njn] 2011-05-15 17:04:02 PDT
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.
Comment 5 Nicholas Nethercote [:njn] 2011-05-16 18:27:22 PDT
http://hg.mozilla.org/tracemonkey/rev/cc65c061a9b5
Comment 6 Nicholas Nethercote [:njn] 2011-05-16 20:46:13 PDT
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) 2011-05-23 14:09:35 PDT
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/cc65c061a9b5
http://hg.mozilla.org/mozilla-central/rev/0cf1acdb20b1

Note You need to log in before you can comment on or make changes to this bug.