Bug 193700 - Stop using NetworkProcess::singleton
Summary: Stop using NetworkProcess::singleton
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: 2019-01-22 17:55 PST by Alex Christensen
Modified: 2019-01-23 16:31 PST (History)
6 users (show)

See Also:


Attachments
Patch (9.77 KB, patch)
2019-01-22 17:55 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (9.60 KB, patch)
2019-01-22 18:08 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (11.33 KB, patch)
2019-01-22 19:40 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (11.36 KB, patch)
2019-01-22 21:27 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (11.82 KB, patch)
2019-01-23 14:36 PST, Alex Christensen
don.olmstead: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2019-01-22 17:55:00 PST
Stop using NetworkProcess::singleton
Comment 1 Alex Christensen 2019-01-22 17:55:35 PST
Created attachment 359823 [details]
Patch
Comment 2 Alex Christensen 2019-01-22 18:08:45 PST
Created attachment 359829 [details]
Patch
Comment 3 Sam Weinig 2019-01-22 19:17:33 PST
Comment on attachment 359829 [details]
Patch

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

> Source/WebKit/ChangeLog:3
> +        Stop using NetworkProcess::singleton

Yay!

> Source/WebKit/NetworkProcess/win/NetworkProcessMainWin.cpp:38
> +template<>
> +void initializeChildProcess<WebKit::NetworkProcess>(ChildProcessInitializationParameters&& parameters)
> +{
> +    static NeverDestroyed<Ref<NetworkProcess>> networkProcess(adoptRef(*new NetworkProcess(WTFMove(parameters))));
> +}

I don't think this will ever be called, since the only caller is XPCServiceInitializer.  You probably need to update ChildProcessMain.h as well. (This is also true for the Soup case).

> Source/WebKit/Shared/EntryPointUtilities/mac/XPCService/XPCServiceEntryPoint.h:128
> -    XPCServiceType::singleton().initialize(parameters);
> +    initializeChildProcess<XPCServiceType>(WTFMove(parameters));

We use the delegate pattern for similar-ish (e.g. things that might need to be customized per service type) things above. Can this be modified to also use the delegate?
Comment 4 Alex Christensen 2019-01-22 19:40:02 PST
Comment on attachment 359829 [details]
Patch

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

>> Source/WebKit/Shared/EntryPointUtilities/mac/XPCService/XPCServiceEntryPoint.h:128
>> +    initializeChildProcess<XPCServiceType>(WTFMove(parameters));
> 
> We use the delegate pattern for similar-ish (e.g. things that might need to be customized per service type) things above. Can this be modified to also use the delegate?

I'll do this in the next patch.
Comment 5 Alex Christensen 2019-01-22 19:40:12 PST
Created attachment 359838 [details]
Patch
Comment 6 Daniel Bates 2019-01-22 20:33:08 PST
Comment on attachment 359838 [details]
Patch

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

> Source/WebKit/ChangeLog:8
> +        This replaces it with a NeverDestroyed<Ref<NetworkProcess>> and paves the way for more interesting things.

Yes, like instantiating a NetworkProcess multiple times... Is this intended? What new world are we living in? :/

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:121
> +    : m_downloadManager(*this)

Uniform initializer syntax?

> Source/WebKit/NetworkProcess/NetworkProcess.h:424
>      CacheModel m_cacheModel;

Does initialize() set this to CacheModel::DocumentView? If no, where is this being done?

> Source/WebKit/NetworkProcess/CustomProtocols/soup/LegacyCustomProtocolManagerSoup.cpp:77
> +        auto* customProtocolManager = lastCreatedNetworkProcess()->supplement<LegacyCustomProtocolManager>();

Ah.. So, it sounds like you can have multiple Network Processes. Wow. At the same time?

> Source/WebKit/NetworkProcess/EntryPoint/mac/XPCService/NetworkServiceEntryPoint.mm:46
> +    static NeverDestroyed<Ref<NetworkProcess>> networkProcess(adoptRef(*new NetworkProcess(WTFMove(parameters))));

I know you are just copying the old style. What is the point of all this ref-business? Static pointer is not enough? Must never be a need to access this static-allocated network process because you can’t.

> Source/WebKit/Shared/unix/ChildProcessMain.h:39
> +    ChildProcessInitializationParameters&& initializationParameters() { return WTFMove(m_parameters); }

std::exchange()? Or does ChildProcessInitializationParameters’s move constructor put us in a good state? Or how do prevent someone from calling calling this twice?
Comment 7 Daniel Bates 2019-01-22 20:34:06 PST
Comment on attachment 359838 [details]
Patch

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

> Source/WebKit/Shared/EntryPointUtilities/mac/XPCService/XPCServiceEntryPoint.h:73
> +    XPCServiceType::singleton().initialize(parameters);

Where’s the WTFMove()?

> Source/WebKit/Shared/unix/ChildProcessMain.h:48
> +    ChildProcessType::singleton().initialize(parameters);

Where’s the WTFMove()?
Comment 8 Sam Weinig 2019-01-22 21:02:50 PST
Comment on attachment 359838 [details]
Patch

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

>> Source/WebKit/Shared/unix/ChildProcessMain.h:39
>> +    ChildProcessInitializationParameters&& initializationParameters() { return WTFMove(m_parameters); }
> 
> std::exchange()? Or does ChildProcessInitializationParameters’s move constructor put us in a good state? Or how do prevent someone from calling calling this twice?

Traditionally, we call something like this, takeInitializationParameters() to indicate that it is destructive.
Comment 9 Sam Weinig 2019-01-22 21:04:57 PST
Comment on attachment 359838 [details]
Patch

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

> Source/WebKit/NetworkProcess/soup/NetworkProcessMainSoup.cpp:39
> +    static NeverDestroyed<Ref<NetworkProcess>> networkProcess(adoptRef(*new NetworkProcess(WTFMove(parameters))));

If this is never passed to anyone, I don't see much reason in making this a NeverDestroyed<Ref<NetworkProcess>>. Can probably just make it a NeverDestroyed<NetworkProcess>.
Comment 10 Alex Christensen 2019-01-22 21:16:46 PST
(In reply to Daniel Bates from comment #6)
> Comment on attachment 359838 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=359838&action=review
> 
> > Source/WebKit/ChangeLog:8
> > +        This replaces it with a NeverDestroyed<Ref<NetworkProcess>> and paves the way for more interesting things.
> 
> Yes, like instantiating a NetworkProcess multiple times... Is this intended?
> What new world are we living in? :/
This is the exact intent of future patches.
> 
> > Source/WebKit/NetworkProcess/NetworkProcess.cpp:121
> > +    : m_downloadManager(*this)
> 
> Uniform initializer syntax?
> 
> > Source/WebKit/NetworkProcess/NetworkProcess.h:424
> >      CacheModel m_cacheModel;
> 
> Does initialize() set this to CacheModel::DocumentView? If no, where is this
> being done?
This is already being done by an initializer list in the header.
> 
> > Source/WebKit/NetworkProcess/CustomProtocols/soup/LegacyCustomProtocolManagerSoup.cpp:77
> > +        auto* customProtocolManager = lastCreatedNetworkProcess()->supplement<LegacyCustomProtocolManager>();
> 
> Ah.. So, it sounds like you can have multiple Network Processes. Wow. At the
> same time?
At the same time.  Organized by connection.  It's going to be awesome.  Tell your friends.
> 
> > Source/WebKit/NetworkProcess/EntryPoint/mac/XPCService/NetworkServiceEntryPoint.mm:46
> > +    static NeverDestroyed<Ref<NetworkProcess>> networkProcess(adoptRef(*new NetworkProcess(WTFMove(parameters))));
> 
> I know you are just copying the old style. What is the point of all this
> ref-business? Static pointer is not enough? Must never be a need to access
> this static-allocated network process because you can’t.
Other things already reference it.  They are created in the initialization.
> 
> > Source/WebKit/Shared/unix/ChildProcessMain.h:39
> > +    ChildProcessInitializationParameters&& initializationParameters() { return WTFMove(m_parameters); }
> 
> std::exchange()? Or does ChildProcessInitializationParameters’s move
> constructor put us in a good state? Or how do prevent someone from calling
> calling this twice?
I wasn't sure everything in ChildProcessInitializationParameters can or should always be able to be default initialized.  I don't like the design of Unix's ChildProcessMain.
Comment 11 Alex Christensen 2019-01-22 21:22:26 PST
(In reply to Daniel Bates from comment #6)
> std::exchange()? Or does ChildProcessInitializationParameters’s move
> constructor put us in a good state? Or how do prevent someone from calling
> calling this twice?
I think WTFMove is fine here because it is the last use of that object, so leaving it in an undefined state is fine.  I'll rename it on Sam's suggestion.
Comment 12 Alex Christensen 2019-01-22 21:23:39 PST
Comment on attachment 359838 [details]
Patch

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

>> Source/WebKit/NetworkProcess/NetworkProcess.h:424
>>      CacheModel m_cacheModel;
> 
> Does initialize() set this to CacheModel::DocumentView? If no, where is this being done?

Oops, I was looking at the WebProcess.  I'll fix it.
Comment 13 Alex Christensen 2019-01-22 21:27:48 PST
Created attachment 359847 [details]
Patch
Comment 14 Alex Christensen 2019-01-22 21:28:22 PST
(In reply to Daniel Bates from comment #6)
> > +    : m_downloadManager(*this)
> Uniform initializer syntax?
Stylebot doesn't like this, but I did it anyways.
Comment 15 EWS Watchlist 2019-01-22 21:29:48 PST
Attachment 359847 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/NetworkProcess.cpp:128:  Code inside a namespace should not be indented.  [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.
Comment 16 Alex Christensen 2019-01-23 14:36:22 PST
Created attachment 359944 [details]
Patch
Comment 17 Alex Christensen 2019-01-23 16:30:02 PST
http://trac.webkit.org/r240366
Comment 18 Radar WebKit Bug Importer 2019-01-23 16:31:39 PST
<rdar://problem/47499170>