Closed Bug 1268992 Opened 3 years ago Closed 3 years ago

Assert that the heap is empty when we do a "shutdown" GC

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

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)

The attached patch is clean on jit-tests and can stop the browser without problems. Next step is Try.
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?
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.
Try results are in and it shockingly looks like only the marionette tests are failing. Am going to try to investigate locally.
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
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
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.
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.
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 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+
(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.
https://hg.mozilla.org/mozilla-central/rev/d82860087d47
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.