fix: exit gracefully when the display server connection is lost - #52603
Conversation
The Ozone shutdown callback that runs when the X11/Wayland connection breaks went straight to LOG(FATAL), so every X server exit or compositor restart produced a crash report. Add Browser::ExitWithCode() (the app.exit() path, callable from native code), have the callback force-exit like app.exit(0), and keep the FATAL as a 10-second delayed watchdog in case exiting hangs.
d00223b to
f913bf3
Compare
jkleinsc
left a comment
There was a problem hiding this comment.
#52603 (review) needs to be addressed.
jkleinsc
left a comment
There was a problem hiding this comment.
Looks like there is a compile error:
The build fails in shell/browser/electron_browser_main_parts.cc with:
error: only virtual member functions can be marked 'override'
void Alarm() override { LOG(FATAL) << "Failed to shutdown."; }
This PR adds a local ShutdownWatchdog class that subclasses base::Watchdog and overrides Alarm():
class ShutdownWatchdog : public base::Watchdog {
public:
ShutdownWatchdog()
: base::Watchdog(base::Seconds(10), "OzoneShutdown", true) {}
void Alarm() override { LOG(FATAL) << "Failed to shutdown."; }
};But, base::Watchdog::Alarm() (base/threading/watchdog.h) is not virtual — only base::Watchdog::Delegate::Alarm() is:
class BASE_EXPORT Watchdog {
public:
class Delegate {
public:
virtual ~Delegate() = default;
virtual void Alarm() = 0; // <- this is the virtual one
};
...
void Alarm(); // <- non-virtual, dispatches to delegate_->Alarm() or DefaultAlarm()In base/threading/watchdog.cc:
void Watchdog::Alarm() {
if (delegate_) {
delegate_->Alarm();
} else {
DefaultAlarm();
}
}So subclassing Watchdog and redefining Alarm() just hides the base method (and would never actually be invoked by the watchdog thread) — hence the compiler correctly rejects override since there's no virtual base method being overridden.
Suggested Fix
Instead of subclassing base::Watchdog, implement base::Watchdog::Delegate and pass it into the Watchdog constructor's delegate parameter:
auto shutdown_cb = base::BindOnce([] {
class ShutdownWatchdogDelegate : public base::Watchdog::Delegate {
public:
void Alarm() override { LOG(FATAL) << "Failed to shutdown."; }
};
static base::NoDestructor<ShutdownWatchdogDelegate> delegate;
static base::NoDestructor<base::Watchdog> watchdog(
base::Seconds(10), "OzoneShutdown", /*enabled=*/true, delegate.get());
watchdog->Arm();
if (Browser* browser = Browser::Get())
browser->ExitWithCode(content::RESULT_CODE_NORMAL_EXIT);
else
LOG(FATAL) << "Failed to shutdown.";
});This uses the delegate pattern base::Watchdog actually supports for custom alarm behavior, rather than trying to override a non-virtual method.
ckerr
left a comment
There was a problem hiding this comment.
👍 on John's analysis wrt using the delegate if we need Alarm() to be fatal.
I'm not 100% sure though, does Alarm() need to be fatal? If so, John's suggestion is the one to use. If not, ...maybe we just remove the breaking Alarm() override call?
|
Release Notes Persisted
|
|
I have automatically backported this PR to "44-x-y", please check out #52684 |
|
I have automatically backported this PR to "43-x-y", please check out #52685 |
|
I have automatically backported this PR to "42-x-y", please check out #52686 |
Description of Change
On Linux the Ozone platform runs the browser-provided shutdown callback when the X11/Wayland connection breaks (X server exit, session logout, compositor restart). Ours went straight to
LOG(FATAL), so every display-server teardown produced a crash report. Chrome's equivalent runschrome::SessionEnding()first and only fatals if that fails to end the process.Add
Browser::ExitWithCode()(theapp.exit()path, callable from native code), have the callback force-exit likeapp.exit(0), and keep the fatal as a 10-second delayed watchdog for the case where exiting hangs.Checklist
npm testpassesRelease Notes
Notes: Fixed a crash report when the X server or Wayland compositor exits while an app is running.