Bug 168010

Created attachment 301326 [details] Patch

Attachment 301326 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.h:120: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.h:121: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.cpp:316: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.cpp:324: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 4 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 301371 [details] Patch

Comment on attachment 301371 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301371&action=review > Source/WebKit2/ChangeLog:12 > + Note: This does not yet do anything in the UIProcess to confirm we should expand the sandbox. Not yet... > Source/WebKit2/ChangeLog:18 > + 2. Each NetworkRTCProvider should remember what sandbox extensions were created on its behalf, > + and revoke them when closing down. Since the NetworkRTCProvider is kind of owned by the NetworkConnectionToWebProcess, we might not want to wait until that WebProcess closes before revoking the sandbox extensions. For example, if a user navigated to a page that used WebRTC once a few months ago and then navigated away, we don't want to keep the sandbox extension. Closing it earlier will require a change to something that isn't touched by this patch, though. > Source/WebKit2/UIProcess/Network/NetworkProcessProxy.cpp:50 > +#if USE(LIBWEBRTC) > +#include <wtf/text/StringBuilder.h> Let's include this unconditionally. > Source/WebKit2/UIProcess/Network/NetworkProcessProxy.cpp:309 > + WTFLogAlways("NetworkProcessProxy::grantSandboxExtensionsToRTCProvider: Expanding sandbox to include: %s", extensionBuilder.toString().utf8().data()); Do we really want to WTFLogAlways this?

Some discussion items we discussed with brent & ap: - Having the extension be passed as a Ref<>&& - Adding a failure message to remove the handler if denied - The UI process may still want to have something to say about TCP client sockets even though these might not need any sandbox extension (to be checked)

Created attachment 301405 [details] Patch

Comment on attachment 301405 [details] Patch LGTM. A few nits below. View in context: https://bugs.webkit.org/attachment.cgi?id=301405&action=review > Source/WebKit2/NetworkProcess/NetworkProcess.h:229 > + HashMap<uint64_t, Function<void(RefPtr<SandboxExtension>)>> m_sandboxExtensionsForRTCProviderCompletionHandlers; Shouldn't it be Function<void(Ref<SandboxExtension>)&&> ? > Source/WebKit2/NetworkProcess/NetworkProcess.messages.in:81 > + DidGrantSandboxExtensionsToRTCProvider(uint64_t identifier, WebKit::SandboxExtension::Handle handle) s/Handle handle/Handle/ > Source/WebKit2/NetworkProcess/webrtc/NetworkRTCProvider.cpp:81 > + [this, identifier, socketAddress, minPort, maxPort](Ref<SandboxExtension>&& extension) { You need to ensure that "this" is not captured by taking a ref from it, something like: protectedThis = makeRef(*this). That is why we also need to ensure the UI process actually denies access instead of not responding, should we have a need for it. Not sure how to best keep that information. Maybe a comment in the UI process code? > Source/WebKit2/NetworkProcess/webrtc/NetworkRTCProvider.cpp:83 > + callOnRTCNetworkThread([this, identifier, socketAddress, minPort, maxPort]() { No need here to protect "this" since NetworkRTCProvider::NetworkRTCProvider does it for you. > Source/WebKit2/NetworkProcess/webrtc/NetworkRTCProvider.cpp:113 > + WTFLogAlways("NetworkRTCProvider::createClientTCPSocket: Remind me to open a client TCP socket from %s to %s once I have an extension.", localAddress.utf8().data(), remoteAddress.utf8().data()); Why having this log here? > Source/WebKit2/NetworkProcess/webrtc/NetworkRTCProvider.h:105 > + Vector<RefPtr<SandboxExtension>> m_sandboxExtensions; We support Vector<Ref<>> I think

Some discussion items related to webrtc networking and sandbox and have webrtc networking in its own process. That would have a few benefits: - No need to relax the current network process sandbox - Tighten webrtc networking sandbox (no file access...) - No risk of compromising the network process with libwebrtc code issues - Go directly from libwebrtc network thread (in the web process) to webrtc networking process

(In reply to comment #9) > Some discussion items related to webrtc networking and sandbox and have > webrtc networking in its own process. > That would have a few benefits: > - No need to relax the current network process sandbox > - Tighten webrtc networking sandbox (no file access...) > - No risk of compromising the network process with libwebrtc code issues > - Go directly from libwebrtc network thread (in the web process) to webrtc > networking process Yeah -- given some of the limitations in what we can dynamically change in the sandbox, moving to a separate process would probably be a better model.

Created attachment 301682 [details] Patch

Comment on attachment 301682 [details] Patch Since we are not able to consume the network sandbox extension, I would remove the code related to it from this patch. The grant/deny callback would then be Function<void()>>. I would also consider splitting the verification to check for socket opening by the UIProcess in a separate patch. View in context: https://bugs.webkit.org/attachment.cgi?id=301682&action=review > Source/WebKit2/NetworkProcess/NetworkProcess.cpp:316 > +void NetworkProcess::grantSandboxExtensionsToRTCProvider(const String& protocol, const String& address, uint16_t minPort, uint16_t maxPort, Function<void(Ref<SandboxExtension>&&)>&& completionHandler) If we keep this mechanism, I would rename it to something like grantRTCConnection/denyRTCConnection. When being allowed, a sandbox extension might be given if the network process can handle dynamic extension. > Source/WebKit2/NetworkProcess/NetworkProcess.cpp:319 > + m_sandboxExtensionsForRTCProviderCompletionHandlers.set(requestID, WTFMove(completionHandler)); I would rename it to m_pendingConnections. > Source/WebKit2/NetworkProcess/NetworkProcess.h:229 > + HashMap<uint64_t, Function<void(Ref<SandboxExtension>)>> m_sandboxExtensionsForRTCProviderCompletionHandlers; If we keep it, it should probably be Function<void(Ref<SandboxExtension>&&> > Source/WebKit2/NetworkProcess/webrtc/NetworkRTCProvider.cpp:112 > + WTFLogAlways("NetworkRTCProvider::createClientTCPSocket: Remind me to open a client TCP socket from %s to %s once I have an extension.", localAddress.utf8().data(), remoteAddress.utf8().data()); Is it needed? > Source/WebKit2/NetworkProcess/webrtc/NetworkRTCProvider.h:105 > + Vector<RefPtr<SandboxExtension>> m_sandboxExtensions; Should be a Vector of Ref. > Source/WebKit2/UIProcess/Cocoa/WebProcessPoolCocoa.mm:249 > + bool webRTCEnabled = m_defaultPageGroup->preferences().peerConnectionEnabled(); We should test for mediaStreamEnabled, so s/webRTCEnabled/mediaStreamEnabled. Also we redefine webRTCEnabled below, this would fix the issue here. > Source/WebKit2/UIProcess/Cocoa/WebProcessPoolCocoa.mm:307 > +#if ENABLE(MEDIA_STREAM) Should be ENABLE(WEB_RTC)

(In reply to comment #12) > Comment on attachment 301682 [details] > Patch > > I would also consider splitting the verification to check for socket opening > by the UIProcess in a separate patch. I'll land that patch under Bug 168242.

Committed r212451: <http://trac.webkit.org/changeset/212451>