Bug 169930 – Make WebSockets work in network process

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Make WebSockets work in network process

[Alex Christensen]

Created attachment 305041 [details]
Patch

[Brady Eidson]

Let's get more EWS on this shall we?

[Alex Christensen]

Created attachment 305080 [details]
Patch

[Alex Christensen]

Created attachment 305099 [details]
Patch

[Michael Catanzaro]

Awesome!

[Brady Eidson]

Comment on attachment 305099 [details]
Patch

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

> Source/WebKit2/WebProcess/Network/WebSocketStream.cpp:57
> +void WebSocketStream::networkProcessCrashed()

You handle the WebProcess side of "networking process did crash", but you don't handle the NetworkProcess side of "web process did crash"

Additionally, I wonder if we can API test both of those.

> Source/WebKit2/WebProcess/Network/WebSocketStream.cpp:86
> +    WebProcess::singleton().networkConnection().connection().send(Messages::NetworkConnectionToWebProcess::DestroySocketStream(m_identifier), 0);

Can we send the DestroySocketStream any sooner than the WebSocketStream d'tor?

My concern would be that some other arbitrary code (JS, Ref<>/RefPtr<>, etc) can keep the WebSocketStream alive indefinitely even after it becomes an inert object and we could clean up its resources in the Networking process.

> Source/WebKit2/WebProcess/Network/WebSocketStream.cpp:119
> +    send(Messages::NetworkSocketStream::Close());

e.g. - Why is this "Close" not a "DestroySocketStream", and we just make sure platformClose is always called before the d'tor?

[Alex Christensen]

(In reply to Brady Eidson from comment #6)
> Comment on attachment 305099 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=305099&action=review
> 
> > Source/WebKit2/WebProcess/Network/WebSocketStream.cpp:57
> > +void WebSocketStream::networkProcessCrashed()
> 
> You handle the WebProcess side of "networking process did crash", but you
> don't handle the NetworkProcess side of "web process did crash"
The NetworkConnectionToWebProcess being destroyed automatically destroys all the NetworkSocketStreams in its map.
> 
> Additionally, I wonder if we can API test both of those.
We could, and I would like to do so in a followup patch.
> 
> > Source/WebKit2/WebProcess/Network/WebSocketStream.cpp:86
> > +    WebProcess::singleton().networkConnection().connection().send(Messages::NetworkConnectionToWebProcess::DestroySocketStream(m_identifier), 0);
> 
> Can we send the DestroySocketStream any sooner than the WebSocketStream
> d'tor?
Maybe.
> 
> My concern would be that some other arbitrary code (JS, Ref<>/RefPtr<>, etc)
> can keep the WebSocketStream alive indefinitely even after it becomes an
> inert object and we could clean up its resources in the Networking process.
I don't think this is a valid concern, and if something were to keep the WebSocketStream alive, then we would want the NetworkSocketStream to stay alive, too, since their only purpose is to mirror each others' lifetimes and talk to each other.
> 
> > Source/WebKit2/WebProcess/Network/WebSocketStream.cpp:119
> > +    send(Messages::NetworkSocketStream::Close());
> 
> e.g. - Why is this "Close" not a "DestroySocketStream", and we just make
> sure platformClose is always called before the d'tor?
platformClose is separate from the destructor.  We might be able to do more cleanup like destroying the NetworkSocketStream when WebSocketStream.platformClose is called, but that would complicate the lifetimes of the two objects, which should be the same in my current design.

[Alex Christensen]

Created attachment 305256 [details]
Patch

[Build Bot]

Comment on attachment 305256 [details]
Patch

Attachment 305256 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3401548

New failing tests:
http/tests/websocket/tests/hybi/network-process-crash-error.html

[Build Bot]

Created attachment 305267 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[youenn fablet]

Comment on attachment 305256 [details]
Patch

LGTM, some comments below.

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

> Source/WebCore/ChangeLog:9
> +        This also does fine with the 544 websocket tests in the web-platform-tests which we have not yet imported.

Is there anything preventing us to import them?

> Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.cpp:104
> +        auto it = m_networkSocketStreams.find(decoder.destinationID());

Rename it to socketIterator to be consistent with other code?

> Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.h:108
> +    void createSocketStream(const WebCore::URL&, WebCore::SessionID, String cachePartition, uint64_t);

Would be nice to move to URL&& in the future.

> Source/WebKit2/NetworkProcess/NetworkSocketStream.cpp:38
> +Ref<NetworkSocketStream> NetworkSocketStream::create(const WebCore::URL& url, WebCore::SessionID sessionID, const String& credentialPartition, uint64_t identifier, IPC::Connection& connection)

Can we use URL&& and String&& here?

> Source/WebKit2/NetworkProcess/NetworkSocketStream.cpp:52
> +    m_impl->platformSend(reinterpret_cast<const char *>(data.data()), data.size(), [this, protectedThis = makeRef(*this), identifier] (bool success) {

Do we need to capture both this and protectedThis?

> Source/WebKit2/NetworkProcess/NetworkSocketStream.h:57
> +    uint64_t messageSenderDestinationID() final;

Do we need to have these public?

> Source/WebKit2/NetworkProcess/NetworkSocketStream.h:65
> +    void didFailSocketStream(WebCore::SocketStreamHandle&, const WebCore::SocketStreamError&) final;

Ditto.

> Source/WebKit2/UIProcess/WebProcessPool.cpp:1238
> +    sendSyncToNetworkingProcess(Messages::NetworkProcess::SetAllowsAnySSLCertificateForWebSocket(allows), Messages::NetworkProcess::SetAllowsAnySSLCertificateForWebSocket::Reply());

Why do we have to do a synchronous call for it?

> Source/WebKit2/WebProcess/WebProcess.cpp:1129
> +    WebSocketStream::networkProcessCrashed();

Why not adding a WebSocketStreamManager owning the map and tied to the WebProcess?

> Source/WebKit2/WebProcess/Network/WebSocketStream.h:54
> +    IPC::Connection* messageSenderConnection() final;

Can be made private probably?

> LayoutTests/http/tests/websocket/tests/hybi/network-process-crash-error.html:27
> +        testRunner.terminateNetworkProcess();

Nice, might want to do that for other features as well.

[Alex Christensen]

(In reply to youenn fablet from comment #11)
> Comment on attachment 305256 [details]
> Patch
> 
> LGTM, some comments below.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=305256&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        This also does fine with the 544 websocket tests in the web-platform-tests which we have not yet imported.
> 
> Is there anything preventing us to import them?
No.  We should.
> > Source/WebKit2/NetworkProcess/NetworkSocketStream.cpp:38
> > +Ref<NetworkSocketStream> NetworkSocketStream::create(const WebCore::URL& url, WebCore::SessionID sessionID, const String& credentialPartition, uint64_t identifier, IPC::Connection& connection)
> 
> Can we use URL&& and String&& here?
Probably.
> 
> > Source/WebKit2/NetworkProcess/NetworkSocketStream.cpp:52
> > +    m_impl->platformSend(reinterpret_cast<const char *>(data.data()), data.size(), [this, protectedThis = makeRef(*this), identifier] (bool success) {
> 
> Do we need to capture both this and protectedThis?
Yes.  Capturing this is so we can call member functions and access member variables without using silly syntax like protectedThis->memberFunction().  Capturing protectedThis is so we can guarantee we will not use any pointers after freeing them if the lambda is called asynchronously after all other references to this object are gone.
> > Source/WebKit2/UIProcess/WebProcessPool.cpp:1238
> > +    sendSyncToNetworkingProcess(Messages::NetworkProcess::SetAllowsAnySSLCertificateForWebSocket(allows), Messages::NetworkProcess::SetAllowsAnySSLCertificateForWebSocket::Reply());
> 
> Why do we have to do a synchronous call for it?
This is just for testing.  We want it to be as reliable as possible for tests.
> 
> > Source/WebKit2/WebProcess/WebProcess.cpp:1129
> > +    WebSocketStream::networkProcessCrashed();
> 
> Why not adding a WebSocketStreamManager owning the map and tied to the
> WebProcess?
Either way, it would need to be a process-global map in the WebProcess and a map owned by the NetworkConnectionToWebProcess in the NetworkProcess, which I already have.
> > LayoutTests/http/tests/websocket/tests/hybi/network-process-crash-error.html:27
> > +        testRunner.terminateNetworkProcess();
> 
> Nice, might want to do that for other features as well.
Definitely.  We want to clean up nicely when things crash.

[Alex Christensen]

Created attachment 305341 [details]
patch for landing

These two tests time out sometimes with this.  Could someone take a look?
http/tests/websocket/tests/hybi/inspector/client-close.html
http/tests/websocket/tests/hybi/inspector/server-close.html

[Alex Christensen]

http://trac.webkit.org/r214413

[Alex Christensen]

(In reply to Alex Christensen from comment #13)
> Created attachment 305341 [details]
> patch for landing
> 
> These two tests time out sometimes with this.  Could someone take a look?
> http/tests/websocket/tests/hybi/inspector/client-close.html
> http/tests/websocket/tests/hybi/inspector/server-close.html

If these tests start timing out, mark them appropriately and have Nikita look into it.

[Carlos Garcia Campos]

Comment on attachment 305341 [details]
patch for landing

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

> Tools/WebKitTestRunner/gtk/TestControllerGtk.cpp:139
> +WKContextRef TestController::platformContext()
> +{
> +    return nullptr;
> +}

Why do we need this platformContext()? isn't cocoa implementation also returning m_context in the end? I've implemented this in r214661 just to fix the crashes, but I think callers of platformContext() could simply use m_context or context(), no?

[Alex Christensen]

I just needed a WKContextRef from the currently executing test.  I didn't see context().  If it's the same as m_context, we should consolidate the two.

[Carlos Garcia Campos]

(In reply to Alex Christensen from comment #17)
> I just needed a WKContextRef from the currently executing test.  I didn't
> see context().  If it's the same as m_context, we should consolidate the two.

Yes, I think it's the same.