Bug 193700 – Stop using NetworkProcess::singleton

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

[Alex Christensen]

Stop using NetworkProcess::singleton

[Alex Christensen]

Created attachment 359823 [details]
Patch

[Alex Christensen]

Created attachment 359829 [details]
Patch

[Sam Weinig]

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?

[Alex Christensen]

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.

[Alex Christensen]

Created attachment 359838 [details]
Patch

[Daniel Bates]

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?

[Daniel Bates]

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

[Sam Weinig]

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.

[Sam Weinig]

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>.

[Alex Christensen]

(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.

[Alex Christensen]

(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.

[Alex Christensen]

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.

[Alex Christensen]

Created attachment 359847 [details]
Patch

[Alex Christensen]

(In reply to Daniel Bates from comment #6)
> > +    : m_downloadManager(*this)
> Uniform initializer syntax?
Stylebot doesn't like this, but I did it anyways.

[EWS Watchlist]

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.

[Alex Christensen]

Created attachment 359944 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r240366

[Radar WebKit Bug Importer]

<rdar://problem/47499170>