1506554 - Fix some issues exposed by bug 1499644

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Jan de Mooij [:jandem]]

Splitting off some small fixes from bug 1499644.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Bug 1506554 part 1 - Sweep JitZone/JitRealm *after* discarding JIT code. r?jonco

This is not a problem yet because of how GC/TypeScript/BaselineScript interact,
but this caused crashes on Try with the patch for bug 1499644.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Bug 1506554 part 2 - Actually call cleanupLocaleTests in storage/test/unit/test_locale_collation.js. r?asuth

The cleanup function was there but never called, as far as I can see.

Depends on D11616

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Bug 1506554 part 3 - Perform a shrinking GC in SpecialPowersAPI.exactGC. r?mccr8

Some tests use exactGC to trigger a GC to verify some resource was cleaned up
by the GC. This causes some issues on Try with bug 1499644 because ICScripts
(like TypeScripts) will not be purged on every non-shrinking GC and ICScripts
can hold on to JS objects.

Depends on D11617

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Bug 1506554 part 4 - Remove an unnecessary ensureHasTypes call in DebugEnvironmentProxyHandler::handleUnaliasedAccess. r?bhackett

Depends on D11618

Comment 5

[Jan de Mooij [:jandem]]

Actually the issue part 2 tried to fix is that I got the following assertion failure:

  MOZ_ASSERT(threadOpenedOn == NS_GetCurrentThread());

With the following stack (I left out some less-interesting frames):

  nsCycleCollector::ShutdownCollect
  -> nsCycleCollector::FixGrayBits
  -> JS::NonIncrementalGC
  -> CycleCollectedJSRuntime::FinalizeDeferredThings
  -> IncrementalFinalizeRunnable::ReleaseNow
  -> mozilla::storage::Connection::Release()
  -> mozilla::storage::Connection::Close()
  -> mozilla::storage::Connection::isAsyncExecutionThreadAvailable()

However I now also see this on Android with test_logins_metainfo.js :(

How is this supposed to work? The patch for bug 1499644 that triggers this might keep some objects alive a bit longer, does the storage code rely on GC/CC behavior?

Comment 6

[Jan de Mooij [:jandem]]

It's also a bit curious that this fails:

  MOZ_ASSERT(threadOpenedOn == NS_GetCurrentThread());

But this check in Connection::Close apparently passes:

  bool onOpenerThread = false;
  (void)threadOpenedOn->IsOnCurrentThread(&onOpenerThread);
  MOZ_ASSERT(onOpenerThread);

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

If I'm using searchfox right, this cycle collection is happening beyond the point that the "xpcom-shutdown-threads" observer notification is generated and threads are shutdown and "what thread am I" is no longer a valid question to ask.  That's way too late.

Specifically, the call point it looks like is https://searchfox.org/mozilla-central/rev/7f7c353e969e61a6a85201cc8ad3c3de12ac30d8/xpcom/build/XPCOMInit.cpp#1006 which is strictly after https://searchfox.org/mozilla-central/rev/7f7c353e969e61a6a85201cc8ad3c3de12ac30d8/xpcom/build/XPCOMInit.cpp#908 but maybe that's wrong and you can provide a full stack so I can better understand what's going on.

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

Given that these are both xpcshell tests, it may be the case that we want to more proactively nuke the testing global before full XPCOM teardown?  Alternately, mozStorage could further ratchet up the explosion factor if we get to "xpcom-shutdown-threads" in an xpcshell test with open connections since it seems clear the tests are broken in a uniquely xpcshell-way.

Comment 9

[Jan de Mooij [:jandem]]

Thanks Andrew. I just did some more debugging and it turns out XPCShellImpl.cpp has a JS_GC call here that I think was sufficient before my changes:

https://searchfox.org/mozilla-central/rev/7f7c353e969e61a6a85201cc8ad3c3de12ac30d8/js/xpconnect/src/XPCShellImpl.cpp#1448

One option is to change that to a shrinking GC..

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #9)
> One option is to change that to a shrinking GC..

Hm it's probably nicer/safer to discard TypeScripts whenever we discard JIT code. I think that's OK perf wise and it should fix this as well. I'll try this first.

Comment 11

[Andrew McCreight [:mccr8]]

(In reply to Jan de Mooij [:jandem] from comment #9)
> One option is to change that to a shrinking GC..

That seems like a good improvement. One thing to watch for in your try pushes is intermittent leaks. It sounds like your changes could cause more of them. Shutdown collections are kind of a pile of hacks.

I'm not sure how FinalizeDeferredThings ends up triggering a GC, but I guess that whole stack is just some kind of symptom of a leak.

Comment 12

[Jan de Mooij [:jandem]]

(In reply to Andrew McCreight [:mccr8] from comment #11)
> That seems like a good improvement.

Hm maybe I should just continue with this approach (triggering more shrinking GCs in test code) and if that's too intermittent-prone I can try comment 10. With these patches my Try push look pretty great, so fingers crossed.

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Bug 1506554 part 5 - Trigger shrinking GCs instead of non-shrinking GCs when shutting down xpcshell. r?jonco

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d7d3b7b0b12
part 1 - Sweep JitZone/JitRealm *after* discarding JIT code. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae382e944ea8
part 4 - Remove an unnecessary ensureHasTypes call in DebugEnvironmentProxyHandler::handleUnaliasedAccess. r=bhackett

Comment 15

[Jan de Mooij [:jandem]]

If bug 1506774 lands and sticks, parts 2, 3, 5 here become obsolete but I'll keep this open for now.

Comment 16

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/4d7d3b7b0b12
https://hg.mozilla.org/mozilla-central/rev/ae382e944ea8

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/89279cd689a3
part 2 - Actually call cleanupLocaleTests in storage/test/unit/test_locale_collation.js. r=asuth

Comment 18

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/89279cd689a3