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

Close CefSharp.BrowserSubprocess.exe if parent process exits #2375

Merged
merged 6 commits into from
Jun 9, 2018

Conversation

joaompneves
Copy link
Contributor

@joaompneves joaompneves commented Apr 26, 2018

Following the issue #2359 here is the "solution".

I tested this solution making the following changes in CefSharp.Wpf.Example\Views\BrowserTabView.xaml.cs:

browser.RenderProcessMessageHandler = new RenderProcessMessageHandler();

browser.FrameLoadStart += (sender, args) => {
    browser.Dispatcher.BeginInvoke((Action) (() => {
        Thread.Sleep(10);
        Environment.Exit(0);
    }));
};

@AppVeyorBot
Copy link

Copy link
Member

@amaitland amaitland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments inline

const string typePrefix = "--type=";
var typeArgument = args.SingleOrDefault(arg => arg.StartsWith(typePrefix));
var type = typeArgument.Substring(typePrefix.Length);
Task.Run(() => AwaitParentProcessExit(parentProcessId));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whilst the ThreadPool won't be used in the render process, it just doesn't seem appropriate to use a vanilla Task.Run, should probably add TaskCreationOptions.LongRunning, from memory it should spawn a new Thread and a quick check on stackoverflow suggests it will.

https://stackoverflow.com/a/10298761

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I though about adding that, but since this process is running mostly unmanaged code and is not using the ThreadPool, I skipped it. But its a small change, I will do it.

{
//main process probably died already, bailout
Debug.WriteLine(e);
return;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the purpose of this return?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assumed that the process would die by it self, but I'll remove it... its safer.


Task.Delay(1000); //wait a bit before exiting

Debug.WriteLine("BrowserSubprocess shutting down forcibly.");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should probably log to the native CEF Logging, trivial to add an wrapper method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just copied the same pattern that was already there.

@@ -119,13 +119,12 @@ public CefCustomScheme()
/// <returns>list of scheme objects</returns>
public static List<CefCustomScheme> ParseCommandLineArguments(IEnumerable<string> args)
{
var schemes = args.FirstOrDefault(a => a.StartsWith(CefSharpArguments.CustomSchemeArgument));
var schemes = args.GetArgumentValue(CefSharpArguments.CustomSchemeArgument);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tested these changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes... and I even spotted an existing "bug". Some of the parameters sent to the process are duplicated, that's why the FirstOrDefault works but not not SIngleOrDefault.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which paramaters are duplicated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those defined in CefSharpArguments

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, thanks

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

amaitland added a commit that referenced this pull request Apr 30, 2018
Improve command line parsing

Partial import of changes from #2375
amaitland added a commit that referenced this pull request Apr 30, 2018
Improve command line parsing

Partial import of changes from #2375
@amaitland
Copy link
Member

I don't have time to properly review this before the 65.0.0-pre01 release which will be out shortly.

I've manually cherry picked some pieces that are included in commit f2fcbf0

The process id will always be passed through and I've updated the command line parsing. This should give anyone wishing to try this feature out the ability to create their own BrowserSubprocess with minimal effort and include the Task that waits for the parent to exit.

When it comes time to merge this I'll resolve the conflicts.

@amaitland amaitland changed the title Close cefsharp subprocess when main process dies Close CefSharp.BrowserSubprocess.exe if parent process exits May 2, 2018
const string typePrefix = "--type=";
var typeArgument = args.SingleOrDefault(arg => arg.StartsWith(typePrefix));
var type = typeArgument.Substring(typePrefix.Length);
Task.Factory.StartNew(() => AwaitParentProcessExit(parentProcessId), TaskCreationOptions.LongRunning);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does not take a thread

var parentProcess = Process.GetProcessById(parentProcessId)
if(parentProcess != null)
{
  parentProcess.EnableRaisingEvents = true;
  parentProcess.Exited += (o,a) => 
  {
    Debug.WriteLine("BrowserSubprocess shutting down forcibly.");
    Environment.Exit(0);
  }
}

Debug.WriteLine(e);
}

Task.Delay(1000); //wait a bit before exiting
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

await Task.Delay(1000);

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@amaitland amaitland merged commit 274e841 into cefsharp:master Jun 9, 2018
amaitland added a commit that referenced this pull request Jun 11, 2018
…k to monitor parent if command line arg present

Follow up to #2375
amaitland pushed a commit that referenced this pull request Jun 20, 2018
* Close cefsharp subprocess if main process dies

* Code review

* Await before exiting

* Update Program.cs
amaitland added a commit that referenced this pull request Jun 20, 2018
…k to monitor parent if command line arg present

Follow up to #2375
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

Successfully merging this pull request may close these issues.

4 participants