Closed Bug 1569655 Opened 5 years ago Closed 5 years ago

Don't crash debug Firefox builds when we're leaking ExecutablePool

Categories

(Core :: JavaScript Engine: JIT, task, P1)

task

Tracking

()

RESOLVED DUPLICATE of bug 1407593
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.

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?

Flags: needinfo?(jdemooij)

I'll look at this tomorrow morning. There might be other asserts so I'll add a leak and see which asserts trigger...

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:

  1. Force-release all leaked executable pools in debug builds.
  2. 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.

Flags: needinfo?(jdemooij) → needinfo?(continuation)

Any fatal assertion (MOZ_ASSERT) is going to interfere with the leak reporting.

Those options both sound okay to me.

Flags: needinfo?(continuation)
Flags: needinfo?(jdemooij)

Jon is working on bug 1407593, let's see how that progresses the coming days...

Depends on: 1407593
Flags: needinfo?(jdemooij)
Priority: -- → P1

Should be fixed now.

Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.