Closed Bug 1413011 Opened 2 years ago Closed 2 years ago

Crashes in MessageLoop::PostTask_Helper, mostly related to shutdown

Categories

(Core :: Graphics, defect, P3)

All
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: billm, Assigned: aosmond)

References

Details

(Keywords: crash, regression, Whiteboard: [gfx-noted])

Crash Data

Attachments

(1 file)

Priority: -- → P3
Flags: needinfo?(hshih)
Whiteboard: [gfx-noted]
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.
Flags: needinfo?(aosmond)
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
Blocks: 1389021, 1402592
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.
Assignee: nobody → aosmond
Status: NEW → ASSIGNED
Flags: needinfo?(aosmond)
Keywords: crash, regression
OS: Unspecified → Windows
Hardware: Unspecified → All
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.
Flags: needinfo?(hshih)
Attachment #8925507 - Flags: review?(dvander)
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
Attachment #8925507 - Flags: review?(dvander) → review+
Pushed by aosmond@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/531d0b700225
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.