1413011 - Crashes in MessageLoop::PostTask_Helper, mostly related to shutdown

Status: RESOLVED FIXED

(Core :: Graphics, defect, P3)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Filing this bug for crashes like these:

https://crash-stats.mozilla.com/report/index/ee1ecd18-7f56-47c9-a78d-e16410171028
https://crash-stats.mozilla.com/report/index/a5d5c0ac-6db2-469a-a9ce-09e9b0171027
https://crash-stats.mozilla.com/report/index/6df0ee21-8105-46da-b013-2b7340171028
https://crash-stats.mozilla.com/report/index/9064e5eb-fbc7-46fd-8245-967200171023

Most of them appear related to shutdown, although the last one is different.

Comment 1

[Andrew Osmond [:aosmond] (he/him)]

Some more recent crashes:

https://crash-stats.mozilla.com/report/index/31494389-46d6-4c7c-8552-eadd80171101
https://crash-stats.mozilla.com/report/index/96367283-9df0-4ec3-8f81-a8b1c0171031
https://crash-stats.mozilla.com/report/index/c316faa9-c068-4ca3-8736-bed110171031

All of the reports I have seen have a GPU process. That is where the real compositor thread lives. These crashes are related to shutting down the, in effect, dummy compositor thread in the UI process. This is backed up by the CompositorThreadHolderDebug annotations in the more recent builds -- if anything was using the UI process compositor thread, there would be gfxGTH metadata entries, and there are none.

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

We must assume either sCompositorThreadHolder is null (never started or already shutdown once) or sCompositorThreadHolder->message_loop() is null (thread exited another way, e.g. Thread::Stop). This crash would not have been observed prior to bug 1389021 and bug 1402592, because those bugs introduced the only dependencies on CompositorThreadHolder::Loop in CompositorThreadHolder::Shutdown. The report trend follows the introduction of bug 1389021 because this was landed in time just for the final spin of 56 (hence why it didn't appear on beta), as well as roughly when 57 beta started (hence why it didn't appear on nightly before 58).

A crash report from 56.0 which doesn't contain bug 1402592 agrees with the above:

https://crash-stats.mozilla.com/report/index/333317f5-f799-4d78-a746-cfa160171030

Comment 3

[Andrew Osmond [:aosmond] (he/him)]

There is evidence of Thread::Start failing in the crash reports.

https://crash-stats.mozilla.com/report/index/7db6ec25-2e24-4aa0-a645-1aaa40171025
https://crash-stats.mozilla.com/report/index/408e0451-87f5-43ad-b3ac-c17360171025

Unfortunately we don't have the GetLastError() return value in the above cases which may have helped diagnose what exactly went wrong in CreateThread; low memory is a candidate for one).

While these asserts are not tripped as frequently as the crash reports for this bug, I expect it is the same root cause. CompositorThreadHolder::CreateCompositorThread will return nullptr if Thread::StartWithOptions fails. In turn this means CompositorThreadHolder::GetCompositorThread will return nullptr. As a quick fix, we can make it skip shutdown if it doesn't have a thread instead of crashing. Ideally however we won't spawn a compositor thread at all in the UI process until we find we need it.

Comment 4

[Andrew Osmond [:aosmond] (he/him)]

Attachment: 0001-Bug-1413011-Allow-the-UI-chrome-process-to-shutdown-.patch

dvander, for now I'd like to just address the crash. My line of thinking is in a followup bug I will change how the compositor thread is created, so that it is on demand. That way we know we actually need it and can release assert the failure to start. But I'd rather not make that sort of change on the eve of 58 beta, and leave it to 59.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8925507 [details] [diff] [review]
0001-Bug-1413011-Allow-the-UI-chrome-process-to-shutdown-.patch

Review of attachment 8925507 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ipc/CompositorThread.cpp
@@ +126,2 @@
>    sCompositorThreadHolder = new CompositorThreadHolder();
> +  if (MOZ_UNLIKELY(!sCompositorThreadHolder->GetCompositorThread())) {

nit: no MOZ_UNLIKELY here, it's a performance hint that is dubious in effectiveness

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/531d0b700225
Allow the UI/chrome process to shutdown if the compositor thread failed to start. r=dvander

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/531d0b700225