releaseExecutableMemory() should canonicalize cell liveness data before it scans the GC roots.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

Not doing this resulted in a runtime assertion failure due to a MarkedBlock being in the FreeListed state while releaseExecutableMemory() is calling Heap::getConservativeRegisterRoots().  This was encountered while testing script timeout code (which uses releaseExecutableMemory()).

Patch forthcoming.

[Mark Lam]

Created attachment 198500 [details]
the patch.

[Mark Hahnenberg]

Comment on attachment 198500 [details]
the patch.

r=me. Maybe we should add an assertion that the Heap has been canonicalized whenever we're doing anything with root scanning (i.e. inside getConservativeRegisterRoots, gatherConservativeRoots, etc.)

[Oliver Hunt]

(In reply to comment #2)
> (From update of attachment 198500 [details])
> r=me. Maybe we should add an assertion that the Heap has been canonicalized whenever we're doing anything with root scanning (i.e. inside getConservativeRegisterRoots, gatherConservativeRoots, etc.)

i'd r+ patch that did that

[Mark Lam]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 198500 [details] [details])
> > r=me. Maybe we should add an assertion that the Heap has been canonicalized whenever we're doing anything with root scanning (i.e. inside getConservativeRegisterRoots, gatherConservativeRoots, etc.)
> 
> i'd r+ patch that did that

How does one assert that the heap has be canonicalized without actually doing a canonicalizing pass?

[Mark Lam]

Thanks for the review.  Landed in r148616: <http://trac.webkit.org/changeset/148616>.

[Oliver Hunt]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (From update of attachment 198500 [details] [details] [details])
> > > r=me. Maybe we should add an assertion that the Heap has been canonicalized whenever we're doing anything with root scanning (i.e. inside getConservativeRegisterRoots, gatherConservativeRoots, etc.)
> > 
> > i'd r+ patch that did that
> 
> How does one assert that the heap has be canonicalized without actually doing a canonicalizing pass?

No idea, that's why i said i'd r+ it rather than write it :D

[Mark Hahnenberg]

(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > (From update of attachment 198500 [details] [details] [details])
> > > r=me. Maybe we should add an assertion that the Heap has been canonicalized whenever we're doing anything with root scanning (i.e. inside getConservativeRegisterRoots, gatherConservativeRoots, etc.)
> > 
> > i'd r+ patch that did that
> 
> How does one assert that the heap has be canonicalized without actually doing a canonicalizing pass?

There's a number of ways. One would be to add a flag that states whether the Heap (or the MarkedSpace) is in the "Canonicalized" state. You would then set this flag whenever canonicalize gets called and unset it whenever allocation caused us to leave the "Canonicalized" state.

You could also just ask all of the MarkedAllocators if their internal state matches that of being canonicalized (i.e. their FreeLists are empty and their m_currentBlock == 0).