Bug 224191 - REGRESSION(r267763) NetworkProcess never terminates
Summary: REGRESSION(r267763) NetworkProcess never terminates
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-04-05 10:15 PDT by Alex Christensen
Modified: 2021-04-05 21:03 PDT (History)
5 users (show)

See Also:


Attachments
Patch (9.11 KB, patch)
2021-04-05 10:19 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (10.06 KB, patch)
2021-04-05 11:32 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (10.81 KB, patch)
2021-04-05 11:54 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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]
Patch
Comment 2 Alex Christensen 2021-04-05 10:19:38 PDT
<rdar://problem/76124590>
Comment 3 Chris Dumez 2021-04-05 10:37:09 PDT
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.
Comment 4 Alex Christensen 2021-04-05 11:32:07 PDT
Created attachment 425178 [details]
Patch
Comment 5 Chris Dumez 2021-04-05 11:41:41 PDT
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().
Comment 6 Alex Christensen 2021-04-05 11:54:22 PDT
Created attachment 425181 [details]
Patch
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].