Bug 224191

Summary: REGRESSION(r267763) NetworkProcess never terminates
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Severity: Normal CC: beidson, cdumez, ggaren, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
Patch none

Description Alex Christensen 2021-04-05 10:15:17 PDT
REGRESSION(r267763) NetworkProcess never terminates
Comment 1 Alex Christensen 2021-04-05 10:19:35 PDT
Created attachment 425168 [details]
Comment 2 Alex Christensen 2021-04-05 10:19:38 PDT
Comment 3 Chris Dumez 2021-04-05 10:37:09 PDT
Comment on attachment 425168 [details]

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

Comment 4 Alex Christensen 2021-04-05 11:32:07 PDT
Created attachment 425178 [details]
Comment 5 Chris Dumez 2021-04-05 11:41:41 PDT
Comment on attachment 425178 [details]

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:

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().
Comment 6 Alex Christensen 2021-04-05 11:54:22 PDT
Created attachment 425181 [details]
Comment 7 EWS 2021-04-05 21:03:37 PDT
Committed r275487: <https://commits.webkit.org/r275487>

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