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

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: 7.0_assert_heap_is_empty_after_shutdown_gc-v0.diff

The attached patch is clean on jit-tests and can stop the browser without problems. Next step is Try.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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?

Comment 2

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e548a3d4ed0

Comment 3

[Terrence Cole [:terrence]]

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.

Comment 4

[Terrence Cole [:terrence]]

Try results are in and it shockingly looks like only the marionette tests are failing. Am going to try to investigate locally.

Comment 5

[Terrence Cole [:terrence]]

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

Comment 6

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ba5f436f9eaf

Comment 7

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7f16233e865

Comment 8

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd61ffbf19d5

Comment 9

[Terrence Cole [:terrence]]

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

Comment 10

[Terrence Cole [:terrence]]

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.

Comment 11

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=12a73334486d

Comment 12

[Terrence Cole [:terrence]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0548f6409892

Comment 13

[Terrence Cole [:terrence]]

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.

Comment 14

[Terrence Cole [:terrence]]

Attachment: 7.0_assert_heap_is_empty_after_shutdown_gc-v1.diff

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.

Comment 15

[Jon Coppeard (:jonco) (PTO until 3rd April)]

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?

Comment 16

[Terrence Cole [:terrence]]

(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.

Comment 17

[Terrence Cole [:terrence]]

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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d82860087d47