From 75f86a6f6079271ba48288a242d13f841f933d04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonas=20Dre=C3=9Fler?= Date: Sun, 1 Oct 2023 20:10:33 +0200 Subject: [PATCH] screencastService: Propagate machine-parseable error types to gnome-shell We'll be using hardware encoding for screencasts soon, so we'll likely see more things go wrong in the future, including crashes of the whole screencastService. To deal with this, we'll introduce logic to blocklist certain recording pipelines in case of failure and also add some logic to retry the recording automatically. To allow for better messaging to the user in those failure cases, we want to be aware in gnome-shell, what exactly the error in the recorder was. So propagate the most common types of errors that can happen in the ScreencastService to gnome-shell using the new DBusError module. Part-of: --- .../org.gnome.Shell.Screencast.xml | 1 + .../screencast/screencastService.js | 98 +++++++++++++------ js/misc/dbusErrors.js | 11 +++ js/ui/screenshot.js | 18 ++-- 4 files changed, 91 insertions(+), 37 deletions(-) diff --git a/data/dbus-interfaces/org.gnome.Shell.Screencast.xml b/data/dbus-interfaces/org.gnome.Shell.Screencast.xml index 6512396d9..9a8eb3293 100644 --- a/data/dbus-interfaces/org.gnome.Shell.Screencast.xml +++ b/data/dbus-interfaces/org.gnome.Shell.Screencast.xml @@ -93,6 +93,7 @@ + diff --git a/js/dbusServices/screencast/screencastService.js b/js/dbusServices/screencast/screencastService.js index 9c69af9cf..b87443e08 100644 --- a/js/dbusServices/screencast/screencastService.js +++ b/js/dbusServices/screencast/screencastService.js @@ -6,6 +6,7 @@ import Gtk from 'gi://Gtk?version=4.0'; import {ServiceImplementation} from './dbusService.js'; +import {ScreencastErrors, ScreencastError} from './misc/dbusErrors.js'; import {loadInterfaceXML, loadSubInterfaceXML} from './misc/dbusUtils.js'; import * as Signals from './misc/signals.js'; @@ -150,13 +151,13 @@ class Recorder extends Signals.EventEmitter { } } - _bailOutOnError(error) { + _bailOutOnError(message, errorDomain = ScreencastErrors, errorCode = ScreencastError.RECORDER_ERROR) { + const error = new GLib.Error(errorDomain, errorCode, message); + this._teardownPipeline(); this._unwatchSender(); this._stopSession(); - log(`Recorder error: ${error.message}`); - if (this._startRequest) { this._startRequest.reject(error); delete this._startRequest; @@ -170,13 +171,13 @@ class Recorder extends Signals.EventEmitter { this.emit('error', error); } - _handleFatalPipelineError(message) { + _handleFatalPipelineError(message, errorDomain, errorCode) { this._pipelineState = PipelineState.ERROR; - this._bailOutOnError(new Error(`Fatal pipeline error: ${message}`)); + this._bailOutOnError(message, errorDomain, errorCode); } _senderVanished() { - this._bailOutOnError(new Error('Sender has vanished')); + this._bailOutOnError('Sender has vanished'); } _onSessionClosed() { @@ -184,7 +185,7 @@ class Recorder extends Signals.EventEmitter { return; // We closed the session ourselves this._sessionState = SessionState.STOPPED; - this._bailOutOnError(new Error('Session closed unexpectedly')); + this._bailOutOnError('Session closed unexpectedly'); } _initSession(sessionPath) { @@ -197,7 +198,8 @@ class Recorder extends Signals.EventEmitter { _tryNextPipeline() { const {done, value: pipelineConfig} = this._pipelineConfigs.next(); if (done) { - this._handleFatalPipelineError('All pipelines failed to start'); + this._handleFatalPipelineError('All pipelines failed to start', + ScreencastErrors, ScreencastError.ALL_PIPELINES_FAILED); return; } @@ -325,7 +327,8 @@ class Recorder extends Signals.EventEmitter { case PipelineState.PLAYING: this._addRecentItem(); - this._handleFatalPipelineError('Unexpected EOS message'); + this._handleFatalPipelineError('Unexpected EOS message', + ScreencastErrors, ScreencastError.PIPELINE_ERROR); break; case PipelineState.FLUSHING: @@ -358,11 +361,14 @@ class Recorder extends Signals.EventEmitter { break; case PipelineState.PLAYING: - case PipelineState.FLUSHING: - // Everything else we can't handle, so error out + case PipelineState.FLUSHING: { + const [error] = message.parse_error(); this._handleFatalPipelineError( - `GStreamer error while in state ${this._pipelineState}: ${message.parse_error()[0].message}`); + `GStreamer error while in state ${this._pipelineState}: ${error.message}`, + ScreencastErrors, ScreencastError.PIPELINE_ERROR); + break; + } default: break; @@ -526,17 +532,19 @@ export const ScreencastService = class extends ServiceImplementation { } async ScreencastAsync(params, invocation) { - let returnValue = [false, '']; - if (this._lockdownSettings.get_boolean('disable-save-to-disk')) { - invocation.return_value(GLib.Variant.new('(bs)', returnValue)); + invocation.return_error_literal(ScreencastErrors, + ScreencastError.SAVE_TO_DISK_DISABLED, + 'Saving to disk is disabled'); return; } const sender = invocation.get_sender(); if (this._recorders.get(sender)) { - invocation.return_value(GLib.Variant.new('(bs)', returnValue)); + invocation.return_error_literal(ScreencastErrors, + ScreencastError.ALREADY_RECORDING, + 'Service is already recording'); return; } @@ -558,7 +566,10 @@ export const ScreencastService = class extends ServiceImplementation { invocation); } catch (error) { log(`Failed to create recorder: ${error.message}`); - invocation.return_value(GLib.Variant.new('(bs)', returnValue)); + invocation.return_error_literal(ScreencastErrors, + ScreencastError.RECORDER_ERROR, + error.message); + return; } @@ -566,33 +577,46 @@ export const ScreencastService = class extends ServiceImplementation { try { await recorder.startRecording(); - returnValue = [true, filePath]; + invocation.return_value(GLib.Variant.new('(bs)', [true, filePath])); } catch (error) { log(`Failed to start recorder: ${error.message}`); this._removeRecorder(sender); - } finally { - invocation.return_value(GLib.Variant.new('(bs)', returnValue)); + if (error instanceof GLib.Error) { + invocation.return_gerror(error); + } else { + invocation.return_error_literal(ScreencastErrors, + ScreencastError.RECORDER_ERROR, + error.message); + } + + return; } recorder.connect('error', (r, error) => { + log(`Fatal error while recording: ${error.message}`); this._removeRecorder(sender); this._dbusImpl.emit_signal('Error', - new GLib.Variant('(s)', [error.message])); + new GLib.Variant('(ss)', [ + Gio.DBusError.encode_gerror(error), + error.message, + ])); }); } async ScreencastAreaAsync(params, invocation) { - let returnValue = [false, '']; - if (this._lockdownSettings.get_boolean('disable-save-to-disk')) { - invocation.return_value(GLib.Variant.new('(bs)', returnValue)); + invocation.return_error_literal(ScreencastErrors, + ScreencastError.SAVE_TO_DISK_DISABLED, + 'Saving to disk is disabled'); return; } const sender = invocation.get_sender(); if (this._recorders.get(sender)) { - invocation.return_value(GLib.Variant.new('(bs)', returnValue)); + invocation.return_error_literal(ScreencastErrors, + ScreencastError.ALREADY_RECORDING, + 'Service is already recording'); return; } @@ -613,7 +637,10 @@ export const ScreencastService = class extends ServiceImplementation { invocation); } catch (error) { log(`Failed to create recorder: ${error.message}`); - invocation.return_value(GLib.Variant.new('(bs)', returnValue)); + invocation.return_error_literal(ScreencastErrors, + ScreencastError.RECORDER_ERROR, + error.message); + return; } @@ -621,18 +648,29 @@ export const ScreencastService = class extends ServiceImplementation { try { await recorder.startRecording(); - returnValue = [true, filePath]; + invocation.return_value(GLib.Variant.new('(bs)', [true, filePath])); } catch (error) { log(`Failed to start recorder: ${error.message}`); this._removeRecorder(sender); - } finally { - invocation.return_value(GLib.Variant.new('(bs)', returnValue)); + if (error instanceof GLib.Error) { + invocation.return_gerror(error); + } else { + invocation.return_error_literal(ScreencastErrors, + ScreencastError.RECORDER_ERROR, + error.message); + } + + return; } recorder.connect('error', (r, error) => { + log(`Fatal error while recording: ${error.message}`); this._removeRecorder(sender); this._dbusImpl.emit_signal('Error', - new GLib.Variant('(s)', [error.message])); + new GLib.Variant('(ss)', [ + Gio.DBusError.encode_gerror(error), + error.message, + ])); }); } diff --git a/js/misc/dbusErrors.js b/js/misc/dbusErrors.js index a00288d2c..ff84f3140 100644 --- a/js/misc/dbusErrors.js +++ b/js/misc/dbusErrors.js @@ -43,3 +43,14 @@ export const ExtensionError = { }; export const ExtensionErrors = registerErrorDomain('Extensions', ExtensionError); + +export const ScreencastError = { + ALL_PIPELINES_FAILED: 0, + PIPELINE_ERROR: 1, + SAVE_TO_DISK_DISABLED: 2, + ALREADY_RECORDING: 3, + RECORDER_ERROR: 4, + SERVICE_CRASH: 5, +}; +export const ScreencastErrors = + registerErrorDomain('Screencast', ScreencastError); diff --git a/js/ui/screenshot.js b/js/ui/screenshot.js index f204d383d..e60f1c569 100644 --- a/js/ui/screenshot.js +++ b/js/ui/screenshot.js @@ -25,6 +25,7 @@ Gio._promisify(Shell.Screenshot.prototype, 'screenshot_area'); Gio._promisify(Shell.Screenshot.prototype, 'screenshot_stage_to_content'); Gio._promisify(Shell.Screenshot, 'composite_to_stream'); +import {ScreencastErrors, ScreencastError} from '../misc/dbusErrors.js'; import {loadInterfaceXML} from '../misc/fileUtils.js'; import {DBusSenderChecker} from '../misc/util.js'; @@ -1099,8 +1100,8 @@ export const ScreenshotUI = GObject.registerClass({ this._syncCastButton(); }); - this._screencastProxy.connectSignal('Error', - () => this._screencastFailed()); + this._screencastProxy.connectSignal('Error', (proxy, sender, [errorName, message]) => + this._screencastFailed(Gio.DBusError.new_for_dbus_error(errorName, message))); this._screencastProxy.connect('notify::g-name-owner', () => { if (this._screencastProxy.g_name_owner) @@ -1109,7 +1110,9 @@ export const ScreenshotUI = GObject.registerClass({ if (!this._screencastInProgress) return; - this._screencastFailed(); + this._screencastFailed( + new GLib.Error(ScreencastErrors, ScreencastError.SERVICE_CRASH, + 'Service crashed')); }); this._lockdownSettings = new Gio.Settings({schema_id: 'org.gnome.desktop.lockdown'}); @@ -1974,7 +1977,7 @@ export const ScreenshotUI = GObject.registerClass({ this._setScreencastInProgress(true); try { - const [success, path] = await method( + const [, path] = await method( GLib.build_filenamev([ /* Translators: this is the folder where recorded screencasts are stored. */ @@ -1986,8 +1989,7 @@ export const ScreenshotUI = GObject.registerClass({ _('Screencast from %d %t.webm'), ]), {'draw-cursor': new GLib.Variant('b', drawCursor)}); - if (!success) - throw new Error(); + this._screencastPath = path; } catch (error) { this._setScreencastInProgress(false); @@ -2024,7 +2026,9 @@ export const ScreenshotUI = GObject.registerClass({ this._showNotification(_('Screencast recorded')); } - _screencastFailed() { + _screencastFailed(error) { + console.error(`Screencast failed: ${error}`); + this._setScreencastInProgress(false); // Translators: notification title.