Make WebSockets work in network process
Created attachment 305041 [details] Patch
Let's get more EWS on this shall we?
Created attachment 305080 [details] Patch
Created attachment 305099 [details] Patch
Awesome!
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?
(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.
Created attachment 305256 [details] Patch
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
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
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.
(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.
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
http://trac.webkit.org/r214413
(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.
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?
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.
(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.