Bug 99817 – Add infrastructure for NetworkProcess management

• Component: WebKit2
• Status: RESOLVED FIXED

[Brady Eidson]

Add infrastructure for NetworkProcess management

All of this infrastructure closely follows PluginProcess infrastructure.

While there's no immediate plans to implement multiple NetworkProcesses the support is in place.  Each is identified by a String identifier.  The "default" NetworkProcess has an empty identifier.

With all of this support code in place, the WebProcess itself will now ensure it has a connection to the default NetworkProcess when it is initialized, and the various classes and processes in play will all be notified one someone else crashes.

The NetworkProcess still does nothing for now.

[Brady Eidson]

Created attachment 169564 [details]
Patch v1 - Get EWS rolling

Proposed patch.

It's large, but it's also largely boilerplate and should be quickly reviewable by those familiar with PluginProcess infrastructure.

[Brady Eidson]

(Glaring at you, Anders)

[Early Warning System Bot]

Comment on attachment 169564 [details]
Patch v1 - Get EWS rolling

Attachment 169564 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14462340

[Brady Eidson]

Created attachment 169565 [details]
Patch v2 - Qt, don't break!

[Early Warning System Bot]

Comment on attachment 169565 [details]
Patch v2 - Qt, don't break!

Attachment 169565 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14457343

[Brady Eidson]

Created attachment 169569 [details]
Patch v3 - And again...

[Anders Carlsson]

(In reply to comment #2)
> (Glaring at you, Anders)

I don't really have time to do a lenghty review today, and I'd rather have Sam review it. However, I can take a look at it this weekend if he hasn't gotten to it first.

[Brady Eidson]

(In reply to comment #7)
> (In reply to comment #2)
> > (Glaring at you, Anders)
> 
> I don't really have time to do a lenghty review today, and I'd rather have Sam review it. However, I can take a look at it this weekend if he hasn't gotten to it first.

He's on vacation today...  :(

[Alexey Proskuryakov]

Comment on attachment 169569 [details]
Patch v3 - And again...

View in context: https://bugs.webkit.org/attachment.cgi?id=169569&action=review

One general note. Since NetworkProcess is more trusted than WebProcess, we need to detect malformed messages, and do the same thing that we do when we get a malformed message in UI process - namely, crash Safari itself. Because we don't want to drag on when WebProcess has parasites.

r=me, mostly on the assumption that this is is closely modeled after existing code. Sam and Anders should take a look post-landing.

> Source/WebKit2/ChangeLog:10
> +        Add proper handling of crashes so any of the 3+ processes in the dance are notified if any of the others crash.

Presumably other processes are immediately terminated when UI process quits or crashes?

I have a concern about this design. When NetworkProcess crashes, both WebProcess and UI process will be notified. Clearly, WebProcess will want a new connection. But UI process may not have handled the crash notification yet. What is the protection against such races?

> Source/WebKit2/ChangeLog:24
> +        * NetworkProcess/NetworkConnectionToWebProcess.cpp: Added.

I do not understand the naming scheme here. We have a large matrix of connection classes, which need to be consistently named.

> Source/WebKit2/ChangeLog:80
> +        * UIProcess/Network/NetworkProcessProxy.messages.in: Copied from Source/WebKit2/NetworkProcess/NetworkProcess.messages.in.

This svn copy doesn't seem to make a whole lot of sense.

> Source/WebKit2/ChangeLog:118
> +        (WebKit::NetworkProcessConnectionManager::defaultConnection):

I'm not sure that we want to have this code imply that there can be multiple connections already. We don't use it now, and we may never will in the future. Having concurrent connections to processes with different permissions is not that much different from having a connection to a single process with a union of these permissions - an exploited Web process can just choose which network process to talk to.

> Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.cpp:2
> + * Copyright (C) 2012 Apple Inc. All rights reserved.

Was this all written from scratch in 2012?

Ditto for all the new files you add.

> Source/WebKit2/NetworkProcess/NetworkProcess.cpp:34
> +#include "NetworkProcessProxyMessages.h"
> +#include "NetworkConnectionToWebProcess.h"

Alphabetic ordering.

> Source/WebKit2/UIProcess/Network/NetworkProcessManager.cpp:13
> + * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY

Apple COMPUTER?

> Source/WebKit2/UIProcess/Network/NetworkProcessManager.cpp:16
> + * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL APPLE COMPUTER, INC. OR

Apple COMPUTER?

Ditto for all the new files you added.

> Source/WebKit2/UIProcess/Network/NetworkProcessManager.cpp:64
> +

Blank line.

> Source/WebKit2/UIProcess/WebProcessProxy.messages.in:49
> +    GetNetworkProcessConnection(WTF::String identifier) -> (CoreIPC::Attachment connectionHandle) Delayed

Is this unique identifier (such as a autoincremented number), or some kind of a "class"? The argument name could be more descriptive.

> Source/WebKit2/WebProcess/Network/NetworkProcessConnectionManager.cpp:50
> +NetworkProcessConnection* NetworkProcessConnectionManager::getNetworkProcessConnection(const String& networkProcessIdentifier)

Seems that these strings should be at least atomic (but then again, perhaps this code can be removed for now).

[Brady Eidson]

(In reply to comment #9)
> (From update of attachment 169569 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=169569&action=review
> 
> One general note. Since NetworkProcess is more trusted than WebProcess, we need to detect malformed messages, and do the same thing that we do when we get a malformed message in UI process - namely, crash Safari itself. Because we don't want to drag on when WebProcess has parasites.
> 
> r=me, mostly on the assumption that this is is closely modeled after existing code. Sam and Anders should take a look post-landing.
> 
> > Source/WebKit2/ChangeLog:10
> > +        Add proper handling of crashes so any of the 3+ processes in the dance are notified if any of the others crash.
> 
> Presumably other processes are immediately terminated when UI process quits or crashes?
> 
> I have a concern about this design. When NetworkProcess crashes, both WebProcess and UI process will be notified. Clearly, WebProcess will want a new connection. But UI process may not have handled the crash notification yet. What is the protection against such races?

I believe this is handled actually.  The Web->UI request for a new connection gets queued up in the UIProcess, which then queues the reply before asynchronously asks the NetworkProcess for a new connection.

When the UIProcess later gets the message that the NetworkProcess crashed, it goes through the pending replies telling them they get no connection.

> > Source/WebKit2/ChangeLog:24
> > +        * NetworkProcess/NetworkConnectionToWebProcess.cpp: Added.
> 
> I do not understand the naming scheme here. We have a large matrix of connection classes, which need to be consistently named.

I agree we need more consistent naming, but I don't think the current naming scheme is either consistent or obvious.

For example, in the PluginProcess version of all of this code, the "Plugin Processes connection to a WebProcess" is called "WebProcessConnection"  That tells me nothing!

By naming the NetworkProcess equivalent "NetworkConnectionToWebProcess" I hoped to at least be precise if not concise.

Personally I think some of the others should be renamed - Notably "WebProcessConnection" would be "PluginConnectionToWebProcess" under this scheme.

> > Source/WebKit2/ChangeLog:80
> > +        * UIProcess/Network/NetworkProcessProxy.messages.in: Copied from Source/WebKit2/NetworkProcess/NetworkProcess.messages.in.
> 
> This svn copy doesn't seem to make a whole lot of sense.

I don't even know how it got there.  I didn't explicitly copy it.  I'll punch around git a bit and see if I can't fix that.

> > Source/WebKit2/ChangeLog:118
> > +        (WebKit::NetworkProcessConnectionManager::defaultConnection):
> 
> I'm not sure that we want to have this code imply that there can be multiple connections already. We don't use it now, and we may never will in the future. Having concurrent connections to processes with different permissions is not that much different from having a connection to a single process with a union of these permissions - an exploited Web process can just choose which network process to talk to.

As discussed on IRC, my motivation here was not to have different processes with different privileges but rather different processes with different *purposes*.  A "normal" network process, one or more "private browsing" network processes, or a "downloads" process for when a connection is initiated as a download.

This is a direction I really think we will be going but it's probably too forward thinking at this point.  I'll rethink it a bit.

> 
> > Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.cpp:2
> > + * Copyright (C) 2012 Apple Inc. All rights reserved.
> 
> Was this all written from scratch in 2012?
> 
> Ditto for all the new files you add.

Even though the other files just say 2010 each had edits in 2011... I'll make these 10/11/12

> 
> > Source/WebKit2/NetworkProcess/NetworkProcess.cpp:34
> > +#include "NetworkProcessProxyMessages.h"
> > +#include "NetworkConnectionToWebProcess.h"
> 
> Alphabetic ordering.
> 
> > Source/WebKit2/UIProcess/Network/NetworkProcessManager.cpp:13
> > + * THIS SOFTWARE IS PROVIDED BY APPLE COMPUTER, INC. ``AS IS'' AND ANY
> 
> Apple COMPUTER?
> 
> > Source/WebKit2/UIProcess/Network/NetworkProcessManager.cpp:16
> > + * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL APPLE COMPUTER, INC. OR
> 
> Apple COMPUTER?
> 
> Ditto for all the new files you added.

*wow* - I chose the wrong headers to copy over!!!

> 
> > Source/WebKit2/UIProcess/Network/NetworkProcessManager.cpp:64
> > +
> 
> Blank line.
> 
> > Source/WebKit2/UIProcess/WebProcessProxy.messages.in:49
> > +    GetNetworkProcessConnection(WTF::String identifier) -> (CoreIPC::Attachment connectionHandle) Delayed
> 
> Is this unique identifier (such as a autoincremented number), or some kind of a "class"? The argument name could be more descriptive.

I was more descriptive in some of the implementation headers but...

> 
> > Source/WebKit2/WebProcess/Network/NetworkProcessConnectionManager.cpp:50
> > +NetworkProcessConnection* NetworkProcessConnectionManager::getNetworkProcessConnection(const String& networkProcessIdentifier)
> 
> Seems that these strings should be at least atomic (but then again, perhaps this code can be removed for now).

... I'll remove it for now.

[Brady Eidson]

Created attachment 169702 [details]
Patch v4 - With changes, should get a new once-over

The makes WebProcess have only a singular NetworkProcessConnection and removes the concept of the string identifier.

Otherwise, it's structurally unchanged.

[Brady Eidson]

Just waiting on EWS to give me the all-clear, then I'll remove r? and land.

[WebKit Review Bot]

Attachment 169702 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
Source/WebKit2/NetworkProcess/NetworkProcess.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebKit2/WebProcess/WebProcess.cpp:278:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 2 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brady Eidson]

(In reply to comment #13)
> Attachment 169702 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1
> Source/WebKit2/NetworkProcess/NetworkProcess.cpp:33:  Alphabetical sorting problem.  [build/include_order] [4]
> Source/WebKit2/WebProcess/WebProcess.cpp:278:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> Total errors found: 2 in 25 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Fixed locally.  Still just waiting on the green build from wk2 bots.

[Brady Eidson]

http://trac.webkit.org/changeset/131958

[Eric Seidel (no email)]

Comment on attachment 169702 [details]
Patch v4 - With changes, should get a new once-over

Cleared review? from attachment 169702 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).