Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BrowserSubprocess - Update shutdown priority (SetProcessShutdownParameters) #3155

Closed
ekalchev opened this issue Jun 16, 2020 · 25 comments
Closed
Assignees
Milestone

Comments

@ekalchev
Copy link

ekalchev commented Jun 16, 2020

Bug Report

Delete this line and everything above, and then fill in the details below.

  • What version of the product are you using?
    81

  • What architecture x86 or x64?
    x64

  • On what operating system?
    Win10

  • Are you using WinForms, WPF or OffScreen?
    WinForms

  • What steps will reproduce the problem?
    We have MSI installer that try to stop the app process when it is about to make an install/update. A standard dialog is shown with the list of processes that must be stopped before install/update to complete. After the update to CefSharp 81 (from 73) we started seeing Browser subprocess in the list of processes that MSI need to kill. With version 73 the subprocess was not listed. Now with version 81, MSI installer fails to install/update because it fails to stop the processes. We believe the problem is in browser subprocess, because if we kill it manually the MSI install can complete.

@amaitland
Copy link
Member

Generally speaking there has been some refactoring lately, nothing that I can think of that would influence this.

The default CefSharp.BrowserSubprocess.exe should terminate itself when the parent process exits.

https://github.com/cefsharp/CefSharp/blob/cefsharp/83/CefSharp.BrowserSubprocess.Core/BrowserSubprocessExecutable.h#L110
https://github.com/cefsharp/CefSharp/blob/cefsharp/83/CefSharp/Internals/ParentProcessMonitor.cs#L24

Is you application closing itself as expected???

  • We believe the problem is in browser subprocess

What testing have you done to confirm this exactly? Have you tried packaging the CEF Sample Application and confirming that it works as expected? It's possible that Chromium has changed it's behaviour.

@ekalchev
Copy link
Author

ekalchev commented Jul 1, 2020

Our testing was comparing CefSharp 73 with CefSharp 81. With Cef sharp 73 MSI installer shows a dialog with the processes it needs to close in order to complete the installation. v73 doesn't list BrowserSubprocess in this list. With v81 BrowserSubprocess is part of this list and it is first. I suspect MSI is sending shutdown signals or trying to kill BrowserSubprocess unsuccessfully. Probably MSI is trying to close the processes sequentially and since BrowserSubprocess is the first in the list and it can't close it it won't get to the actual app process that BrowserSubprocess is child to. I am looking for a change in the code where BrowserSubprocess was registered as child process to the main app process.

@amaitland
Copy link
Member

I'd still suggest trying to package the cefclient sample application, you may need to go digging through the CEF/chromium source if the behaviour reproduces.

You can compare the source history for the CefSharp.BrowserSubprocess.Core and CefSharp.BrowserSubprocess projects. There's nothing I can think of that would be influencing the behaviour from a CefSharp point of view.

@amaitland
Copy link
Member

You can probably include the cefclient.exe from the matching CEF distribution and use that as the browsersubprocess for a simpler test, you'll only be able to do basic browsing, that should be sufficient for testing purposes.

@ekalchev
Copy link
Author

You can probably include the cefclient.exe from the matching CEF distribution and use that as the browsersubprocess for a simpler test, you'll only be able to do basic browsing, that should be sufficient for testing purposes.

Will try that.

@no-response
Copy link

no-response bot commented Jul 29, 2020

This issue has been automatically closed because there has been no response to our request for more information from the original author. With only the information that is currently in the issue, we don't have enough information to take action. Please reach out if you have or find the answers we need so that we can investigate further.

@no-response no-response bot closed this as completed Jul 29, 2020
@GSonofNun
Copy link
Contributor

We recently realized this happens with our application and installer as well. From what I can gather, the installer asks Windows to shut down the application so it can update the assembly files. Windows uses the Restart Manager to query and instruct applications to close. According to the documentation, it will

  1. Send a WM_QUERYENDSESSION message to the application
    a. If the application returns false, it will cancel the shutdown request
    b. If the application returns true, it will continue
  2. Send a WM_ENDSESSION message to the application.
  3. If the application hasn't shut down after a few seconds, it will send a WM_CLOSE message, which will terminate the process.

Since the shutdown request seems to be canceled (step 1.a), that would lead me to believe that someone is returning false when they receive the WM_QUERYENDSESSION message. (However, I can't find anywhere in Cef or CefSharp's code that it is doing that)

If we don't create a ChromiumBrowser or initialize CefSharp, then our application will receive and respond appropriately to the messages. However, once Cef has been initialized, we no longer receive those particular messages, even though we continue receiving other standard window messages

I've been trying to fix this for a day and a half, and have gotten no closer to fixing it, other than gaining better knowledge on how it is supposed to work.

CEF4Delphi looks to have had a similar problem, but I wasn't able to get anything useful from their solution. I added a window message handler in ChromiumWebBrowser to receive the WM_QUERYENDSESSION and WM_ENDSESSION messages, but after testing and still not receiving the messages, I realized that I was just attaching to the event on the same window handle as my main application, so of course I wouldn't see anything different. (Also, I don't like their solution because it forces their application to close when receiving the WM_QUERYENDSESSION message, when it should really do it when it receives the WM_ENDSESSION message.

I apologize for the dump of information, if you have any ideas or questions, feel free to voice them.

@salvadordf
Copy link

salvadordf commented Dec 6, 2021

Sorry for intruding in this project. The solution in CEF4Delphi was this :
// Subprocesses will be the last to be notified about the Windows shutdown.
// The main browser process will receive WM_QUERYENDSESSION before the subprocesses
// and that allows to close the application in the right order.
SetProcessShutdownParameters(0x100, SHUTDOWN_NORETRY);

Chromium also calls SetProcessShutdownParameters :
https://source.chromium.org/chromium/chromium/src/+/main:chrome/app/main_dll_loader_win.cc?q=SetProcessShutdownParameters&ss=chromium

@amaitland amaitland changed the title MSI installer fails because of Browser.Subprocess Core - Update BrowserSubprocess shutdown priority Dec 7, 2021
@amaitland amaitland reopened this Dec 7, 2021
@amaitland
Copy link
Member

Sorry for intruding in this project. The solution in CEF4Delphi was this :

@salvadordf Thanks, greatly appreciated.

If I understand correctly for the sub processes we should call SetProcessShutdownParameters with a low value.

All processes start at shutdown level 0x280.

As per https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-setprocessshutdownparameters

Chromium also calls SetProcessShutdownParameters :
https://source.chromium.org/chromium/chromium/src/+/main:chrome/app/main_dll_loader_win.cc?q=SetProcessShutdownParameters&ss=chromium

Looking through the Chromium source there appears to be two different values used,

//For crashpad
if (!SetProcessShutdownParameters(0x100, SHUTDOWN_NORETRY))
    PLOG(ERROR) << "SetProcessShutdownParameters";

//For all others
//The browser uses the default shutdown order, which is 0x280.
const int kNonBrowserShutdownPriority = 0x280;
::SetProcessShutdownParameters(kNonBrowserShutdownPriority - 1,
                                   SHUTDOWN_NORETRY);

@salvadordf Is using 0x100 for all processes working as expected for you? It appears that Chromium uses 0x100 only for crashpad and 0x280 - 1 for other subprocesses.

@amaitland amaitland self-assigned this Dec 7, 2021
@amaitland
Copy link
Member

// Shut down as late as possible relative to programs we're watching.

Based on the comment in the Chromium source I'd be inclined to set Crashpad to a value lower than the other sub processes.

@amaitland amaitland added this to the 96.0.x milestone Dec 7, 2021
@amaitland amaitland changed the title Core - Update BrowserSubprocess shutdown priority BrowserSubprocess - Update shutdown priority (SetProcessShutdownParameters) Dec 7, 2021
@salvadordf
Copy link

salvadordf commented Dec 7, 2021

@amaitland The current 0x100 value works fine but any value lower than the default should also work. I guess Chromium developers wanted to be sure the crashpad is closed last, even after the subprocesses.... but you're right, Even if we don't use the crashpad I should use the same value as Chromium for the subprocesses.

@GSonofNun
Copy link
Contributor

@salvadordf Thank you for sharing that info!

@amaitland I'll pull that commit into my local CefSharp repo and give it a try. I'll check in later with the results.

@GSonofNun
Copy link
Contributor

No luck. ☹️ Something is still preventing the main application from receiving the messages.

Also, the comments you left on your (@amaitland) commit say "Lower the shutdown priority so the browser process is shutdown first", but the documentation on the SetProcessShutdownParameters say:

The system shuts down processes from high dwLevel values to low.

So I think lowering the shutdown priority actually makes them shut down after the main process? With that in mind, I experimented by increasing the priority above 0x280 and still had the problem.

@amaitland
Copy link
Member

So I think lowering the shutdown priority actually makes them shut down after the main process?

@GSonofNun Correct, this is the desired behaviour, matches that used by Chromium. Ideally your application (browser process) will shutdown gracefully and terminate the sub processes.

Shutting the sub process down first would likely cause chromium to attempt to relaunch them.

@GSonofNun
Copy link
Contributor

Oh, I think I see my confusion. The "browser" referenced in your comments on the commit were referring to what would be the main application using CefSharp?

@amaitland
Copy link
Member

Oh, I think I see my confusion. The "browser" referenced in your comments on the commit were referring to what would be the main application using CefSharp?

@GSonofNun Correct. See https://github.com/cefsharp/CefSharp/wiki/General-Usage#processes

Probably MSI is trying to close the processes sequentially and since BrowserSubprocess is the first in the list and it can't close it it won't get to the actual app process that BrowserSubprocess is child to. I am looking for a change in the code where BrowserSubprocess was registered as child process to the main app process.

As per #3155 (comment)

The order should hopefully be changed, so the Browser Process (main application) is closed first. Subsequently the BrowserSubProcesses should terminate, they will forcefully close themselves when the Browser Process closes so they aren't left lingering if shutdown wasn't successful.

This should hopefully resolve the original issue raised by @ekalchev

Something is still preventing the main application from receiving the messages.

What exactly are you doing? What does your code look like? Not clear this is related to the original issue. Relevant events appear to be raised with a cursory test for me.

Using https://github.com/cefsharp/CefSharp.MinimalExample and rmlogotest.exe I can see that the FormClosing event is raised with a shutdown reason code and SystemEvents.SessionEnding event is fired.

@GSonofNun
Copy link
Contributor

I did some more testing. We are listening to all windows messages and writing them to a log file.
For reference, here's what it looks like.

private IntPtr WindowProc(IntPtr hwnd, int msg, IntPtr wParam, IntPtr lParam, ref bool handled)
{
    Logger.Log(EventLevel.Informational, $"Windows Message: {msg} {wParam} {lParam}");

    if (msg == WindowsMessages.WM_QUERYENDSESSION)
    {
        Logger.Log(EventLevel.Informational, "Windows Restart Manager: WM_QUERYENDSESSION message received");
        return IntPtr.Zero;
    }
    else if (msg == WindowsMessages.WM_ENDSESSION)
    {
        Logger.Log(EventLevel.Informational, "Windows Restart Manager: WM_ENDSESSION message received");
        if (wParam.ToInt64() == 0)
        {
            Logger.Log(EventLevel.Informational, "Windows Restart Manager: Canceling close");
        }
        else
        {
            Logger.Log(EventLevel.Informational, "Windows Restart Manager: Ending sessions");
            Environment.Exit(0);// Close Application TODO: Save user state
        }
        handled = true;
        return IntPtr.Zero;
    }

    return IntPtr.Zero;
}
  • Test 1: Debugging our application in Visual Studio and putting breakpoints in the message handler method.

    • Using rmlogotest.exe with our app instance's pid causes it to correctly receive and handle WM_QUERYENDSESSION and WM_ENDSESSION, and our application restarts as it should.
  • Test 2: Creating an installer for our app, installing it, and opening it.

    • Using rmlogotest.exe with our app instance's pid results in our application receiving the WM_QUERYENDSESSION message, then a delay, then receiving the WM_CLOSE message, which closes the window, but leaves the process running in the background, and needs to be killed via Task Manager.
      Log:

      INFO - Windows Message: 17 0 1
      INFO - Windows Restart Manager: WM_QUERYENDSESSION message received
      INFO - Windows Message: 16 0 0
      INFO - Application closed

      RMLOGOTEST results:
      image

  • Test 3: Creating an installer for our app, installing it, and opening it.

    • Open installer again, select "Repair", it detects the application is running, lists the processes (main process and some browser subproceses), and asks if it can close them.
    image

    I click "OK" and it tries to close them, but after a few seconds reports that it was unable to close them.
    image

    And there are nothing in the log that indicates the application received the WM_QUERYENDSESSION message.

@amaitland
Copy link
Member

For reference, here's what it looks like.

Are you using WPF? Please provide more detail.

What is the reason for calling Environment.Exit? Doesn't WPF handle the messages gracefully?

Test 1: Debugging our application in Visual Studio and putting breakpoints in the message handler method.

How does it behave if you run directly from your bin folder?

If you create an installer for the minimum example how does it behave? What installer are you using? Just saying installer is vaigue.

https://github.com/cefsharp/CefSharp.MinimalExample

@GSonofNun
Copy link
Contributor

Are you using WPF?

Sorry, yes. A WPF application using .NET 6.

What is the reason for calling Environment.Exit? Doesn't WPF handle the messages gracefully?

I don't think that matters. Removing that line makes no difference. It was just temporarily there to simulate our application saving state and shutting down.

How does it behave if you run directly from your bin folder?

I behaves like Test 2. I receive WM_QUERYENDSESSION, followed by WM_CLOSE, which closes the window but leaves the process "running" in the background.

What installer are you using?

We're using an installer generated by Wix

I'll try packaging the CefSharp.MinimalExample and get back to you.

amaitland added a commit that referenced this issue Dec 9, 2021
…x200

- Change to 0x200 which is low end of 'Application reserved "in between" shutdown range.'
- Remove logging as it wasn't working, must be too early to use the LOG(INFO) CEF macro

Test with a GetProcessShutdownParameters in debugger shows working e.g.

if (SetProcessShutdownParameters(0x200, SHUTDOWN_NORETRY))
{
	DWORD level = 0;
	DWORD flags = 0;
	GetProcessShutdownParameters(&level, &flags);
}

Issue #3155
amaitland added a commit that referenced this issue Dec 9, 2021
…x200

- Change to 0x200 which is low end of 'Application reserved "in between" shutdown range.'
- Remove logging as it wasn't working, must be too early to use the LOG(INFO) CEF macro

Test with a GetProcessShutdownParameters in debugger shows working e.g.

if (SetProcessShutdownParameters(0x200, SHUTDOWN_NORETRY))
{
	DWORD level = 0;
	DWORD flags = 0;
	GetProcessShutdownParameters(&level, &flags);
}

Issue #3155
@GSonofNun
Copy link
Contributor

Well, my day has been disappointing. ☹️

I spent most of the day figuring out how to build the minimal example into an installer using Wix (I'm not super familiar with Wix, so I had to re-learn how it works). I finally got it to work, only to have results that disappoint me, but will probably please you. 🥲

While the installed version of the minimal example is running, if I use rmlogotest.exe, it closes perfectly, as it should.

However, while it is running, if I re-run the installer to repair it, and give it permission to shut down the running application, it fails.

Which leads me to wonder if it is a problem with Wix and a multi-process application. Maybe it isn't sending the shutdown message to the correct application. ¯\_(ツ)_/¯

@amaitland
Copy link
Member

I spent most of the day figuring out how to build the minimal example into an installer using Wix (I'm not super familiar with Wix, so I had to re-learn how it works)

Can you fork the minimum example and push your changes to GitHub so I can have a look. Preferably squashed into a single commit. Thanks.

@GSonofNun
Copy link
Contributor

Here you go. Let me know if you have any questions.
https://github.com/GSonofNun/CefSharp.MinimalExample/tree/add-installer

@GSonofNun
Copy link
Contributor

Update: Because of a change we're making to our solution, I remembered that we are building our application as a .NET 6 Self-Contained application. So I updated our application to self-host the browser subprocess, and now it responds to the installer's shutdown request as expected.

@amaitland
Copy link
Member

Here you go. Let me know if you have any questions.
https://github.com/GSonofNun/CefSharp.MinimalExample/tree/add-installer

@GSonofNun Thanks for the example.

So I updated our application to self-host the browser subprocess, and now it responds to the installer's shutdown request as expected.

I had a suspicion this might be the case, was on my list of cases to test. From what I can tell Wix doesn't like when there are two different exes running from the same folder. Using Spy++ to check the messages sent to the hwnd and I don't a WM_QUERYENDSESSION message.

After much digging and experimentation calling WixCloseApplications earlier in the installer sequence works just fine. I've pushed an updated version of your example to https://github.com/cefsharp/CefSharp.MinimalExample/tree/wix-installer

I don't know enough about Wix to know exactly what's going on, if anyone is interested in following up with the Wix Team then please post a link back here for reference.

A quick example using https://github.com/oleg-shilo/wixsharp to package the WinForms version for reference.

using System;
using WixSharp;
using Microsoft.Deployment.WindowsInstaller;
using System.IO;

namespace CefSharp.MinimalExample.WinForms.WixSharp
{
    public static class Program
    {
        public static void Main(string[] args)
        {
            var bitness = Environment.Is64BitProcess ? "x64" : "x86";
#if DEBUG
            const string Build = "Debug";
#else
            const string Build = "Release";
#endif
            var buildPath = Path.GetFullPath(@"..\CefSharp.MinimalExample.WinForms\bin.net472\" + bitness + @"\" + Build + @"\net472");
            var project = new Project("CefSharp.MinimalExample.WinForms",
                          new Dir(@"%ProgramFiles%\CefSharp.MinimalExample.WinForms",
                              new DirFiles($"{buildPath}\\*.*"),
                              new Dir("locales", new DirFiles($"{buildPath}\\locales\\*.*")),
                              new Dir("swiftshader", new DirFiles($"{buildPath}\\swiftshader\\*.*"))),
                          new CustomActionRef("WixCloseApplications", When.Before, Step.CostFinalize, new Condition("VersionNT > 400")),
                          new CloseApplication(new Id("CefSharp.MinimalExample.WinForms"), "CefSharp.MinimalExample.WinForms.net472.exe")
                          {
                              Timeout = 15,
                              EndSessionMessage = true,
                              RebootPrompt = false,
                          },
                          new Property("MsiLogging", "vocewarmup"));
            
            project.GUID = new Guid("9007D203-B084-482F-BC23-5F4BF82EE679");
            project.Platform = Platform.x64;

            Compiler.BuildMsi(project);
        }
    }
}

I'm going to close this issue now. If someone contacts the Wix Team and it's determined that further code changes are required then it's probably best to create a new issue and reference this one.

@amaitland
Copy link
Member

See #3407 (comment) for example of self hosting the browser sub process (using your own applications exe instead of CefSharp.BrowserSubprocess.exe).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants