Closed
Bug 1268992
Opened 7 years ago
Closed 7 years ago
Assert that the heap is empty when we do a "shutdown" GC
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: terrence, Assigned: terrence)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
7.05 KB,
patch
|
jonco
:
review+
|
Details | Diff | Splinter Review |
The attached patch is clean on jit-tests and can stop the browser without problems. Next step is Try.
Comment 1•7 years ago
|
||
Comment on attachment 8747295 [details] [diff] [review] 7.0_assert_heap_is_empty_after_shutdown_gc-v0.diff Review of attachment 8747295 [details] [diff] [review]: ----------------------------------------------------------------- \o/ ::: js/src/jsgc.cpp @@ +3940,5 @@ > + case AllocKind::FAT_INLINE_STRING: return "FAT_INLINE_STRING"; > + case AllocKind::STRING: return "STRING"; > + case AllocKind::EXTERNAL_STRING: return "EXTERNAL_STRING"; > + case AllocKind::SYMBOL: return "SYMBOL"; > + case AllocKind::JITCODE: return "JITCODE"; Don't we have some kind of higher order macro we could use here?
Assignee | ||
Comment 2•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e548a3d4ed0
Assignee | ||
Comment 3•7 years ago
|
||
For trace kind yes. I didn't see one for AllocKind; we should add one. I only didn't because I want to find something to actually test this against before I spend too much time on it.
Assignee | ||
Comment 4•7 years ago
|
||
Try results are in and it shockingly looks like only the marionette tests are failing. Am going to try to investigate locally.
Assignee | ||
Comment 5•7 years ago
|
||
Cleaned up and added path printing when we fail. If we don't have an official "this is where we clean up pointers before shutdown", it may be sufficient to fix up users in an ad-hoc fashion as they pop up. https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2bfe2c59c64
Assignee | ||
Comment 6•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba5f436f9eaf
Assignee | ||
Comment 7•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7f16233e865
Assignee | ||
Comment 8•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd61ffbf19d5
Assignee | ||
Comment 9•7 years ago
|
||
The findPaths process needs to iterate roots (obvs), so my first attempt was crashing when we try to do so when under PHASE_SWEEP. I've added an enum flag to choose when we do the findPaths and added a new method that uses that before we sweep the zones outside of the PHASE_SWEEP. https://treeherder.mozilla.org/#/jobs?repo=try&revision=83a5216930fe
Assignee | ||
Comment 10•7 years ago
|
||
Even if we avoid doing this in the sweep phase, there are way too many other GC state concerns. As a new approach, I think we want to leak the zones instead of forcing teardown and collect report on any still active at the top level.
Assignee | ||
Comment 11•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=12a73334486d
Assignee | ||
Comment 12•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0548f6409892
Assignee | ||
Comment 13•7 years ago
|
||
I think we are actually SOL for doing findPaths during DESTROY_RUNTIME because the atoms have already been brought down. I think this could use a redesign, but that's out of scope for this bug.
Assignee | ||
Comment 14•7 years ago
|
||
I think this is more or less the best we can do. Note that because we cannot print the paths here, we also cannot crash -- doing so would prevent the CC shutdown logging from happening, which would make these problems much harder to debug.
Attachment #8747295 -
Attachment is obsolete: true
Attachment #8751797 -
Flags: review?(jcoppeard)
Comment 15•7 years ago
|
||
Comment on attachment 8751797 [details] [diff] [review] 7.0_assert_heap_is_empty_after_shutdown_gc-v1.diff Review of attachment 8751797 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! ::: dom/base/nsXMLHttpRequest.h @@ -140,5 @@ > IMPL_EVENT_HANDLER(error) > IMPL_EVENT_HANDLER(load) > IMPL_EVENT_HANDLER(timeout) > IMPL_EVENT_HANDLER(loadend) > - While I agree with this change, you proabably didn't meant to include it here. ::: js/src/jsgc.cpp @@ +3932,5 @@ > +{ > +#ifdef DEBUG > + if (!arenaLists[kind].isEmpty()) { > + for (Arena* current = arenaLists[kind].head(); current; current = current->next) { > + for (ArenaCellIterUnderFinalize i(current); !i.done(); i.next()) { Can we not use ZoneCellIterUnderGC (or Steve's replacement) here?
Attachment #8751797 -
Flags: review?(jcoppeard) → review+
Assignee | ||
Comment 16•7 years ago
|
||
(In reply to Jon Coppeard (:jonco) from comment #15) > ::: js/src/jsgc.cpp > @@ +3932,5 @@ > > +{ > > +#ifdef DEBUG > > + if (!arenaLists[kind].isEmpty()) { > > + for (Arena* current = arenaLists[kind].head(); current; current = current->next) { > > + for (ArenaCellIterUnderFinalize i(current); !i.done(); i.next()) { > > Can we not use ZoneCellIterUnderGC (or Steve's replacement) here? Yes, but not without some annoying restructuring. I'm going to go ahead and try to land it in its current form. If you feel strongly about it, I can try to rework it to iterate the zones before we tear them down.
Assignee | ||
Comment 17•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d82860087d47d10791757d56e289076800318ae1 Bug 1268992 - Assert that the heap is empty after a shutdown GC; r=jonco
Comment 18•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d82860087d47
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in
before you can comment on or make changes to this bug.
Description
•