Fix some issues exposed by bug 1499644

RESOLVED FIXED in Firefox 65

Status

()

enhancement
RESOLVED FIXED
6 months ago
6 months ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla65
Points:
---

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

6 months ago
Splitting off some small fixes from bug 1499644.
(Assignee)

Comment 1

6 months ago
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.
(Assignee)

Comment 2

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

Depends on D11616
(Assignee)

Comment 3

6 months ago
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
(Assignee)

Comment 5

6 months ago
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?
Flags: needinfo?(bugmail)
(Assignee)

Comment 6

6 months ago
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);
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.
Flags: needinfo?(bugmail)
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.
(Assignee)

Comment 9

6 months ago
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..
(Assignee)

Comment 10

6 months ago
(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.
Attachment #9024389 - Attachment is obsolete: true
(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.
(Assignee)

Comment 12

6 months ago
(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.
Attachment #9024389 - Attachment is obsolete: false

Comment 14

6 months ago
Pushed by jandemooij@gmail.com:
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
(Assignee)

Comment 15

6 months ago
If bug 1506774 lands and sticks, parts 2, 3, 5 here become obsolete but I'll keep this open for now.
Keywords: leave-open
(Assignee)

Updated

6 months ago
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Attachment #9024492 - Attachment is obsolete: true
Attachment #9024389 - Attachment is obsolete: true

Comment 17

6 months ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/89279cd689a3
part 2 - Actually call cleanupLocaleTests in storage/test/unit/test_locale_collation.js. r=asuth
You need to log in before you can comment on or make changes to this bug.