Fix the two remaining UI-thread elevation freezes (profile save + installer)
Review finding, same class as the shipped install-hang fix: two paths still ran the elevated service helper synchronously on the UI thread, freezing the window and live audio for the duration (worst case minutes across two UAC prompts). 1) Service-profile save (MainForm.ConfigureServiceProfile): saving while the service runs did RunElevated(stop) + RunElevated(start) inline. Now: the restart runs on a background task, and tries a NO-UAC restart first - ServiceControl.TryRestartNoAdmin uses the start/stop rights the installer grants the installing account, so the normal case has no elevation prompt at all. Elevated verbs remain the fallback (service installed by a different account). Success is silent; only a failed restart reports back. The save popup now says the service is restarting. 2) App installer's optional service step (AppInstaller): the install + start-now calls ran RunElevated inline. The flow is sequential (can't fire-and-forget - the installer relaunches and exits afterwards), so RunElevatedResponsive runs the helper on a worker while a small modal "working..." shell pumps messages: UI and audio stay live, nothing can be double-triggered, NVDA announces the step, and the exit code still returns inline. Test: "No-admin service restart fails safe" - TryRestartNoAdmin against a missing service returns false promptly without throwing (that false routes callers onto the elevated fallback). The success path needs the real SCM + grant, covered by hand-test. Gate 47/47. Part of the review-fix batch; no release until the whole plan lands. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -186,7 +186,7 @@ internal static class AppInstaller
|
||||
if (wantService == DialogResult.Yes)
|
||||
{
|
||||
log?.Invoke("install: user opted to install the service too");
|
||||
var rc = ServiceControl.RunElevated(ServiceControl.InstallVerb);
|
||||
var rc = RunElevatedResponsive(owner, ServiceControl.InstallVerb, "Installing the RemSound service...");
|
||||
if (rc == 0)
|
||||
{
|
||||
// Offer to start it now — otherwise it only comes up at the next boot, so a
|
||||
@@ -199,7 +199,7 @@ internal static class AppInstaller
|
||||
if (startNow == DialogResult.Yes)
|
||||
{
|
||||
log?.Invoke("install: user opted to start the service now");
|
||||
var startRc = ServiceControl.RunElevated(ServiceControl.StartVerb);
|
||||
var startRc = RunElevatedResponsive(owner, ServiceControl.StartVerb, "Starting the RemSound service...");
|
||||
MessageBox.Show(owner,
|
||||
startRc == 0
|
||||
? "The RemSound service is running."
|
||||
@@ -226,6 +226,45 @@ internal static class AppInstaller
|
||||
Environment.Exit(0);
|
||||
}
|
||||
|
||||
/// <summary>Run an elevated service verb while keeping the UI thread ALIVE. The install flow is
|
||||
/// sequential (offer → install → offer start → start → relaunch), so a fire-and-forget task doesn't
|
||||
/// fit — but blocking the UI thread in WaitForExit froze the window and any live audio (the same
|
||||
/// hang class as the service install-freeze). This runs the elevated helper on a worker while a tiny
|
||||
/// modal "working…" shell pumps messages: the app stays responsive, the user can't double-trigger
|
||||
/// anything, NVDA announces what's happening, and the call still returns the exit code in-line.</summary>
|
||||
private static int RunElevatedResponsive(IWin32Window owner, string verb, string statusText)
|
||||
{
|
||||
var rc = -1;
|
||||
using var wait = new Form
|
||||
{
|
||||
Text = "RemSound",
|
||||
FormBorderStyle = FormBorderStyle.FixedDialog,
|
||||
StartPosition = FormStartPosition.CenterParent,
|
||||
MinimizeBox = false,
|
||||
MaximizeBox = false,
|
||||
ControlBox = false,
|
||||
ShowInTaskbar = false,
|
||||
AutoSize = true,
|
||||
AutoSizeMode = AutoSizeMode.GrowAndShrink,
|
||||
Padding = new Padding(24),
|
||||
AccessibleName = statusText,
|
||||
};
|
||||
wait.Controls.Add(new Label
|
||||
{
|
||||
Text = statusText + Environment.NewLine + "Windows may ask for administrator permission.",
|
||||
AutoSize = true,
|
||||
AccessibleName = statusText,
|
||||
});
|
||||
wait.Shown += (_, _) => Task.Run(() =>
|
||||
{
|
||||
try { rc = ServiceControl.RunElevated(verb); }
|
||||
catch { rc = -1; }
|
||||
try { wait.BeginInvoke(new Action(wait.Close)); } catch { /* already closed */ }
|
||||
});
|
||||
wait.ShowDialog(owner);
|
||||
return rc;
|
||||
}
|
||||
|
||||
// ---------------- uninstall ----------------
|
||||
|
||||
/// <summary>Options → "Uninstall RemSound from this PC", chosen from inside the running installed
|
||||
|
||||
@@ -2367,12 +2367,42 @@ public sealed class MainForm : Form
|
||||
logFile.Event($"service: profile saved (service logging {(dlg.ServiceLoggingEnabled ? "on" : "off")})");
|
||||
// Remove any stray copy the old design left in the user's profiles folder.
|
||||
try { profileStore?.Delete(ServiceControl.ServiceProfileTitle); } catch { /* best-effort */ }
|
||||
if (ServiceControl.Query() == ServiceState.Running)
|
||||
var restartNeeded = ServiceControl.Query() == ServiceState.Running;
|
||||
if (restartNeeded)
|
||||
{
|
||||
ServiceControl.RunElevated(ServiceControl.StopVerb);
|
||||
ServiceControl.RunElevated(ServiceControl.StartVerb);
|
||||
// Restart OFF the UI thread — the old inline RunElevated stop+start pair here blocked the
|
||||
// window (and its audio) through two UAC prompts; same bug class as the install hang.
|
||||
// No-UAC first: the installer granted this account start/stop rights, so a plain SCM
|
||||
// restart normally needs no elevation at all. Elevated verbs are the fallback (service
|
||||
// installed by a different account, grant missing). Only a FAILURE is reported back;
|
||||
// success needs no second popup.
|
||||
ServiceStore.AppendServiceEvent("restart requested (service profile changed)");
|
||||
Task.Run(() =>
|
||||
{
|
||||
var ok = ServiceControl.TryRestartNoAdmin();
|
||||
if (!ok)
|
||||
{
|
||||
ServiceControl.RunElevated(ServiceControl.StopVerb);
|
||||
ok = ServiceControl.RunElevated(ServiceControl.StartVerb) == 0;
|
||||
}
|
||||
ServiceStore.AppendServiceEvent(ok
|
||||
? "restart finished (new service profile is live)"
|
||||
: "restart FAILED after profile change");
|
||||
if (ok || IsDisposed) return;
|
||||
try
|
||||
{
|
||||
BeginInvoke(new Action(() => MessageBox.Show(this,
|
||||
"The service profile was saved, but the running service could not be restarted to pick it up. Use the Service menu to stop and start it.",
|
||||
AppName, MessageBoxButtons.OK, MessageBoxIcon.Warning)));
|
||||
}
|
||||
catch { /* window closing */ }
|
||||
});
|
||||
}
|
||||
MessageBox.Show(this, "Service profile saved.", AppName, MessageBoxButtons.OK, MessageBoxIcon.Information);
|
||||
MessageBox.Show(this,
|
||||
restartNeeded
|
||||
? "Service profile saved. The running service is restarting to pick it up."
|
||||
: "Service profile saved.",
|
||||
AppName, MessageBoxButtons.OK, MessageBoxIcon.Information);
|
||||
}
|
||||
catch (Exception ex)
|
||||
{
|
||||
|
||||
@@ -65,6 +65,7 @@ internal static class SelfTest
|
||||
RunStep(results, "Service app-yield token", ServiceInteractivePresence);
|
||||
RunStep(results, "Service sender parity (crypto + Opus frame)", ServiceSenderParity);
|
||||
RunStep(results, "Elevated helper: no pipe deadlock on flooded output", ServiceProcessCaptureNoDeadlock);
|
||||
RunStep(results, "No-admin service restart fails safe (missing service)", ServiceRestartNoAdminFailsSafe);
|
||||
RunStep(results, "Default-output follower (service follows Windows default)", DefaultOutputFollower);
|
||||
RunStep(results, "Default follower exclusivity (locks out specific cards)", DefaultFollowerExclusivity);
|
||||
RunStep(results, "Service profile isolation (location + hidden from pickers)", ServiceProfileIsolation);
|
||||
@@ -945,6 +946,20 @@ internal static class SelfTest
|
||||
return "follower flagged + sentinel shared with the app; service resolves it to the live default render endpoint";
|
||||
}
|
||||
|
||||
/// <summary>The no-UAC restart used after a service-profile save (TryRestartNoAdmin) must FAIL SAFE:
|
||||
/// against a service that doesn't exist it returns false, promptly, and never throws — that false is
|
||||
/// what routes the caller onto the elevated fallback. (The success path needs the real installed
|
||||
/// service + granted rights, so it's covered by hand-testing, not the gate.)</summary>
|
||||
private static string? ServiceRestartNoAdminFailsSafe()
|
||||
{
|
||||
var sw = Stopwatch.StartNew();
|
||||
var ok = ServiceControl.TryRestartNoAdmin("RemSoundSelfTestNoSuchService");
|
||||
sw.Stop();
|
||||
Check(!ok, "restarting a non-existent service must report false, not throw");
|
||||
Check(sw.ElapsedMilliseconds < 5000, $"the failure must be prompt (took {sw.ElapsedMilliseconds} ms)");
|
||||
return $"missing service → false in {sw.ElapsedMilliseconds} ms, no throw";
|
||||
}
|
||||
|
||||
/// <summary>Reproduces the install-hang condition and proves it's fixed: a child that floods BOTH
|
||||
/// stdout and stderr far past the ~4 KB pipe buffer (a big directory listing plus a failing dir). The
|
||||
/// old "read stderr to end, then stdout" order deadlocked exactly here (icacls /T over the 100-file
|
||||
|
||||
@@ -251,6 +251,33 @@ public static class ServiceControl
|
||||
return rc;
|
||||
}
|
||||
|
||||
/// <summary>Restart the service WITHOUT elevation, using the start/stop rights the installer granted
|
||||
/// the installing user (the SDDL ACE from <see cref="GrantUserStartStop"/>). Returns true when the
|
||||
/// service ends up Running. No UAC prompt, no elevated helper — so it's safe to run from a background
|
||||
/// thread after a profile save. Returns false (never throws) when the caller lacks rights, the service
|
||||
/// isn't installed, or a state wait times out; callers fall back to the elevated verbs then.
|
||||
/// <paramref name="serviceNameOverride"/> exists for the self-test (probe a non-existent name).</summary>
|
||||
public static bool TryRestartNoAdmin(string? serviceNameOverride = null)
|
||||
{
|
||||
try
|
||||
{
|
||||
using var sc = new ServiceController(serviceNameOverride ?? ServiceName);
|
||||
if (sc.Status is not (ServiceControllerStatus.Stopped or ServiceControllerStatus.StopPending))
|
||||
{
|
||||
sc.Stop();
|
||||
sc.WaitForStatus(ServiceControllerStatus.Stopped, TimeSpan.FromSeconds(15));
|
||||
}
|
||||
else
|
||||
{
|
||||
sc.WaitForStatus(ServiceControllerStatus.Stopped, TimeSpan.FromSeconds(15));
|
||||
}
|
||||
sc.Start();
|
||||
sc.WaitForStatus(ServiceControllerStatus.Running, TimeSpan.FromSeconds(15));
|
||||
return true;
|
||||
}
|
||||
catch { return false; }
|
||||
}
|
||||
|
||||
/// <summary>Starts the service. Must be run elevated. Returns 0 on success.</summary>
|
||||
public static int DoStart()
|
||||
{
|
||||
|
||||
Reference in New Issue
Block a user