screenshot-manager: don't wedge on a failed screencopy - #7
Open
wayne-tbl wants to merge 1 commit into
Open
Conversation
maybe_screencopy_done() completed the D-Bus invocation itself on the failure path and returned without disposing self->frames. Two things follow from that. An internally requested screenshot -- the power button, or anything else reaching phosh_screenshot_manager_take_screenshot() with a NULL invocation -- has nothing to complete, so it tripped g_dbus_method_invocation_return_value_internal: assertion 'G_IS_DBUS_METHOD_INVOCATION (invocation)' failed And since self->frames stayed set -- on every failure path, not just the internal one -- phosh_screenshot_manager_do_screenshot() refused every later request with "Screenshot already in progress". So one failed screenshot disabled screenshots until phosh was restarted. screenshot_done() already handles both cases: it completes the invocation only when there is one, and disposes the frames either way. Route the failure through it. Reproduced on a FuriPhone FLX1 by shooting with the screen blanked, where wlr screencopy cannot read the output. The first request logged "Failed to copy output 'HWCOMPOSER-1'" and the assertion above; every request after it returned (false, '') immediately, without reaching screencopy at all.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
One failed screenshot currently disables screenshots until phosh is restarted.
maybe_screencopy_done()completes the D-Bus invocation itself on the failurepath and returns without disposing
self->frames:Two problems.
The invocation may be NULL. An internally requested shot — the power
button, or anything else calling
phosh_screenshot_manager_take_screenshot()— has none, so the failure path asserts:
self->framesis never disposed, on any failure path, D-Bus included.phosh_screenshot_manager_do_screenshot()refuses to start while it is set:so every screenshot after the first failure is rejected before it reaches
screencopy.
screenshot_done(), a few lines up in the same file, already handles bothcases — it completes the invocation only when there is one, and disposes the
frames either way. This routes the failure through it.
Reproduced
On a FuriPhone FLX1, by taking a screenshot with the screen blanked, where wlr
screencopy cannot read the hwcomposer output:
The next
org.gnome.Shell.Screenshot.Screenshotcall then returned(false, '')immediately, with noFailed to copy outputline of its own —i.e. it never reached screencopy.
That it is the stuck
framesrather than the blanked screen was confirmed bywaking the display and trying again: still
(false, ''), still instant, stillno log line. It stayed that way until phosh was restarted, after which the
identical call returned
(true, '/home/furios/Pictures/…png')and wrote avalid 1080x2416 PNG.
With the patch the failure disposes the frames, so the following screenshot
proceeds normally instead of inheriting the wedge.
Notes
forky.