1635196 - Crash in tests, when some tests fail

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect, P2)

Description

[Julien Wajsberg [:julienw]]

Hey,
while looking at intermittents, I've seen this crash:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=300547008&repo=mozilla-central&lineNumber=4072

It looks like we crash because a promise isn't disconnected when (maybe) Firefox shuts down.
I'm not sure where this promise is created in the first place, but I do see nsProfiler::StartProfiler is part of the stacktrace.

I don't know more really :-)

Comment 1

[Markus Stange [:mstange]]

This is a crash in a diagnostic assert, but I'm not sure I completely understand its purpose.
My guess is that we should call mPromiseHolder->Reject(...) before we destroy the MozPromiseHolder object.

Comment 2

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Thank you Julien for the report, and Markus for the early analysis.

I think by design, MozPromises must be resolved or rejected if there is at least one Then waiting for it, otherwise that Then will wait forever.
So yes, we should reject the promise before resetting it.

(Another option would be to "disconnect" the Then, but we don't want that here, because the existing Thens are themselves tasked with resolving/rejecting a JS Promise, which we should respond to as well.)

I hope whoever waits for these JS promises can handle rejections! I'll have a look...

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1635196 - Make sure profile-gathering promise is fulfilled before destroying holder - r?mstange

While gathering profiles from child processes, some actions (e.g., shutting down, restarting the profiler) may reset the gathering operation.
In this case we must ensure that the promise is rejected if not already fulfilled, so that anyone waiting on it won't be blocked forever (and MozPromise enforces it in a MOZ_DIAGNOSTIC_ASSERT).

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1635196 - GetSymbolTableMozPromise can fulfill the promise on any thread - r?mstange

It is not necessary to dispatch another task to the main thread to resolve/reject a MozPromiseHolder, because wherever this happens, the attached Then will do its own dispatch to the main thread.
Also, this removes a dispatch that could potentially fail, leaving the promise unfulfilled.

Depends on D73790

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1635196 - Make sure GetSymbolTableMozPromise promise is always fulfilled - r?mstange

In rare cases, a dispatch may fail (e.g., threads are shutting down), we should handle this failure by rejecting the promise on the spot.

Depends on D73791

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/103d46d0ce15
Make sure profile-gathering promise is fulfilled before destroying holder - r=mstange
https://hg.mozilla.org/integration/autoland/rev/63b9ac1938ba
GetSymbolTableMozPromise can fulfill the promise on any thread - r=mstange
https://hg.mozilla.org/integration/autoland/rev/dc9ab7ee6c95
Make sure GetSymbolTableMozPromise promise is always fulfilled - r=mstange

Comment 7

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/103d46d0ce15
https://hg.mozilla.org/mozilla-central/rev/63b9ac1938ba
https://hg.mozilla.org/mozilla-central/rev/dc9ab7ee6c95