Don't crash debug Firefox builds when we're leaking ExecutablePool
Categories
(Core :: JavaScript Engine: JIT, task, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox70 | --- | affected |
People
(Reporter: mccr8, Unassigned)
References
(Blocks 1 open bug)
Details
In debug Firefox builds, we have a leak checking system set up that records what exactly is leaking, which lets us separate out different leaks on TreeHerder and gives information about what might be going wrong. As seen in bug 1527846, bug 1486101, and bug 896124, when SpiderMonkey implements its own ad hoc leak checking via fatal asserts, it interferes with our ability to understand and fix these leaks. Any leak severed enough to trigger one of these asserts will almost certainly trigger our debug or LSan leak detection.
The MOZ_ASSERT_IF(willDestroy, m_refCount == 1); assertion in ExecutablePool::release() is one example of this. If you look at the failures in bug 1527846, there might be 3 different leaks that are all getting bucketed together. The first step in any investigation of these leaks will be to locally disable the assertion, then try to reproduce the leak. I would prefer it if in Firefox builds this assertion did not happen at all.
Reporter | ||
Comment 1•5 years ago
|
||
Jan, could somebody take a look at this? A possible fix would be to remove the assert. It could also be turned into a printf, but it is possible that it will end up flooding the log a bit. Maybe there's some ifdef that would let it be defined in shell builds but not Firefox builds?
Comment 2•5 years ago
|
||
I'll look at this tomorrow morning. There might be other asserts so I'll add a leak and see which asserts trigger...
Comment 3•5 years ago
|
||
I think we could remove this assert because it's implied by this other assert in the caller, and that one does correctly check for shutdown GC leaks.
However we would still assert in the process-wide allocator in JS_ShutDown. Is an assertion failure under JS_ShutDown also bad for Gecko's leak checking? If it is, our options are:
- Force-release all leaked executable pools in debug builds.
- Add a process-wide "leaked any GC things" flag (that flag would be nice to have also to assert no leaks in the JS shell...).
Also, jonco mentioned bug 1407593 and he may pick that up soon to avoid these issues.
Reporter | ||
Comment 4•5 years ago
|
||
Any fatal assertion (MOZ_ASSERT) is going to interfere with the leak reporting.
Those options both sound okay to me.
Updated•5 years ago
|
Comment 5•5 years ago
|
||
Jon is working on bug 1407593, let's see how that progresses the coming days...
Updated•5 years ago
|
Comment 6•5 years ago
|
||
Should be fixed now.
Description
•