Add browser PID to BrowserProcessExitedEventArgs by Syul968 · Pull Request #826 · MicrosoftEdge/WebView2Feedback

Syul968 · 2021-01-16T01:30:25Z

Adding BrowserProcessId property to CoreWebView2BrowserProcessExitedEventArgs.

CoreWebView2Environment can attach to multiple browser processes throughout its lifetime. When repeatedly creating and closing a single WebView from the same environment, a new browser process could be created each time. The BrowserProcessId lets apps tracking the browser exit know the PID of the browser process for which the BrowserProcessExited event was raised.

oldnewthing · 2021-04-27T15:32:11Z

+
+  // Save PID of the browser process serving last WebView created from our
+  // CoreWebView2Environment.
+  CHECK_FAILURE(m_webView->get_BrowserProcessId(&m_newestBrowserPid));


What happens if you call get_BrowserProcessId too soon (before the WebView browser process has been created) or too late (after it crashed)?

If the creation completed handler (ICoreWebView2CreateCoreWebView2ControllerCompletedHandler) was called with an S_OK HRESULT, the connection has already been established to a running browser process. The full sample app has an S_OK check before this call, I'll update to reflect that here.

On browser process crash, the webview moves to its closed state and the call will fail. In this case, there will be a ProcessFailed event raised for the crash.

Is the transition to the Closed state synchronous or asynchronous? In other words, is it possible for the app to observe the Closed state before it receives the ProcessFailed event? If synchronous, then the "too late" failure is avoidable. if asynchronous, then apps cannot protect against the "too-late" case.

Synchronous.

Sounds good. Please make this clear in the documentation as appropriate.

Since this is about get_BrowserProcessId which is from a different interface, I don't think this API is the right place to document that. I added a comment to the sample code, though.

oldnewthing · 2021-04-27T15:43:55Z

-          // Watch for graceful browser process exit. Let ProcessFailed event
-          // handler take care of failed browser process termination.
-          if (kind == COREWEBVIEW2_BROWSER_PROCESS_EXIT_KIND_NORMAL)
+          CHECK_FAILURE(args->get_BrowserProcessId(&pid));


Does the system keep the process handle open for the lifetime of this event (to prevent PID recycling)? Otherwise, we cannot guarantee that "the PID of the new browser process will be different to the PID of the older process".

Sorry, I marked an earlier discussion around this as resolved, but it's still relevant here:

The new browser process can't start with the same PID because the environment instance tracking the old process keeps a handle open and so the process object stays valid; its PID can't be taken until the environment instance observing it closes the handle, which happens only after the queued event gets processed. [...]

oldnewthing · 2021-04-27T15:46:54Z

-            CleanupUserDataFolder();
+            // The exiting process is not the last in use. Do not attempt cleanup
+            // as we might still have a webview open over the user data folder.
+            MessageBox(


⟦boilerplate comment about blocking calls inside event handlers⟧

Note also that a real program probably shouldn't annoy the user by displaying messages about things the user doesn't care about.

This sample was originally written before the broader discussion around blocking calls inside event handlers. I will update.

In this case, the user data folder cleanup is only ever attempted upon user action (menu item selection).

Thanks please do.

Updated to post as async task.

oldnewthing · 2021-04-27T15:55:08Z

+            {
+              CHECK_FAILURE(
+                  m_webViewEnvironment->remove_BrowserProcessExited(m_browserExitedEventToken));
+              // Release the environment only after the handler is invoked.


If multiple apps are sharing a user data folder, and the first app shuts down its WebView, the first app will never receive a BrowserProcessExited (since the browser process is still being used by other apps). How does it know when it's safe to proceed with shutdown? Is it the apps' responsibility to coordinate among themselves to keep track of "Will the last one out the door please turn off the lights (i.e., wait for BrowserProcessExited)?"

Yes. The app can either coordinate with other processes sharing the user data folder or set a timer.

Do we already note this in the documentation? If not please do.

We had something about app processes coordination but not explicitly for waits, I added a comment about that.

MikeHillberg · 2021-04-30T02:40:34Z


 This event is raised for both expected and unexpected termination of the
-collection of processes, after all resources, including the user data folder, 
+collection of processes, after all resources, including the user data folder,


The plural vs singular of "process" is confusing. I think ProcessFailed is one process (rendering), and ProcessExited is a set of processes (more than just the rendering process). But for that collection, only one ProcessId is provided in the args.

Should this be BrowserProcessesExited (plural)? Or is that an implementation detail I shouldn't worry about? If the latter, then maybe remove "collection of processes" from the various pieces of text.

My interpretation: There is one browser process and N render processes. The browser process is the last one to exit, so you can assume that when it exits, the render processes have also already exited. (In the case where the browser process crashes, we kill all the render processes before raising this event, so as to preserve the illusion that the browser process exited last.)

Do we have a diagram somewhere that shows the processes?

Number one search hit for "Webview2 process" is Process Model which has diagrams.

Oh, thanks, that was a good idea. There's also a link to that page in this spec :(

So BrowserProcessExited is raised when everything goes away, ProcessFailed is raised when any process fails (including the Browser process). BrowserProcessExited isn't on the WebView object alongside ProcessFailed, because the Environment object is fundamentally associated with the Browser.

Makes sense, thanks.

The WebView2 Runtime has the same process model as Edge/Chromium. We document that here: Process model. The "browser process" is in charge of all other processes. The "BrowserProcess" part in BrowserProcessExited refers to that "browser process" in charge, and this is the PID being provided.

ProcessFailed indicates failure of one process of any kind (browser, render, GPU, etc.). BrowserProcessExited indicates exit of the "browser process" by crash or normal exit. The "browser process" is last to exit most of the times, but there are some cases in which some descendants might outlive it; the BrowserProcessExited event will only be raised upon those have exited too. This is the reason "collection of processes" was introduced during the earlier review.

I'll try to make this clearer in the documentation.

Sounds good. Please update documentation.

Add browser PID to event args

86f74ad

Syul968 added the API Proposal Review WebView2 API Proposal for review. label Jan 16, 2021

Syul968 requested a review from david-risney January 16, 2021 01:30

david-risney requested changes Jan 20, 2021

View reviewed changes

Comment thread specs/BrowserProcessExited.md

Comment thread specs/BrowserProcessExited.md Outdated

Comment thread specs/BrowserProcessExited.md

Comment thread specs/BrowserProcessExited.md Outdated

Add comments for PID use

e9e8435

oldnewthing reviewed Apr 27, 2021

View reviewed changes

MikeHillberg reviewed Apr 30, 2021

View reviewed changes

david-risney added the review completed WebView2 API Proposal that's been reviewed and now needs final update and push label Apr 30, 2021

lflores-ms added 6 commits May 4, 2021 14:40

Don't block from event handler

dec7367

Replace 'e.g.' for 'for example'

35271c5

Update ProcessFailed remarks

7b1afb8

Add explicit note for waits with shared UDF

3382224

Add check, comment for get_BrowserProcessId

a6315ad

Make process distinction more explicit

08a319b

Syul968 merged commit 63dbaa5 into master May 20, 2021

Conversation

Syul968 commented Jan 16, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants