114733 2013-04-17 05:20:32 -0700 releaseExecutableMemory() should canonicalize cell liveness data before it scans the GC roots. 2013-04-17 09:36:20 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 mark.lam mark.lam fpizlo ggaren mhahnenberg msaboff oliver oldest_to_newest 877121 0 mark.lam 2013-04-17 05:20:32 -0700 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. 877123 1 198500 mark.lam 2013-04-17 05:23:12 -0700 Created attachment 198500 the patch. 877253 2 198500 mhahnenberg 2013-04-17 08:48:49 -0700 Comment on attachment 198500 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.) 877339 3 oliver 2013-04-17 09:30:12 -0700 (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 877342 4 mark.lam 2013-04-17 09:31:59 -0700 (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? 877344 5 mark.lam 2013-04-17 09:32:53 -0700 Thanks for the review. Landed in r148616: <http://trac.webkit.org/changeset/148616>. 877345 6 oliver 2013-04-17 09:33:31 -0700 (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 877347 7 mhahnenberg 2013-04-17 09:36:20 -0700 (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). 198500 2013-04-17 05:23:12 -0700 2013-04-17 08:48:49 -0700 the patch. bug-114733.patch text/plain 2534 mark.lam SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTQ4NTk5KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE3IEBA CisyMDEzLTA0LTE3ICBNYXJrIExhbSAgPG1hcmsubGFtQGFwcGxlLmNvbT4KKworICAgICAgICBy ZWxlYXNlRXhlY3V0YWJsZU1lbW9yeSgpIHNob3VsZCBjYW5vbmljYWxpemUgY2VsbCBsaXZlbmVz cyBkYXRhIGJlZm9yZQorICAgICAgICBpdCBzY2FucyB0aGUgR0Mgcm9vdHMuCisgICAgICAgIGh0 dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMTQ3MzMuCisKKyAgICAgICAg UmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBoZWFwL0hlYXAuY3BwOgor ICAgICAgICAoSlNDOjpIZWFwOjpjYW5vbmljYWxpemVDZWxsTGl2ZW5lc3NEYXRhKToKKyAgICAg ICAgKiBoZWFwL0hlYXAuaDoKKyAgICAgICAgKiBydW50aW1lL0pTR2xvYmFsRGF0YS5jcHA6Cisg ICAgICAgIChKU0M6OkpTR2xvYmFsRGF0YTo6cmVsZWFzZUV4ZWN1dGFibGVNZW1vcnkpOgorCiAy MDEzLTA0LTE2ICBDb21taXQgUXVldWUgIDxybml3YUB3ZWJraXQub3JnPgogCiAgICAgICAgIFVu cmV2aWV3ZWQsIHJvbGxpbmcgb3V0IHIxNDg1NzYuCkluZGV4OiBTb3VyY2UvSmF2YVNjcmlwdENv cmUvaGVhcC9IZWFwLmNwcAo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvSmF2YVNjcmlwdENvcmUvaGVh cC9IZWFwLmNwcAkocmV2aXNpb24gMTQ4NDgyKQorKysgU291cmNlL0phdmFTY3JpcHRDb3JlL2hl YXAvSGVhcC5jcHAJKHdvcmtpbmcgY29weSkKQEAgLTQwNSw2ICs0MDUsMTEgQEAgaW5saW5lIEpT U3RhY2smIEhlYXA6OnN0YWNrKCkKICAgICByZXR1cm4gbV9nbG9iYWxEYXRhLT5pbnRlcnByZXRl ci0+c3RhY2soKTsKIH0KIAordm9pZCBIZWFwOjpjYW5vbmljYWxpemVDZWxsTGl2ZW5lc3NEYXRh KCkKK3sKKyAgICBtX29iamVjdFNwYWNlLmNhbm9uaWNhbGl6ZUNlbGxMaXZlbmVzc0RhdGEoKTsK K30KKwogdm9pZCBIZWFwOjpnZXRDb25zZXJ2YXRpdmVSZWdpc3RlclJvb3RzKEhhc2hTZXQ8SlND ZWxsKj4mIHJvb3RzKQogewogICAgIEFTU0VSVChpc1ZhbGlkVGhyZWFkU3RhdGUobV9nbG9iYWxE YXRhKSk7CkluZGV4OiBTb3VyY2UvSmF2YVNjcmlwdENvcmUvaGVhcC9IZWFwLmgKPT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PQotLS0gU291cmNlL0phdmFTY3JpcHRDb3JlL2hlYXAvSGVhcC5oCShyZXZpc2lvbiAxNDg0ODIp CisrKyBTb3VyY2UvSmF2YVNjcmlwdENvcmUvaGVhcC9IZWFwLmgJKHdvcmtpbmcgY29weSkKQEAg LTE2MSw2ICsxNjEsNyBAQCBuYW1lc3BhY2UgSlNDIHsKICAgICAgICAgSGFuZGxlU2V0KiBoYW5k bGVTZXQoKSB7IHJldHVybiAmbV9oYW5kbGVTZXQ7IH0KICAgICAgICAgSGFuZGxlU3RhY2sqIGhh bmRsZVN0YWNrKCkgeyByZXR1cm4gJm1faGFuZGxlU3RhY2s7IH0KIAorICAgICAgICB2b2lkIGNh bm9uaWNhbGl6ZUNlbGxMaXZlbmVzc0RhdGEoKTsKICAgICAgICAgdm9pZCBnZXRDb25zZXJ2YXRp dmVSZWdpc3RlclJvb3RzKEhhc2hTZXQ8SlNDZWxsKj4mIHJvb3RzKTsKIAogICAgICAgICBkb3Vi bGUgbGFzdEdDTGVuZ3RoKCkgeyByZXR1cm4gbV9sYXN0R0NMZW5ndGg7IH0KSW5kZXg6IFNvdXJj ZS9KYXZhU2NyaXB0Q29yZS9ydW50aW1lL0pTR2xvYmFsRGF0YS5jcHAKPT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0g U291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvSlNHbG9iYWxEYXRhLmNwcAkocmV2aXNpb24g MTQ4NDgyKQorKysgU291cmNlL0phdmFTY3JpcHRDb3JlL3J1bnRpbWUvSlNHbG9iYWxEYXRhLmNw cAkod29ya2luZyBjb3B5KQpAQCAtNDg3LDYgKzQ4Nyw3IEBAIHZvaWQgSlNHbG9iYWxEYXRhOjpy ZWxlYXNlRXhlY3V0YWJsZU1lbW8KICAgICBpZiAoZHluYW1pY0dsb2JhbE9iamVjdCkgewogICAg ICAgICBTdGFja1ByZXNlcnZpbmdSZWNvbXBpbGVyIHJlY29tcGlsZXI7CiAgICAgICAgIEhhc2hT ZXQ8SlNDZWxsKj4gcm9vdHM7CisgICAgICAgIGhlYXAuY2Fub25pY2FsaXplQ2VsbExpdmVuZXNz RGF0YSgpOwogICAgICAgICBoZWFwLmdldENvbnNlcnZhdGl2ZVJlZ2lzdGVyUm9vdHMocm9vdHMp OwogICAgICAgICBIYXNoU2V0PEpTQ2VsbCo+OjppdGVyYXRvciBlbmQgPSByb290cy5lbmQoKTsK ICAgICAgICAgZm9yIChIYXNoU2V0PEpTQ2VsbCo+OjppdGVyYXRvciBwdHIgPSByb290cy5iZWdp bigpOyBwdHIgIT0gZW5kOyArK3B0cikgewo=