Bug 114733

Summary: releaseExecutableMemory() should canonicalize cell liveness data before it scans the GC roots.
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: fpizlo, ggaren, mhahnenberg, msaboff, oliver
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
the patch. mhahnenberg: review+

Description Mark Lam 2013-04-17 05:20:32 PDT
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.
Comment 1 Mark Lam 2013-04-17 05:23:12 PDT
Created attachment 198500 [details]
the patch.
Comment 2 Mark Hahnenberg 2013-04-17 08:48:49 PDT
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.)
Comment 3 Oliver Hunt 2013-04-17 09:30:12 PDT
(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
Comment 4 Mark Lam 2013-04-17 09:31:59 PDT
(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?
Comment 5 Mark Lam 2013-04-17 09:32:53 PDT
Thanks for the review.  Landed in r148616: <http://trac.webkit.org/changeset/148616>.
Comment 6 Oliver Hunt 2013-04-17 09:33:31 PDT
(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
Comment 7 Mark Hahnenberg 2013-04-17 09:36:20 PDT
(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).