Bug 183989 – Move online state detection from the WebProcess to the NetworkProcess

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Move online state detection from the WebProcess to the NetworkProcess. This avoid executing the same (expensive) code in EACH web process whenever a network interface's state changes.

[Chris Dumez]

<rdar://problem/37093299>

[Chris Dumez]

Created attachment 336488 [details]
Patch

[EWS Watchlist]

Attachment 336488 [details] did not pass style-queue:


ERROR: Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:75:  Missing spaces around :  [whitespace/init] [4]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Comment on attachment 336488 [details]
Patch

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

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:561
> +    for (auto& listener : m_onlineStateChangeListeners)
> +        listener(isOnLine);

Can the listener modify m_onlineStateChangeListeners? Even if not, what guarantees that this will not become a problem in the future?

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.h:101
> +    bool m_isOnLine { true };

That's optimistic :)

[Chris Dumez]

Comment on attachment 336488 [details]
Patch

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

>> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:561
>> +        listener(isOnLine);
> 
> Can the listener modify m_onlineStateChangeListeners? Even if not, what guarantees that this will not become a problem in the future?

not An issue right now but I will make a copy for future proofing.

>> Source/WebKit/WebProcess/Network/WebLoaderStrategy.h:101
>> +    bool m_isOnLine { true };
> 
> That's optimistic :)

I think this matches the spec which says to return false only if we know we do not have network access.

[Chris Dumez]

Created attachment 336499 [details]
Patch

[EWS Watchlist]

Attachment 336499 [details] did not pass style-queue:


ERROR: Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:75:  Missing spaces around :  [whitespace/init] [4]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

ping review.

[youenn fablet]

Comment on attachment 336499 [details]
Patch

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

> Source/WebCore/loader/LoaderStrategy.h:79
> +    virtual void addOnlineStateChangeListener(WTF::Function<void(bool)>&&) = 0;

I think we no longer need the WTF prefix.

> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:257
> +        return;

Connection::sendMessage is already checking whether valid or not.
We do not gain much except not encoding isOnline.
In some other places we are not checking this and still call send.
Maybe we should do the same?

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:130
> +    });

Although ok, it seems somehow strange that we are never removing the listener.
Ideally, we would add the listener/remove the listener whenever NetworkProcess has a WebProcess connection.

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:548
> +void WebLoaderStrategy::addOnlineStateChangeListener(WTF::Function<void(bool)>&& listener)

WTF:: here and below?

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:551
> +    m_onlineStateChangeListeners.append(WTFMove(listener));

It seems we will only call WebLoaderStrategy::addOnlineStateChangeListener once.
Maybe m_onlineStateChangeListeners should not be a Vector.

[Chris Dumez]

Comment on attachment 336499 [details]
Patch

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

>> Source/WebCore/loader/LoaderStrategy.h:79
>> +    virtual void addOnlineStateChangeListener(WTF::Function<void(bool)>&&) = 0;
> 
> I think we no longer need the WTF prefix.

ok

>> Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp:257
>> +        return;
> 
> Connection::sendMessage is already checking whether valid or not.
> We do not gain much except not encoding isOnline.
> In some other places we are not checking this and still call send.
> Maybe we should do the same?

ok

>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:130
>> +    });
> 
> Although ok, it seems somehow strange that we are never removing the listener.
> Ideally, we would add the listener/remove the listener whenever NetworkProcess has a WebProcess connection.

NetworkStateNotifier does not even have an API to remove listeners. Listeners are expected to be permanent at the moment.

>> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:548
>> +void WebLoaderStrategy::addOnlineStateChangeListener(WTF::Function<void(bool)>&& listener)
> 
> WTF:: here and below?

ok

>> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:551
>> +    m_onlineStateChangeListeners.append(WTFMove(listener));
> 
> It seems we will only call WebLoaderStrategy::addOnlineStateChangeListener once.
> Maybe m_onlineStateChangeListeners should not be a Vector.

This is inaccurate, it is called twice, once for pages and once for workers.

[Chris Dumez]

Created attachment 336606 [details]
Patch

[Chris Dumez]

Comment on attachment 336499 [details]
Patch

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

>>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:130
>>> +    });
>> 
>> Although ok, it seems somehow strange that we are never removing the listener.
>> Ideally, we would add the listener/remove the listener whenever NetworkProcess has a WebProcess connection.
> 
> NetworkStateNotifier does not even have an API to remove listeners. Listeners are expected to be permanent at the moment.

I guess we could add an API to NetworkStateNotifier to remove listeners but I am not sure it is worth the code / complexity since having a NetworkProcess and no WebProcess is rare.

[EWS Watchlist]

Attachment 336606 [details] did not pass style-queue:


ERROR: Source/WebCore/workers/service/context/ServiceWorkerThread.cpp:75:  Missing spaces around :  [whitespace/init] [4]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

> >> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:551
> >> +    m_onlineStateChangeListeners.append(WTFMove(listener));
> > 
> > It seems we will only call WebLoaderStrategy::addOnlineStateChangeListener once.
> > Maybe m_onlineStateChangeListeners should not be a Vector.
> 
> This is inaccurate, it is called twice, once for pages and once for workers.

OK but it could be easily reduced to only one listener.
It feels like we are propagating NetworkStateNotifier multi listener-API although that does not seem necessary.

[youenn fablet]

Comment on attachment 336606 [details]
Patch

r = me.
I would try to look at simplifying the loader strategy API.
Maybe we could just have something like loaderStrategy->startListeningToOnlineChange().

[WebKit Commit Bot]

Comment on attachment 336606 [details]
Patch

Clearing flags on attachment: 336606

Committed r230007: <https://trac.webkit.org/changeset/230007>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.