Randell Jesup [:jesup] (needinfo me) Reporter
	Description • 7 years ago

+++ This bug was initially created as a clone of Bug #1348454 +++
Nathan - I looked into "using shared threadpools causes test timeouts"...  Even with code to drop the reference at shutdown time from the Identity service, it still happens (opt only typically).  It appears this is because we feed a KeyPair object with a reference to the 'thread' (SharedThreadPool) back to the JS, and this doesn't always get collected.  (As to why opt only, I'm not sure, though perhaps it affects if the shared thread is already shut down due to the 60 second idle timer - shutdown does a SpinUntilIdle, and 60 second is also around the shutdown killer time.  However, setting the IdleThreadTimeout to 6 seconds doesn't seem to make this avoid failing).

This was verified by using a recording in rr and watching the refcount values for the SharedThreadPool; you can see a ref taken for KeyPair that's never dropped (it appears).

Test locally with ./mach mochitest -f browser browser browser/components/uitour/test

SharedThreadPool here is nice (don't leave a rarely-used thread eating memory), so we could add another layer of abstraction to let it shut down regardless of JS holding a ref; we could add a "shut down anyways damnit" method to SharedThreadPool.  Or just add a shutdown-threads (or just xpcom-shutdown) observer and call mThread->Shutdown, and live with the wasted memory.

	Eric Rahm [:erahm]
	Comment 1 • 7 years ago

Randell, is the bug summary correct? Also can you provide a more concise comment of the problem you'd like to fix? After reading through bug 1348454 it sounds like we went from leaking a thread for each KeyPair signing to having one long running thread, but that isn't freed on shutdown, so presumably that's what this is about but I'm not sure I have the full context.

	Nathan Froyd [:froydnj]
	Comment 2 • 7 years ago

We probably see the test timeouts in opt only because we aggressively run GC/CC at shutdown in debug builds and therefore collect the KeyPair that's still floating around somewhere.  It would be good to know why we still have this KeyPair object floating around during tests...especially doing uitour tests, which shouldn't really have anything to do with crypto...

A SharedThreadPool would be nicer if we could get it to work.

	Randell Jesup [:jesup] (needinfo me) Reporter
	Comment 3 • 7 years ago

Yeah, once I realized it was in JS land, the fact that gc can be deferred makes sense -- though that also speaks to gc should happen during Idle, and especially when in a shutdown-wait idle.  

erahm: yes, we knew we were leaking the thread; I have patches that would move to a SharedThreadPool -- but as you see, they trip over what Nathan hit when he tried.  Updated title..

	Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo
	Comment 4 • 6 years ago

Is this bug still needed now that Persona's gone?

	Ryan Kelly [:rfkelly]
	Comment 5 • 6 years ago

I believe these signing routines are used by the Desktop implementation of Firefox Accounts, so the bug is still relevant.

We could move it "up the stack" into an FxA-related component such as Firefox::Firefox Accounts, or perhaps "down the stack" into a different crypto-related component.  I don't expect it to be a priority for the Sync or FxA teams any time soon though (but am open to arguments that it should be).

	Randell Jesup [:jesup] (needinfo me) Reporter
	Comment 6 • 6 years ago

Per rfkelly, clearing link to persona bugs