Bug 174270 – NetworkProcess should close listening WebRTC sockets when being suspended

• Component: Media
• Status: RESOLVED FIXED

[youenn fablet]

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.

[youenn fablet]

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

[youenn fablet]

rdar://problem/33139844

[youenn fablet]

Created attachment 314868 [details]
Patch

[Chris Dumez]

Does not build?

[youenn fablet]

Created attachment 314876 [details]
Patch

[Chris Dumez]

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.

[youenn fablet]

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.

[Chris Dumez]

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.

[Chris Dumez]

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.

[Chris Dumez]

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.

[youenn fablet]

> 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.

[youenn fablet]

Created attachment 314898 [details]
Patch

[Build Bot]

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.

[Chris Dumez]

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().

[Chris Dumez]

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.

[youenn fablet]

Created attachment 314902 [details]
Patch

[Build Bot]

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.

[youenn fablet]

(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.

[youenn fablet]

Created attachment 314921 [details]
Patch

[Build Bot]

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.

[youenn fablet]

Created attachment 314932 [details]
Fixing release logging

[Build Bot]

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.

[Chris Dumez]

Promises in C++ code now? :( I have not looked in detail yet but the previous iteration seemed much simpler.

[Chris Dumez]

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.

[youenn fablet]

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.

[Chris Dumez]

I think lambdas are much simpler here. I do not see the point of introducing a new concept for this tiny piece of code.

[youenn fablet]

Created attachment 315034 [details]
Patch

[youenn fablet]

(In reply to youenn fablet from comment #27)
> Created attachment 315034 [details]
> Patch

Here is the version with lambdas

[Build Bot]

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.

[Chris Dumez]

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?

[youenn fablet]

(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.

[Chris Dumez]

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().

[youenn fablet]

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.

[Chris Dumez]

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.

[WebKit Commit Bot]

Comment on attachment 315034 [details]
Patch

Clearing flags on attachment: 315034

Committed r219328: <http://trac.webkit.org/changeset/219328>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

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.