1805802 - Make Windows remote server command-line handling async with `WndProc`

Status: REOPENED

(Toolkit :: Startup and Profile System, task, P3)

Description

[Nick Rishel [:nrishel]]

Currently remote server command-line handling is directly called in the window procedure. Ideally we should minimize the time spent with WndProc on the call stack as it can create risks of WndProc reentrancy from direct and indirect calls to SendMessage, and results in errors at a distance when attempting to interact with COM objects that require marshaling from another apartment/process.

One potential solution is offloading command-line handling to the main thread task queue after retrieving everything that requires the WndProc context.

Comment 1

[Nick Rishel [:nrishel]]

:mossop My idea was to apply the following patch to the above linked WndProc:

LRESULT CALLBACK WindowProc(HWND msgWindow, UINT msg, WPARAM wp, LPARAM lp) {
  if (msg == WM_COPYDATA) {
    WinRemoteMessageReceiver receiver;
    if (NS_SUCCEEDED(receiver.Parse(reinterpret_cast<COPYDATASTRUCT*>(lp)))) {
-      receiver.CommandLineRunner()->Run();
+      nsCOMPtr<nsICommandLineRunner> runner = receiver.CommandLineRunner();
+      NS_DispatchToMainThread(
+          NS_NewRunnableFunction("CommandLineRunner", [runner]() { runner->Run(); }));
    } else {
      NS_ERROR("Error initializing command line.");
    }

    // Get current window and return its window handle.
    nsCOMPtr<mozIDOMWindowProxy> win;
    GetMostRecentWindow(getter_AddRefs(win));
    return win ? (LRESULT)hwndForDOMWindow(win) : 0;
  }
  return DefWindowProcW(msgWindow, msg, wp, lp);
}

The intent here is just to defer CommandLineRunner long enough to get it off the WndProc call stack. Do you have a sense of the risk here? I was concerned there might be ordering dependencies, e.g. from remote clients relying on something having happened in the CommandLineRunner or GetMostRecentWindow relying on window creation occurring. I didn't see anything suspicious in ./mach try auto but some issues I'm concerned about would be probabilistic.

Comment 2

[Dave Townsend [:mossop]]

(In reply to Nick Rishel [:nrishel] from comment #1)

:mossop My idea was to apply the following patch to the above linked WndProc:

LRESULT CALLBACK WindowProc(HWND msgWindow, UINT msg, WPARAM wp, LPARAM lp) {
  if (msg == WM_COPYDATA) {
    WinRemoteMessageReceiver receiver;
    if (NS_SUCCEEDED(receiver.Parse(reinterpret_cast<COPYDATASTRUCT*>(lp)))) {
-      receiver.CommandLineRunner()->Run();
+      nsCOMPtr<nsICommandLineRunner> runner = receiver.CommandLineRunner();
+      NS_DispatchToMainThread(
+          NS_NewRunnableFunction("CommandLineRunner", [runner]() { runner->Run(); }));
    } else {
      NS_ERROR("Error initializing command line.");
    }

    // Get current window and return its window handle.
    nsCOMPtr<mozIDOMWindowProxy> win;
    GetMostRecentWindow(getter_AddRefs(win));
    return win ? (LRESULT)hwndForDOMWindow(win) : 0;
  }
  return DefWindowProcW(msgWindow, msg, wp, lp);
}

The intent here is just to defer CommandLineRunner long enough to get it off the WndProc call stack. Do you have a sense of the risk here? I was concerned there might be ordering dependencies, e.g. from remote clients relying on something having happened in the CommandLineRunner or GetMostRecentWindow relying on window creation occurring. I didn't see anything suspicious in ./mach try auto but some issues I'm concerned about would be probabilistic.

I wouldn't expect there to be any problem with this. The client is basically fire and forget, it sends the arguments via the window message but doesn't check for any kind of success and then just quits the process. Honestly looking at the documentation for SendMessageW we might be better off changing the client code to use PostMessage instead since we don't appear to care about any result from the message but that is a separate issue.

Comment 3

[Nick Rishel [:nrishel]]

Attachment: Bug 1805802 - Make Windows remote server command-line handling async with `WndProc` r=mossop

Previously remote server command-line handling was directly called in the window procedure. Ideally we should minimize the time spent with WndProc on the call stack as it can create risks of WndProc reentrancy from direct and indirect calls to SendMessage, and results in errors at a distance when attempting to interact with STA COM objects which require marshaling via SendMessage.

This resolves the issue by dispatching commandline processing to the main thread queue once we have extracted data scoped to the duration of the WndProc callback.

Comment 4

[Nick Rishel [:nrishel]]

Attachment: Bug 1805802 - Post: Send remote client commandline with `PostMessageW` instead of `SendMessageW` to make explicit that we won't use the result or order operations after message handling. r=mossop

Depends on D166844

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c2ddb25d9523
Make Windows remote server command-line handling async with `WndProc` r=mossop
https://hg.mozilla.org/integration/autoland/rev/700e3d729c8d
Post: Send remote client commandline with `PostMessageW` instead of `SendMessageW` to make explicit that we won't use the result or order operations after message handling. r=mossop

Comment 6

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/c2ddb25d9523
https://hg.mozilla.org/mozilla-central/rev/700e3d729c8d

Comment 7

[Cosmin Sabou [:CosminS]]

Backed out for causing Bug 1811252. It will be merged to central later on.

Backout link: https://hg.mozilla.org/integration/autoland/rev/5f0184d7afd9accaf9e32510b11a6282361347fb

Comment 8

[Marian-Vasile Laza]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/5f0184d7afd9accaf9e32510b11a6282361347fb

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:nrishel, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.