As per https://developer.apple.com/library/content/technotes/tn2277/_index.html#//apple_ref/doc/uid/DTS40010841-CH1-SUBSECTION2, listening sockets should be closed when process is suspended. Otherwise, there is a risk for spinning in physicalsocketserver: select will return with a listening socket that should be read but accept will fail as there is nothing to read.
Closing listening TCP sockets might be an issue in the case where we do WebRTC: - no a/v capture - Using server TCP sockets for the communication In that case, the connection will be closed and the web page will probably need to redo a negotiation and ICE gathering to continue exchanging data
rdar://problem/33139844
Created attachment 314868 [details] Patch
Does not build?
Created attachment 314876 [details] Patch
Comment on attachment 314876 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314876&action=review > Source/WebKit2/ChangeLog:13 > + Tested through manual testing by going to a website doing WebRTC, homing out so that the network process is suspended and reopening Safari. Indentation problem. > Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.h:67 > + bool needToCleanupForSuspension() const Instead of having a separate method, can we just have cleanupForSuspension() call the completion handler right away if there is nothing to clean. I think it would simplify the code. > Source/WebKit2/NetworkProcess/NetworkProcess.cpp:645 > + RELEASE_LOG(ProcessSuspension, "%p - NetworkProcess::prepareToSuspend() Sending ProcessReadyToSuspend IPC message", this); You need to update the method name in there. > Source/WebKit2/NetworkProcess/NetworkProcess.cpp:646 > + parentProcessConnection()->send(Messages::NetworkProcessProxy::ProcessReadyToSuspend(), 0); We should ASSERT(!m_webProcessConnectionsToClean); in this method. > Source/WebKit2/NetworkProcess/NetworkProcess.cpp:653 > + if (shouldAcknowledgeWhenReadyToSuspend == ShouldAcknowledgeWhenReadyToSuspend::Yes) Not sure we need this check. Setting member to 0 does not hurt, right? > Source/WebKit2/NetworkProcess/NetworkProcess.cpp:661 > + if (shouldAcknowledgeWhenReadyToSuspend == ShouldAcknowledgeWhenReadyToSuspend::Yes) Not sure we need this check. > Source/WebKit2/NetworkProcess/NetworkProcess.cpp:662 > + m_webProcessConnectionsToClean++; ++m_webProcessConnectionsToClean; > Source/WebKit2/NetworkProcess/NetworkProcess.cpp:668 > + if (!--m_webProcessConnectionsToClean) We should ASSERT(m_webProcessConnectionsToClean) before decrementing. > Source/WebKit2/NetworkProcess/NetworkProcess.h:244 > + uint64_t m_webProcessConnectionsToClean { 0 }; how about just unsigned? Why 64 bit? > Source/WebKit2/NetworkProcess/webrtc/NetworkRTCProvider.cpp:237 > + completionHandler(); Can we do this at the end of the function instead? This seems safer. > Source/WebKit2/NetworkProcess/webrtc/NetworkRTCProvider.cpp:239 > + if (!m_connection) This should not be an early return if you move the completionHandler call below.
Comment on attachment 314876 [details] Patch Thanks for the review. View in context: https://bugs.webkit.org/attachment.cgi?id=314876&action=review >> Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.h:67 >> + bool needToCleanupForSuspension() const > > Instead of having a separate method, can we just have cleanupForSuspension() call the completion handler right away if there is nothing to clean. I think it would simplify the code. In most cases, no cleanup will be done. If we do not do this, we will increment/decrement counters, call functions, create callbacks for nothing. Given the small added complexity, I slightly prefer the current approach. If we need to add more regular clean-up in the future, we can easily change that. >> Source/WebKit2/NetworkProcess/NetworkProcess.cpp:645 >> + RELEASE_LOG(ProcessSuspension, "%p - NetworkProcess::prepareToSuspend() Sending ProcessReadyToSuspend IPC message", this); > > You need to update the method name in there. ok >> Source/WebKit2/NetworkProcess/NetworkProcess.cpp:646 >> + parentProcessConnection()->send(Messages::NetworkProcessProxy::ProcessReadyToSuspend(), 0); > > We should ASSERT(!m_webProcessConnectionsToClean); in this method. ok >> Source/WebKit2/NetworkProcess/NetworkProcess.cpp:653 >> + if (shouldAcknowledgeWhenReadyToSuspend == ShouldAcknowledgeWhenReadyToSuspend::Yes) > > Not sure we need this check. Setting member to 0 does not hurt, right? The issue is if processWillSuspendImminently is called while we still have some connections to clean. Then we will set m_webProcessConnectionsToClean to zero and it will get decremented later on. >> Source/WebKit2/NetworkProcess/NetworkProcess.cpp:661 >> + if (shouldAcknowledgeWhenReadyToSuspend == ShouldAcknowledgeWhenReadyToSuspend::Yes) > > Not sure we need this check. We could get rid of it if we are sure that prepareToSuspend() is always called before processWillSuspendImminently(). I am not sure this is guaranteed. >> Source/WebKit2/NetworkProcess/NetworkProcess.h:244 >> + uint64_t m_webProcessConnectionsToClean { 0 }; > > how about just unsigned? Why 64 bit? yes, makes sense. >> Source/WebKit2/NetworkProcess/webrtc/NetworkRTCProvider.cpp:237 >> + completionHandler(); > > Can we do this at the end of the function instead? This seems safer. We can do both ways. It is ok to be suspended as soon as sockets are closed, which is ensured at this point. Doing IPC to notify the web process is optional.
Comment on attachment 314876 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314876&action=review >>> Source/WebKit2/NetworkProcess/NetworkProcess.cpp:653 >>> + if (shouldAcknowledgeWhenReadyToSuspend == ShouldAcknowledgeWhenReadyToSuspend::Yes) >> >> Not sure we need this check. Setting member to 0 does not hurt, right? > > The issue is if processWillSuspendImminently is called while we still have some connections to clean. > Then we will set m_webProcessConnectionsToClean to zero and it will get decremented later on. Hmm, I could see this happen indeed. Good point.
Comment on attachment 314876 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314876&action=review >>>> Source/WebKit2/NetworkProcess/NetworkProcess.cpp:653 >>>> + if (shouldAcknowledgeWhenReadyToSuspend == ShouldAcknowledgeWhenReadyToSuspend::Yes) >>> >>> Not sure we need this check. Setting member to 0 does not hurt, right? >> >> The issue is if processWillSuspendImminently is called while we still have some connections to clean. >> Then we will set m_webProcessConnectionsToClean to zero and it will get decremented later on. > > Hmm, I could see this happen indeed. Good point. We could also use something like: class DelayedReadyToSuspendScope : public RefCounted< DelayedReadyToSuspendScope > { DelayedReadyToSuspendScope(networkProcess, shouldAcknowledgeWhenReadyToSuspend); ~DelayedReadyToSuspendScope() { if (m_shouldAcknowledgeWhenReadyToSuspend == ShouldAcknowledgeWhenReadyToSuspend::Yes) m_networkProcess->notifyProcessReadyToSuspend() } private: m_shouldAcknowledgeWhenReadyToSuspend; m_networkProcess; } Then you capture a RefPtr<> to this in each lambda.
Comment on attachment 314876 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314876&action=review >>>>> Source/WebKit2/NetworkProcess/NetworkProcess.cpp:653 >>>>> + if (shouldAcknowledgeWhenReadyToSuspend == ShouldAcknowledgeWhenReadyToSuspend::Yes) >>>> >>>> Not sure we need this check. Setting member to 0 does not hurt, right? >>> >>> The issue is if processWillSuspendImminently is called while we still have some connections to clean. >>> Then we will set m_webProcessConnectionsToClean to zero and it will get decremented later on. >> >> Hmm, I could see this happen indeed. Good point. > > We could also use something like: > > class DelayedReadyToSuspendScope : public RefCounted< DelayedReadyToSuspendScope > { > DelayedReadyToSuspendScope(networkProcess, shouldAcknowledgeWhenReadyToSuspend); > > ~DelayedReadyToSuspendScope() > { > if (m_shouldAcknowledgeWhenReadyToSuspend == ShouldAcknowledgeWhenReadyToSuspend::Yes) > m_networkProcess->notifyProcessReadyToSuspend() > } > > private: > m_shouldAcknowledgeWhenReadyToSuspend; > m_networkProcess; > } > > Then you capture a RefPtr<> to this in each lambda. We don't even need the shouldAcknowledgeWhenReadyToSuspend. This can be dealt with upon initial construction: RefPtr<DelayedReadyToSuspendScope> scope; if (shouldAcknowledgeWhenReadyToSuspend == ShouldAcknowledgeWhenReadyToSuspend::Yes) scope = adoptRef(DelayedReadyToSuspendScope(*this)); Then capture scope in the lambdas.
> We don't even need the shouldAcknowledgeWhenReadyToSuspend. This can be > dealt with upon initial construction: > RefPtr<DelayedReadyToSuspendScope> scope; > if (shouldAcknowledgeWhenReadyToSuspend == > ShouldAcknowledgeWhenReadyToSuspend::Yes) > scope = adoptRef(DelayedReadyToSuspendScope(*this)); > > Then capture scope in the lambdas. Yep, sounds much better.
Created attachment 314898 [details] Patch
Attachment 314898 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.cpp:651: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 314898 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314898&action=review > Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.h:67 > + bool needToCleanupForSuspension() const I still don't like that we have this function. The complexity should be inside cleanupForSuspension(), not at the call site. Also, the API is risky since it will crash if you call cleanupForSuspension() without checking needToCleanupForSuspension() first. I still think the m_rtcProvider check should be inside cleanupForSuspension().
Comment on attachment 314898 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=314898&action=review > Source/WebKit2/NetworkProcess/NetworkProcess.cpp:643 > +void NetworkProcess::notifyProcessReadyToSuspend() Not sure we need a function for this. We could just move those 2 lines in the DelayedReadyToSuspendScope destructor. > Source/WebKit2/NetworkProcess/NetworkProcess.cpp:661 > + RefPtr<DelayedReadyToSuspendScope> delayedReadyToSuspend = (shouldAcknowledgeWhenReadyToSuspend == ShouldAcknowledgeWhenReadyToSuspend::Yes) ? adoptRef(new DelayedReadyToSuspendScope(*this)) : nullptr; No need for the parentheses.
Created attachment 314902 [details] Patch
Attachment 314902 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.cpp:651: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to youenn fablet from comment #16) > Created attachment 314902 [details] > Patch Removed two-step cleanup util. Will retest it on iOS over the weekend.
Created attachment 314921 [details] Patch
Attachment 314921 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/webrtc/NetworkRTCProvider.cpp:225: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.cpp:645: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 2 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 314932 [details] Fixing release logging
Attachment 314932 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/webrtc/NetworkRTCProvider.cpp:225: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.cpp:645: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 2 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Promises in C++ code now? :( I have not looked in detail yet but the previous iteration seemed much simpler.
Comment on attachment 314932 [details] Fixing release logging View in context: https://bugs.webkit.org/attachment.cgi?id=314932&action=review > Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.cpp:192 > +std::optional<SuspensionResult> NetworkConnectionToWebProcess::cleanupForSuspension() I would much prefer passing a lambda rather than returning an optional promise. > Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.cpp:198 > + return std::nullopt; This could call the lambda right away, as I suggested previously. > Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.h:54 > +class SuspensionResult { I don't think this added complexity is necessary. My previous review comments were about simplify the code. > Source/WebKit2/NetworkProcess/NetworkProcess.cpp:643 > +class TaskCounter : public RefCounted<TaskCounter> { This looks extremely similar to WTF's RefCounter now.
Main change here is making sure no new TCP listening socket is created when we start closing them. That should give us full protection against occasional spins. Ideally we would close the listening sockets when resuming the process. That way we would not close the sockets in case of cancelled suspension. But the select/accept issue does not allow us that at the moment. > > Source/WebKit2/NetworkProcess/NetworkConnectionToWebProcess.cpp:192 > > +std::optional<SuspensionResult> NetworkConnectionToWebProcess::cleanupForSuspension() > > I would much prefer passing a lambda rather than returning an optional > promise. I like the way NetworkProcess::actualPrepareToSuspend is written. That makes NetworkProcess code clearer to me. I also like the fact of returning something instead of passing a parameter that we should make sure to call in every case. If this is a blocker, I am fine going with more lambdas. > > Source/WebKit2/NetworkProcess/NetworkProcess.cpp:643 > > +class TaskCounter : public RefCounted<TaskCounter> { > > This looks extremely similar to WTF's RefCounter now. Using RefCounter seems fine to me.
I think lambdas are much simpler here. I do not see the point of introducing a new concept for this tiny piece of code.
Created attachment 315034 [details] Patch
(In reply to youenn fablet from comment #27) > Created attachment 315034 [details] > Patch Here is the version with lambdas
Attachment 315034 [details] did not pass style-queue: ERROR: Source/WebKit2/NetworkProcess/NetworkProcess.cpp:646: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4] Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 315034 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315034&action=review Looks good otherwise. > Source/WebKit2/NetworkProcess/NetworkProcess.cpp:643 > +// FIXME: We can remove this one by adapting RefCounter. I think we should do this. Why introduce a new class if there is a direct replacement? As far as I can tell, this is a trivial change, no? If so, why add a FIXME comment instead of doing it right?
(In reply to Chris Dumez from comment #30) > Comment on attachment 315034 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=315034&action=review > > Looks good otherwise. > > > Source/WebKit2/NetworkProcess/NetworkProcess.cpp:643 > > +// FIXME: We can remove this one by adapting RefCounter. > > I think we should do this. Why introduce a new class if there is a direct > replacement? As far as I can tell, this is a trivial change, no? If so, why > add a FIXME comment instead of doing it right? We need the counter value of RefCounter to trigger the IPC call when it is decremented to zero. This counter value is not passed to the lambda right now by RefCounter.
Comment on attachment 315034 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315034&action=review >>> Source/WebKit2/NetworkProcess/NetworkProcess.cpp:643 >>> +// FIXME: We can remove this one by adapting RefCounter. >> >> I think we should do this. Why introduce a new class if there is a direct replacement? As far as I can tell, this is a trivial change, no? If so, why add a FIXME comment instead of doing it right? > > We need the counter value of RefCounter to trigger the IPC call when it is decremented to zero. > This counter value is not passed to the lambda right now by RefCounter. You can get the counter value via RefCounter::value().
The lambda has no reference to the counter at the moment. I'll look at this refactoring when I will have some more time in a few weeks. I don't think this should block landing this patch.
Comment on attachment 315034 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=315034&action=review >>>> Source/WebKit2/NetworkProcess/NetworkProcess.cpp:643 >>>> +// FIXME: We can remove this one by adapting RefCounter. >>> >>> I think we should do this. Why introduce a new class if there is a direct replacement? As far as I can tell, this is a trivial change, no? If so, why add a FIXME comment instead of doing it right? >> >> We need the counter value of RefCounter to trigger the IPC call when it is decremented to zero. >> This counter value is not passed to the lambda right now by RefCounter. > > You can get the counter value via RefCounter::value(). For the record, I preferred the previous version that had the IPC in its destructor. There is no point in making this generic and it is unlikely to be reused considering it is in a cpp file. I preferred the previous iteration because it resulted in simpler code below.
Comment on attachment 315034 [details] Patch Clearing flags on attachment: 315034 Committed r219328: <http://trac.webkit.org/changeset/219328>
All reviewed patches have been landed. Closing bug.
Thanks for the review. > For the record, I preferred the previous version that had the IPC in its > destructor. There is no point in making this generic and it is unlikely to > be reused considering it is in a cpp file. > I preferred the previous iteration because it resulted in simpler code below. This allowed removing the friend class TaskCounter declaration in WebPageProxy header. TaskCounter is probably not a very good name, improving it might be good for readability. As of reusability, let's decide what we want when looking at merging it with RefCounter.