[DO NOT MERGE] output: return on wlr_output_commit failures

[RedSoxFan]

Related to #4916
Fixes the original coredump backtrace in #4904

There were two wlr_output_commit calls that were not being checked. If
the commits fail, apply_output_config should early return false instead
of continuing to apply more. In one of the two cases, it was possible
for output_enable to be called immediately after a commit failed for
wlr_output_enable. In some cases, wlroots will destroy the output when
it fails to enable. When this happens, it was possible for output_enable
to call apply_output_config again for the destroyed output. Since this
is a use-after-free, the wlr_output has already been reset to NULL
before freeing. This could results in wlr_output_enable or any of the
other wlr_output_* calls being called with NULL as the wlr_output.

[prometheanfire]

just for completeness, here's the log with it https://gist.github.com/fd71537afd99123df9dba9cb484dfc68 and the behaviour I see is that I cannot enable other outputs at all now. I know you said you would continue to work on this, but wanted the log just in case it's useful further on (if it's forgotten from irc or something).

[RedSoxFan]

@emersion Is there any value in having the wlr_output_enable call in it's own commit before calling output_enable, which basically immediately calls apply_output_config again? When the call was originally added, the structure of the rest of the function was different and the output had to be enabled before other things occurred. With the addition of the atomic output commits, I'm not sure the call is needed at all now.

I think changing:

sway/sway/config/output.c

Lines 326 to 338 in 9d827ef

	} else if (!output->enabled) {
	// Output is not enabled. Enable it, output_enable will call us again.
	if (!oc || oc->dpms_state != DPMS_OFF) {
	sway_log(SWAY_DEBUG, "Enabling output %s", oc->name);
	wlr_output_enable(wlr_output, true);
	wlr_output_commit(wlr_output);
	}
	return output_enable(output, oc);
	}
	if (!oc || oc->dpms_state != DPMS_OFF) {
	sway_log(SWAY_DEBUG, "Turning on output %s", wlr_output->name);
	wlr_output_enable(wlr_output, true);

To:

} else if (!output->enabled) { 
 	// Output is not enabled. Enable it, output_enable will call us again.
 	return output_enable(output, oc); 
 } 
  
 if (!oc || oc->dpms_state != DPMS_OFF) { 
 	sway_log(SWAY_DEBUG, "Turning on output %s", wlr_output->name); 
 	wlr_output_enable(wlr_output, true); 

Would prevent the issue and remove a seemingly extra wlr_output_enable and wlr_output_commit.

[emersion]

In some cases, wlroots will destroy the output when
it fails to enable.

I think this is a wlroots bug: #4916 (comment)

Is there any value in having the wlr_output_enable call in it's own commit before calling output_enable, which basically immediately calls apply_output_config again?

No. The PR switching to the atomic output API just tries to replicate the old behavior to reduce the number of potential bugs. Performing a single commit while configuring the output would be a lot nicer, but we need to be careful not to submit invalid state (e.g. we shouldn't try to set a mode and disable an output at the same time).

[emersion]

swaywm/wlroots#2001

[RedSoxFan]

Superseded by swaywm/wlroots#2001