Bug 224191 – REGRESSION(r267763) NetworkProcess never terminates

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

REGRESSION(r267763) NetworkProcess never terminates

[Alex Christensen]

Created attachment 425168 [details]
Patch

[Alex Christensen]

<rdar://problem/76124590>

[Chris Dumez]

Comment on attachment 425168 [details]
Patch

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

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:1289
> +        terminate();

Why are we calling terminate() instead of clearing the defaultNetworkProcess (which should deallocate the NetworkProcessProxy and cause the process to go away)?

While both are fairly similar, I think clearing the default network process might be nicer for a few reasons:
1. This is what we used to do. When destroying the WebProcessPool, we would deallocate the NetworkProcessProxy and it would just go away (no termination involved)
2. There is a slight risk that there could be code somewhere with a RefPtr<> to a NetworkProcess (e.g. for some kind of transient operation) and terminate would abruptly interrupt the operation instead of letting it finish.
3. Youl will likely run more code doing a termination (due to termination handling) when simple destruction.

> Source/WebKit/UIProcess/WebProcessPool.cpp:381
> +        NetworkProcessProxy::defaultNetworkProcess()->terminate();

Ditto.

[Alex Christensen]

Created attachment 425178 [details]
Patch

[Chris Dumez]

Comment on attachment 425178 [details]
Patch

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

r=me with naming change.

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:104
> +RefPtr<NetworkProcessProxy>& NetworkProcessProxy::defaultNetworkProcessPtr()

I think the WebKit pattern is:
defaultNetworkProcessIfExists()

I don't like the Ptr() naming. When I saw it, it really wasn't clear to me what it meant.

If you don't like defaultNetworkProcessIfExists(), I guess we could call this one defaultNetworkProcess() and the one below: ensureDefaultNetworkProcess().

[Alex Christensen]

Created attachment 425181 [details]
Patch

[EWS]

Committed r275487: <https://commits.webkit.org/r275487>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 425181 [details].