Bug 186841 – [WPE] Pass the backend library name as command line parameter to the web process

• Component: WPE WebKit
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

We are currently using the environment variable in the MiniBrowser and tests, but that is only available when building WPEBackend with debug, so we shouldn't rely on it. We first need to add api to WPE to load a given library instead of using the magic (env var + symlink).

[Carlos Garcia Campos]

Created attachment 343145 [details]
WIP

This is WIP because it requires a WPEBackend patch that needs to land first, then I'll update the jhbuild.

[Zan Dobersek]

Comment on attachment 343145 [details]
WIP

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

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:139
> +    if (m_launchOptions.processType == ProcessLauncher::ProcessType::Web) {
>          argv[i++] = wpeSocket.get();
> +        if (wpeBackendLibraryParameter)
> +            argv[i++] = wpeBackendLibraryParameter.get();
> +    }

IMO the backend library name should be provided unconditionally. That means wpe_loader_get_loaded_library_name() would always return the appropriate library name.

I would also reorganize the WebProcess argument list:
$ WPEWebProcess <library.so> <wpe-fd> <wk-fd>

[Carlos Garcia Campos]

(In reply to Zan Dobersek from comment #2)
> Comment on attachment 343145 [details]
> WIP
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=343145&action=review
> 
> > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:139
> > +    if (m_launchOptions.processType == ProcessLauncher::ProcessType::Web) {
> >          argv[i++] = wpeSocket.get();
> > +        if (wpeBackendLibraryParameter)
> > +            argv[i++] = wpeBackendLibraryParameter.get();
> > +    }
> 
> IMO the backend library name should be provided unconditionally. That means
> wpe_loader_get_loaded_library_name() would always return the appropriate
> library name.

But we can't know if the application called wpe_loader_init() at this point, and would break if using a previous version of WPEBackend. Actually, this patch is bumping the requirement, unless we add a way to make it conditional at build type.

> I would also reorganize the WebProcess argument list:
> $ WPEWebProcess <library.so> <wpe-fd> <wk-fd>

We can't do that, process and connection ids are handled by common code in ChildProcessMainBase::parseCommandLine(), so they need to be first. I don't think this is important anyway, since the web process is not expected to be launched by the user.

[Zan Dobersek]

(In reply to Carlos Garcia Campos from comment #3)
> (In reply to Zan Dobersek from comment #2)
> > Comment on attachment 343145 [details]
> > WIP
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=343145&action=review
> > 
> > > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:139
> > > +    if (m_launchOptions.processType == ProcessLauncher::ProcessType::Web) {
> > >          argv[i++] = wpeSocket.get();
> > > +        if (wpeBackendLibraryParameter)
> > > +            argv[i++] = wpeBackendLibraryParameter.get();
> > > +    }
> > 
> > IMO the backend library name should be provided unconditionally. That means
> > wpe_loader_get_loaded_library_name() would always return the appropriate
> > library name.
> 
> But we can't know if the application called wpe_loader_init() at this point,
> and would break if using a previous version of WPEBackend. Actually, this
> patch is bumping the requirement, unless we add a way to make it conditional
> at build type.
> 

By this point, the UIProcess already had to load the implementation library since it was needed to create the wpe_view_backend instance. It's needed anyway in the next line in ProcessLauncherGLib when the wpe_renderer_host instance is asked to create a client.

If the application hasn't called wpe_loader_init() by now (while it should have called it at the very beginning of program's lifetime), it's already too late, and either the compile-time WPE_BACKEND, libWPEBackend-default.so, or the env-provided library name (in debug builds) was used. That's what should be passed on the command line.

> > I would also reorganize the WebProcess argument list:
> > $ WPEWebProcess <library.so> <wpe-fd> <wk-fd>
> 
> We can't do that, process and connection ids are handled by common code in
> ChildProcessMainBase::parseCommandLine(), so they need to be first. I don't
> think this is important anyway, since the web process is not expected to be
> launched by the user.

WebProcessMain implementation could adjust the argv and argc arguments that are passed to ChildProcessMainBase::parseCommandLine(), but OK, let's not do that.

[Carlos Garcia Campos]

Created attachment 343490 [details]
WIP

Updated patch addressing review comments and using version checks to make it work with previous versions of WPEBackend.

[Carlos Garcia Campos]

I'm using the current version of WPEBackend in the checks, but we should make a new release before landing this, and use the new version number 0.1.1 or 0.2.0.

[Adrian Perez]

Comment on attachment 343490 [details]
WIP

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

Looking good overall, just a couple of small comments below :-)

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:105
> +        wpeBackendLibraryParameter.reset(g_strdup_printf("--backend-library=%s", wpe_loader_get_loaded_implementation_library_name()));

Probably it's just fine to remove the “--backend-library=” prefix.

> Source/WebKit/WebProcess/wpe/WebProcessMainWPE.cpp:71
> +            if (!strncmp(argv[4], "--backend-library=", parameterLength))

Ditto.

> Tools/wpe/backends/ViewBackend.cpp:39
> +    wpe_loader_init("libWPEBackend-fdo-0.1.so");

Maybe it would be good to construct the backend library name to load with:

  GUniquePtr<char> fdoBackendName(g_strdup_printf("libWPEBackend-fdo-%i.%i.so"),
                                                  WPE_BACKEND_MAJOR_VERSION,
                                                  WPE_BACKEND_MINOR_VERSION));
  wpe_loader_init(fdoBackendName.get());

This way we ensure that a backend implementation which is API/ABI-compatible
with the version of libWPEBackend that WPE WebKit is being built against.
(Also it's one place less to manually change strings when versions change.)

[Carlos Garcia Campos]

(In reply to Adrian Perez from comment #7)
> Comment on attachment 343490 [details]
> WIP
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=343490&action=review
> 
> Looking good overall, just a couple of small comments below :-)
> 
> > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:105
> > +        wpeBackendLibraryParameter.reset(g_strdup_printf("--backend-library=%s", wpe_loader_get_loaded_implementation_library_name()));
> 
> Probably it's just fine to remove the “--backend-library=” prefix.

Yes, probably, I'm trying to avoid passing other stuff to wpe loader if we mess it up with the order of the arguments.

> > Source/WebKit/WebProcess/wpe/WebProcessMainWPE.cpp:71
> > +            if (!strncmp(argv[4], "--backend-library=", parameterLength))
> 
> Ditto.
> 
> > Tools/wpe/backends/ViewBackend.cpp:39
> > +    wpe_loader_init("libWPEBackend-fdo-0.1.so");
> 
> Maybe it would be good to construct the backend library name to load with:
> 
>   GUniquePtr<char>
> fdoBackendName(g_strdup_printf("libWPEBackend-fdo-%i.%i.so"),
>                                                   WPE_BACKEND_MAJOR_VERSION,
>                                                  
> WPE_BACKEND_MINOR_VERSION));
>   wpe_loader_init(fdoBackendName.get());
> 
> This way we ensure that a backend implementation which is API/ABI-compatible
> with the version of libWPEBackend that WPE WebKit is being built against.
> (Also it's one place less to manually change strings when versions change.)

That doesn't work this way, you are mixing the project version with the library version. libWPEBackend-fdo-0.1.so will always be API/ABI compatible, it's actually a symlink to the latest version of the library. That 0.1 isn't going to change when we release version 0.2, for example, because that's not the project version, but the API version. If we make a new release that breaks the API/ABI then we will bump the API version, and we will have to update it manually.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #8)
> That doesn't work this way, you are mixing the project version with the
> library version. libWPEBackend-fdo-0.1.so will always be API/ABI compatible,
> it's actually a symlink to the latest version of the library. That 0.1 isn't
> going to change when we release version 0.2, for example, because that's not
> the project version, but the API version. If we make a new release that
> breaks the API/ABI then we will bump the API version, and we will have to
> update it manually.

I would say it will certainly always be API compatible, but not necessarily ABI compatible. E.g. we are going to need a soname bump for WebKitGTK+ sooner or later, because we are running out of space in the WebKitWebView class struct, but we are surely not going to raise the API version when this happens (which would be far more disruptive).

But yes, Zan's suggestion would only work if we increase the API version on every release, which would be undesirable.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #9)
> (In reply to Carlos Garcia Campos from comment #8)
> > That doesn't work this way, you are mixing the project version with the
> > library version. libWPEBackend-fdo-0.1.so will always be API/ABI compatible,
> > it's actually a symlink to the latest version of the library. That 0.1 isn't
> > going to change when we release version 0.2, for example, because that's not
> > the project version, but the API version. If we make a new release that
> > breaks the API/ABI then we will bump the API version, and we will have to
> > update it manually.
> 
> I would say it will certainly always be API compatible, but not necessarily
> ABI compatible. E.g. we are going to need a soname bump for WebKitGTK+
> sooner or later, because we are running out of space in the WebKitWebView
> class struct, but we are surely not going to raise the API version when this
> happens (which would be far more disruptive).

We will bump the api version every time we break either api or abi.

> But yes, Zan's suggestion would only work if we increase the API version on
> every release, which would be undesirable.

It was Adri's suggestion, not Zan's.

[Carlos Garcia Campos]

Created attachment 344677 [details]
Patch

[Zan Dobersek]

Comment on attachment 344677 [details]
Patch

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

> Source/WebKit/WebProcess/wpe/WebProcessMainWPE.cpp:74
> +#if defined(WPE_BACKEND_CHECK_VERSION) && WPE_BACKEND_CHECK_VERSION(0, 2, 0)
> +        if (argc > 4) {
> +            static const unsigned parameterLength = strlen("--backend-library=");
> +            if (!strncmp(argv[4], "--backend-library=", parameterLength))
> +                wpe_loader_init(argv[4] + parameterLength);
> +        }
> +#endif

Like discussed, IMO this should use '-' as a token when no library is specified. The token would be placed before the WPE fd, and the argc should be strictly 5 in value.

[Carlos Garcia Campos]

Created attachment 344682 [details]
Patch

[Zan Dobersek]

Comment on attachment 344682 [details]
Patch

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

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:106
> +#if defined(WPE_BACKEND_CHECK_VERSION) && WPE_BACKEND_CHECK_VERSION(0, 2, 0)
> +        wpeBackendLibraryParameter.reset(g_strdup_printf("--backend-library=%s", wpe_loader_get_loaded_implementation_library_name()));
> +#endif

Do we need the --backend-library flag?

> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:137
> +        argv[i++] = wpeBackendLibraryParameter ? wpeBackendLibraryParameter.get() : "-";

I don't think wpeBackendLibraryParameter can be null if you use g_strdup_printf()? Unless we can remove the --backend-library flag and just strdup the impl library name, if any.

[EWS Watchlist]

Comment on attachment 344682 [details]
Patch

Attachment 344682 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8492806

New failing tests:
http/tests/security/canvas-remote-read-remote-video-localhost.html

[EWS Watchlist]

Created attachment 344685 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Carlos Garcia Campos]

(In reply to Zan Dobersek from comment #14)
> Comment on attachment 344682 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344682&action=review
> 
> > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:106
> > +#if defined(WPE_BACKEND_CHECK_VERSION) && WPE_BACKEND_CHECK_VERSION(0, 2, 0)
> > +        wpeBackendLibraryParameter.reset(g_strdup_printf("--backend-library=%s", wpe_loader_get_loaded_implementation_library_name()));
> > +#endif
> 
> Do we need the --backend-library flag?

Comment #8

"Yes, probably, I'm trying to avoid passing other stuff to wpe loader if we mess it up with the order of the arguments."

I can remove it if you don't like it.

> > Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:137
> > +        argv[i++] = wpeBackendLibraryParameter ? wpeBackendLibraryParameter.get() : "-";
> 
> I don't think wpeBackendLibraryParameter can be null if you use
> g_strdup_printf()? Unless we can remove the --backend-library flag and just
> strdup the impl library name, if any.

It can be nullptr when building with previous versions of WPEBackend.

[Zan Dobersek]

Comment on attachment 344682 [details]
Patch

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

>>> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:106
>>> +#endif
>> 
>> Do we need the --backend-library flag?
> 
> Comment #8
> 
> "Yes, probably, I'm trying to avoid passing other stuff to wpe loader if we mess it up with the order of the arguments."
> 
> I can remove it if you don't like it.

It's out-of-place in that argument array, IMO.

Regardless of that, should the library name be escaped somehow before it's included in the arguments? Is there any way in which the impl library name alone could cause trouble? Should we use FileSystem::fileSystemRepresentation() like we do for other filenames of executables?

>>> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:137
>>> +        argv[i++] = wpeBackendLibraryParameter ? wpeBackendLibraryParameter.get() : "-";
>> 
>> I don't think wpeBackendLibraryParameter can be null if you use g_strdup_printf()? Unless we can remove the --backend-library flag and just strdup the impl library name, if any.
> 
> It can be nullptr when building with previous versions of WPEBackend.

Let's ensure it's non-null by assigning it the "-" token by default, and override it when compiling against the newer WPEBackend lib.

[Carlos Garcia Campos]

(In reply to Zan Dobersek from comment #18)
> Comment on attachment 344682 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=344682&action=review
> 
> >>> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:106
> >>> +#endif
> >> 
> >> Do we need the --backend-library flag?
> > 
> > Comment #8
> > 
> > "Yes, probably, I'm trying to avoid passing other stuff to wpe loader if we mess it up with the order of the arguments."
> > 
> > I can remove it if you don't like it.
> 
> It's out-of-place in that argument array, IMO.
> 
> Regardless of that, should the library name be escaped somehow before it's
> included in the arguments? Is there any way in which the impl library name
> alone could cause trouble? Should we use
> FileSystem::fileSystemRepresentation() like we do for other filenames of
> executables?

That's a good point, I'll use fileSystemRepresantation.

> >>> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:137
> >>> +        argv[i++] = wpeBackendLibraryParameter ? wpeBackendLibraryParameter.get() : "-";
> >> 
> >> I don't think wpeBackendLibraryParameter can be null if you use g_strdup_printf()? Unless we can remove the --backend-library flag and just strdup the impl library name, if any.
> > 
> > It can be nullptr when building with previous versions of WPEBackend.
> 
> Let's ensure it's non-null by assigning it the "-" token by default, and
> override it when compiling against the newer WPEBackend lib.

I don't see the point here. We would be heap allocating "-", just to free and allocate again in the case of new wpebackend, which will be the common one. I don't see the problem with the check here and using a static string "-".

[Zan Dobersek]

Comment on attachment 344682 [details]
Patch

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

>>>>> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:106
>>>>> +#endif
>>>> 
>>>> Do we need the --backend-library flag?
>>> 
>>> Comment #8
>>> 
>>> "Yes, probably, I'm trying to avoid passing other stuff to wpe loader if we mess it up with the order of the arguments."
>>> 
>>> I can remove it if you don't like it.
>> 
>> It's out-of-place in that argument array, IMO.
>> 
>> Regardless of that, should the library name be escaped somehow before it's included in the arguments? Is there any way in which the impl library name alone could cause trouble? Should we use FileSystem::fileSystemRepresentation() like we do for other filenames of executables?
> 
> That's a good point, I'll use fileSystemRepresantation.

r=me with this change.

>>>>> Source/WebKit/UIProcess/Launcher/glib/ProcessLauncherGLib.cpp:137
>>>>> +        argv[i++] = wpeBackendLibraryParameter ? wpeBackendLibraryParameter.get() : "-";
>>>> 
>>>> I don't think wpeBackendLibraryParameter can be null if you use g_strdup_printf()? Unless we can remove the --backend-library flag and just strdup the impl library name, if any.
>>> 
>>> It can be nullptr when building with previous versions of WPEBackend.
>> 
>> Let's ensure it's non-null by assigning it the "-" token by default, and override it when compiling against the newer WPEBackend lib.
> 
> I don't see the point here. We would be heap allocating "-", just to free and allocate again in the case of new wpebackend, which will be the common one. I don't see the problem with the check here and using a static string "-".

Makes sense.

[Carlos Garcia Campos]

Committed r233763: <https://trac.webkit.org/changeset/233763>