[webkit-reviews] review granted: [Bug 99817] Add infrastructure for NetworkProcess management : [Attachment 169569] Patch v3 - And again...

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Brady Eidson
<beidson at apple.com>'s request for review:
Bug 99817: Add infrastructure for NetworkProcess management
https://bugs.webkit.org/show_bug.cgi?id=99817

Attachment 169569: Patch v3 - And again...
https://bugs.webkit.org/attachment.cgi?id=169569&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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).