Bug 194830 - [WPE] Do not create a PlatformDisplay in the Service Worker process
Summary: [WPE] Do not create a PlatformDisplay in the Service Worker process
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WPE WebKit (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-02-19 14:48 PST by Loïc Yhuel
Modified: 2019-03-05 03:59 PST (History)
6 users (show)

See Also:


Attachments
Patch (3.26 KB, patch)
2019-02-19 15:01 PST, Loïc Yhuel
no flags Details | Formatted Diff | Diff
Patch (6.82 KB, patch)
2019-02-25 13:12 PST, Loïc Yhuel
no flags Details | Formatted Diff | Diff
Patch (6.79 KB, patch)
2019-02-26 02:28 PST, Loïc Yhuel
no flags Details | Formatted Diff | Diff
Patch (5.93 KB, patch)
2019-02-27 17:39 PST, Loïc Yhuel
no flags Details | Formatted Diff | Diff
Patch (6.39 KB, patch)
2019-03-04 10:29 PST, Loïc Yhuel
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Loïc Yhuel 2019-02-19 14:48:33 PST
[WPE] Do not create a PlatformDisplay in the Service Worker process
Comment 1 Loïc Yhuel 2019-02-19 15:01:54 PST
Created attachment 362440 [details]
Patch
Comment 2 Carlos Garcia Campos 2019-02-20 00:25:36 PST
Comment on attachment 362440 [details]
Patch

I think this will be easier after bug #194494
Comment 3 Loïc Yhuel 2019-02-20 02:39:01 PST
(In reply to Carlos Garcia Campos from comment #2)
> Comment on attachment 362440 [details]
> Patch
> 
> I think this will be easier after bug #194494

We would need to add a parameter to WebProcessPool::platformInitializeWebProcess (in WebProcessPool::initializeNewWebProcess, the argument would be process.isServiceWorkerProcess()).
Comment 4 Carlos Garcia Campos 2019-02-20 02:46:41 PST
(In reply to Loïc Yhuel from comment #3)
> (In reply to Carlos Garcia Campos from comment #2)
> > Comment on attachment 362440 [details]
> > Patch
> > 
> > I think this will be easier after bug #194494
> 
> We would need to add a parameter to
> WebProcessPool::platformInitializeWebProcess (in
> WebProcessPool::initializeNewWebProcess, the argument would be
> process.isServiceWorkerProcess()).

I would add an implementation of WebProcess::platformInitializeProcess to initialize m_processType (we need to make this non-mac specific in WebProcess.h). Then you can simply check m_pocessType from platformInitializeWebProcess
Comment 5 Loïc Yhuel 2019-02-20 04:11:30 PST
(In reply to Carlos Garcia Campos from comment #4)
> (In reply to Loïc Yhuel from comment #3)
> > (In reply to Carlos Garcia Campos from comment #2)
> > > Comment on attachment 362440 [details]
> > > Patch
> > > 
> > > I think this will be easier after bug #194494
> > 
> > We would need to add a parameter to
> > WebProcessPool::platformInitializeWebProcess (in
> > WebProcessPool::initializeNewWebProcess, the argument would be
> > process.isServiceWorkerProcess()).
> 
> I would add an implementation of WebProcess::platformInitializeProcess to
> initialize m_processType (we need to make this non-mac specific in
> WebProcess.h). Then you can simply check m_pocessType from
> platformInitializeWebProcess

WebProcess::platformInitializeProcess is on WebProcess side, we need a test on the UIProcess side to avoid creating the fd.
Comment 6 Loïc Yhuel 2019-02-25 13:12:22 PST
Created attachment 362919 [details]
Patch
Comment 7 Loïc Yhuel 2019-02-26 02:28:03 PST
Created attachment 362973 [details]
Patch
Comment 8 Zan Dobersek 2019-02-27 05:29:31 PST
Comment on attachment 362973 [details]
Patch

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

> Source/WebKit/UIProcess/glib/WebProcessPoolGLib.cpp:95
> -void WebProcessPool::platformInitializeWebProcess(WebProcessCreationParameters& parameters)
> +void WebProcessPool::platformInitializeWebProcess(WebProcessCreationParameters& parameters, bool isServiceWorkerProcess)
>  {
>  #if PLATFORM(WPE)
> -    parameters.hostClientFileDescriptor = wpe_renderer_host_create_client();
> +    if (!isServiceWorkerProcess)
> +        parameters.hostClientFileDescriptor = wpe_renderer_host_create_client();

Instead of the extra bool parameter to this method, there should be an extra bool variable `isServiceWorkerProcess` added to WebProcessCreationParameters for the WPE port. It would be initialized in WebProcessPool::initializeNewWebProcess() before the platformInitializeWebProcess() call.
Comment 9 Michael Catanzaro 2019-02-27 07:26:54 PST
Problem is it's not actually used in the web process.
Comment 10 Loïc Yhuel 2019-02-27 09:49:33 PST
(In reply to Zan Dobersek from comment #8)
> Comment on attachment 362973 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=362973&action=review
> 
> > Source/WebKit/UIProcess/glib/WebProcessPoolGLib.cpp:95
> > -void WebProcessPool::platformInitializeWebProcess(WebProcessCreationParameters& parameters)
> > +void WebProcessPool::platformInitializeWebProcess(WebProcessCreationParameters& parameters, bool isServiceWorkerProcess)
> >  {
> >  #if PLATFORM(WPE)
> > -    parameters.hostClientFileDescriptor = wpe_renderer_host_create_client();
> > +    if (!isServiceWorkerProcess)
> > +        parameters.hostClientFileDescriptor = wpe_renderer_host_create_client();
> 
> Instead of the extra bool parameter to this method, there should be an extra
> bool variable `isServiceWorkerProcess` added to WebProcessCreationParameters
> for the WPE port. It would be initialized in
> WebProcessPool::initializeNewWebProcess() before the
> platformInitializeWebProcess() call.

So in WebProcessPool::initializeNewWebProcess :
#if PLATFORM(WPE) && ENABLE(SERVICE_WORKER)
    parameters.isServiceWorkerProcess = process.isServiceWorkerProcess();
#endif
Comment 11 Loïc Yhuel 2019-02-27 17:39:25 PST
Created attachment 363167 [details]
Patch
Comment 12 Loïc Yhuel 2019-02-27 17:49:58 PST
(In reply to Zan Dobersek from comment #8)
> Instead of the extra bool parameter to this method, there should be an extra
> bool variable `isServiceWorkerProcess` added to WebProcessCreationParameters
> for the WPE port. It would be initialized in
> WebProcessPool::initializeNewWebProcess() before the
> platformInitializeWebProcess() call.

Done.

I did put in under "#if ENABLE(SERVICE_WORKER)", but I don't know if it's the right choice, since it makes the patch more complicated since those flags must be replicated in several places.

The code style guidelines also don't mention the case of flags mixed with blocks, and I didn't find examples when searching quickly.

First solution, with the "if (1)" to avoid any possible -Wmisleading-indentation :
#if ENABLE(SERVICE_WORKER)
    if (!parameters.isServiceWorkerProcess)
#else
    if (1)
#endif
        parameters.hostClientFileDescriptor = wpe_renderer_host_create_client();

Second solution, with a one-line block (would be against the rules without the #if) :
#if ENABLE(SERVICE_WORKER)
    if (!parameters.isServiceWorkerProcess)
#endif
    {
        parameters.hostClientFileDescriptor = wpe_renderer_host_create_client();
    }
Comment 13 Zan Dobersek 2019-03-01 00:29:50 PST
(In reply to Michael Catanzaro from comment #9)
> Problem is it's not actually used in the web process.

OK, let's then make this more WPE-specific.
Comment 14 Zan Dobersek 2019-03-01 00:34:58 PST
Comment on attachment 363167 [details]
Patch

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

> Source/WebKit/Shared/WebProcessCreationParameters.h:207
> +#if ENABLE(SERVICE_WORKER)
> +    bool isServiceWorkerProcess { false };
> +#endif

Let's rename this to `isWPEClient`, and place it above the other two member variables here. Decoding/encoding sequencing should follow this order. It should also default to true, and not be guarded with ENABLE(SERVICE_WORKER).

> Source/WebKit/UIProcess/WebProcessPool.cpp:944
> +#if PLATFORM(WPE) && ENABLE(SERVICE_WORKER)
> +    parameters.isServiceWorkerProcess = process.isServiceWorkerProcess();
> +#endif

Here, `isWPEClient` should be set to the negative value of `process.isServiceWorkerProcess()`. PLATFORM(WPE) build guards are fitting, SERVICE_WORKER maybe not since the `isServiceWorkerProcess()` getter isn't guarded with it.

> Source/WebKit/UIProcess/glib/WebProcessPoolGLib.cpp:98
> +#if ENABLE(SERVICE_WORKER)
> +    if (!parameters.isServiceWorkerProcess)
> +#else
> +    if (1)
> +#endif

This would now be a simple, unguarded check of `isWPEClient` value.

> Source/WebKit/WebProcess/glib/WebProcessGLib.cpp:59
> +#if ENABLE(SERVICE_WORKER)
> +    if (!parameters.isServiceWorkerProcess)
> +#endif

Ditto.
Comment 15 Loïc Yhuel 2019-03-01 05:22:10 PST
(In reply to Zan Dobersek from comment #14)
> Comment on attachment 363167 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=363167&action=review
> 
> > Source/WebKit/Shared/WebProcessCreationParameters.h:207
> > +#if ENABLE(SERVICE_WORKER)
> > +    bool isServiceWorkerProcess { false };
> > +#endif
> 
> Let's rename this to `isWPEClient`, and place it above the other two member
> variables here. Decoding/encoding sequencing should follow this order. It
> should also default to true, and not be guarded with ENABLE(SERVICE_WORKER).
> 
Then, should it be tested for implementationLibraryName usage too (ie not calling wpe_loader_init in the Service Worker process) ?

The specific `isWPEClient` name might not be appropriate if we later find other things we can skip for Service Workers : for example the `WebCore::initializeGStreamer` in `WebProcess::platformInitializeWebProcess` is probably not needed.
Comment 16 Zan Dobersek 2019-03-04 05:59:32 PST
(In reply to Loïc Yhuel from comment #15)
> (In reply to Zan Dobersek from comment #14)
> > Comment on attachment 363167 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=363167&action=review
> > 
> > > Source/WebKit/Shared/WebProcessCreationParameters.h:207
> > > +#if ENABLE(SERVICE_WORKER)
> > > +    bool isServiceWorkerProcess { false };
> > > +#endif
> > 
> > Let's rename this to `isWPEClient`, and place it above the other two member
> > variables here. Decoding/encoding sequencing should follow this order. It
> > should also default to true, and not be guarded with ENABLE(SERVICE_WORKER).
> > 
> Then, should it be tested for implementationLibraryName usage too (ie not
> calling wpe_loader_init in the Service Worker process) ?
> 

Yes, the wpe_loader_init() call should also be avoided.

> The specific `isWPEClient` name might not be appropriate if we later find
> other things we can skip for Service Workers : for example the
> `WebCore::initializeGStreamer` in `WebProcess::platformInitializeWebProcess`
> is probably not needed.

Makes sense. We can use the current name and expand its use over GStreamer in a future patch.
Comment 17 Loïc Yhuel 2019-03-04 09:26:07 PST
(In reply to Zan Dobersek from comment #16)
> Makes sense. We can use the current name and expand its use over GStreamer
> in a future patch.
What do you mean by current ? `isWPEClient` or `isServiceWorkerProcess` ?
Comment 18 Zan Dobersek 2019-03-04 10:19:45 PST
(In reply to Loïc Yhuel from comment #17)
> (In reply to Zan Dobersek from comment #16)
> > Makes sense. We can use the current name and expand its use over GStreamer
> > in a future patch.
> What do you mean by current ? `isWPEClient` or `isServiceWorkerProcess` ?

`isServiceWorkerProcess` is fine. It can always be amended afterwards, anyway.
Comment 19 Loïc Yhuel 2019-03-04 10:29:50 PST
Created attachment 363524 [details]
Patch
Comment 20 WebKit Commit Bot 2019-03-05 03:59:33 PST
Comment on attachment 363524 [details]
Patch

Clearing flags on attachment: 363524

Committed r242437: <https://trac.webkit.org/changeset/242437>
Comment 21 WebKit Commit Bot 2019-03-05 03:59:35 PST
All reviewed patches have been landed.  Closing bug.