Bug 114733 - releaseExecutableMemory() should canonicalize cell liveness data before it scans the GC roots.
Summary: releaseExecutableMemory() should canonicalize cell liveness data before it sc...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-04-17 05:20 PDT by Mark Lam
Modified: 2013-04-17 09:36 PDT (History)
5 users (show)

See Also:


Attachments
the patch. (2.47 KB, patch)
2013-04-17 05:23 PDT, Mark Lam
mhahnenberg: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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).