compositor: Handle synchronous messages while shutting down

[mrobinson]

During the shutdown process, various threads (such as the
font cache thread) may be finishing up their work. If those threads make
synchronous requests to the compositor, answer them -- even if the
results will be unused. This is at least enough processing for them to
finish their work and exit cleanly.

This addresses crashes that are sometimes seen at exit, particuarly when
the font cache thread tries to register a font during shutdown.

In addition, this change also removes an unused compositor message.

---

• ./mach build -d does not report any errors
• ./mach test-tidy does not report any errors
• There are tests for these changes but the tests are all the WPT. This should lead to less flaky crashes when integrated.

[mrobinson]

Here's an example of the failure that this addresses:

https://github.com/servo/servo/actions/runs/8331571618/job/22800303069

│ Shutting down the Constellation after generating an output file or exit flag specified
  │ called `Result::unwrap()` on an `Err` value: RecvError (thread FontCache, at components/servo/lib.rs:1031)
  │    0: servoshell::backtrace::print
  │    1: servoshell::main::{{closure}}
  │    2: <alloc::boxed::Box<F,A> as core::ops::function::Fn<Args>>::call
  │              at /rustc/a28077b28a02b92985b3a3faecf92813155f1ea1/library/alloc/src/boxed.rs:2021:9
  │       std::panicking::rust_panic_with_hook
  │              at /rustc/a28077b28a02b92985b3a3faecf92813155f1ea1/library/std/src/panicking.rs:735:13
  │    3: std::panicking::begin_panic_handler::{{closure}}
  │              at /rustc/a28077b28a02b92985b3a3faecf92813155f1ea1/library/std/src/panicking.rs:609:13
  │    4: std::sys_common::backtrace::__rust_end_short_backtrace
  │              at /rustc/a28077b28a02b92985b3a3faecf92813155f1ea1/library/std/src/sys_common/backtrace.rs:170:18
  │    5: rust_begin_unwind
  │              at /rustc/a28077b28a02b92985b3a3faecf92813155f1ea1/library/std/src/panicking.rs:597:5
  │    6: core::panicking::panic_fmt
  │              at /rustc/a28077b28a02b92985b3a3faecf92813155f1ea1/library/core/src/panicking.rs:72:14
  │    7: core::result::unwrap_failed
  │              at /rustc/a28077b28a02b92985b3a3faecf92813155f1ea1/library/core/src/result.rs:1652:5
  │    8: <servo::FontCacheWR as gfx_traits::WebrenderApi>::add_font
  │    9: gfx::font_cache_thread::FontCache::run
  │   10: std::sys_common::backtrace::__rust_begin_short_backtrace
  │   11: core::ops::function::FnOnce::call_once{{vtable.shim}}
  │   12: <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
  │              at /rustc/a28077b28a02b92985b3a3faecf92813155f1ea1/library/alloc/src/boxed.rs:2007:9
  │       <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
  │              at /rustc/a28077b28a02b92985b3a3faecf92813155f1ea1/library/alloc/src/boxed.rs:2007:9
  │       std::sys::unix::thread::Thread::new::thread_start
  │              at /rustc/a28077b28a02b92985b3a3faecf92813155f1ea1/library/std/src/sys/unix/thread.rs:108:17
  │   13: <unknown>
  │   14: <unknown>
  │ mozilla::detail::MutexImpl::~MutexImpl: pthread_mutex_destroy failed: Device or resource busy
  │ Caught signal 11thread panicked while processing panic. aborting.
  │ Redirecting call to abort() to mozalloc_abort

[gterzian]

LGTM with some nits.

[gterzian]

typo

[mrobinson]

Fixed.

[gterzian]

typo

[mrobinson]

Fixed.

[gterzian]

The shutdown flow appears to be:

1. EmbedderEvent::Quit handled by maybe_start_shutting_down. This makes sure the state is set to ShutdownState::ShuttingDown, and sends the exit message to the constellation.
2. Constellation sends CompositorMsg::ShutdownComplete when it is done shutting down, after a blocking exit call to the font cache thread.

So receiving this message in the ShutdownState::NotShuttingDown state appears impossible, so I think should panic here, or at least error. It appears to me we don't get many issues based on errors, but we do get a lot of issues when servo crashes. So panicking appears to result in more action being taken to address the underlying problems, hence I prefer it. But I know you prefer to log an error so let's at least do that.

[mrobinson]

Yeah, this situation is highly unexpected, but in the end it's a non-fatal error as we are essentially about to quit anyway. This was handled with a warning before this change as well. Regarding crashing versus recovering from errors, I think that we cannot trust the messages that come from other threads / processes, as they may be experiencing runtime errors or be controlled by an attacker. Instead of immediately crashing, I think there is some benefit to attempting to properly handle when this happens. We should obviously be on top of any warnings printed during the execution of Servo, as they are only printed in very unexpected situations.

In my experience working on other browsers though, they are very complex pieces of software. If you turn on debug assertions, it's quite likely that you can hit one simply by browsing the web or running the tests. If the browser crashed every time that happened, it would be pretty unusable.

[gterzian]

If this comment is still valid, I think it makes sense to copy it to the new location where messages are ignored while shutting down.

[mrobinson]

I think this comment might be invalid now. It refers to Paint which is a pre-WebRender concept as far as my memory serves. The main issue here is that I think we don't want to keep compositing because the other parts of Servo are in a transient state.

[gterzian]

Note: the current code does this even after shutdown has completed, so there is a change of logic, but the change seems to make more sense.

[mrobinson]

This is a really good point that I had overlooked. I agree that it doesn't make sense to keep subtracting one from the pending_frames when shutdown is complete because the count isn't used any longer.

[mrobinson]

@gterzian Thank you for the review!

[mrobinson]

Looks like this failed due to infrastructure issues.