flypig.co.uk

I did very little coding yesterday and yet it was a very fruitful day of development. After a discussion with Frajo I was left with several useful outcomes:

1. Frajo and mal have crafted a nice pull request to fix the Wayland dynamic loading issue.
2. I need to update my workaround so it can be controlled using an environment variable.
3. The default preferences need fixing for WebView apps.
4. The reason for the Email app looking for libxul.so in the wrong directory needs investigation.

It's helpful to be clear about these points because it feels like we're reaching some kind of end state. Once these tasks are completed I'm planning to offer a release for others to test out. A little bit of structure helps me focus and take stock. There are only a few more things to do.

So the first task is to add an environment variable to control whether or not the Wayland EGL dynamic library loading workaround is applied or not. In practice I hope the workaround can be entirely removed before any proper release, but it's needed in the meantime on the current Sailfish OS 4.6 release, so it needs to be kept for now.

Choosing a suitable name for the environment variable turns out to be the hardest part of the task. I've gone for DISABLE_PLAT_EGL_FIX. When this is left unset the workaround will be applied. When set to 1, the workaround will be skipped.

When Frajo requested this environment variable I forgot to check whether he wants a flag to enable the workaround or a flag to disable it. I've gone with a flag to disable it. So it's possible I may need to flip it around at some later date. But for now, I just need something that does the job.

So I've made the following changes to the previous fix in the qtmozembed code:

$ git diff
diff --git a/src/qmozcontext.cpp b/src/qmozcontext.cpp
index 4284895..57622dd 100644
--- a/src/qmozcontext.cpp
+++ b/src/qmozcontext.cpp
@@ -45,10 +45,11 @@ Q_GLOBAL_STATIC(QMozContextPrivate, 
    mozContextPrivateInstance)
 // to a lack of reference counting in ws_init and ws_Terminate here:
 // https://github.com/libhybris/libhybris/blob/master/hybris/egl/ws.c#L99
 // If this can be fixed in libhybris then the loading here can be removed
+static const bool platformEglWorkaround = !getenv(
    "DISABLE_PLAT_EGL_FIX");
 static void *platformEglHandle = nullptr;
 
 static void platform_egl_workaround_open() {
-  if (platformEglHandle) {
+  if (platformEglHandle || !platformEglWorkaround) {
     // Only load once
     return;
   }
@@ -78,8 +79,9 @@ static void platform_egl_workaround_open() {
 }
 
 static void platform_egl_workaround_close() {
-  if (platformEglHandle) {
+  if (platformEglHandle && platformEglWorkaround) {
     dlclose(platformEglHandle);
+    platformEglHandle = nullptr;
   }
 } 

It's a small change but required a bit of thought. To ensure consistency I need the environment variable to be stored once and then never changed. This constant value must then be used in all places in the code. The above approach is intended to fulfil this. I also thought about integrating the workaround fully into the QMozContextPrivate class. For permanent code this would make sense, but if the plan is to remove this workaround later, then I think it's neater to keep all of the changes minimal and in a single source file.

Having applied these changes and built the packages I then reverted Frajo's workaround on my device and checked that the environment variable does what it's supposed to do. As intended we get a crash when executing the following:

$ DISABLE_PLAT_EGL_FIX=1 harbour-webview

Whereas both of the following commands work successfully:

$ DISABLE_PLAT_EGL_FIX=0 harbour-webview 
$ harbour-webview

So that's the first task completed. Next up is fixing the preferences when running the WebView. The problem seems to be the gfx.webrender.force-disabled preference. When this is set to false the browser or WebView crash, so it needs to default to true. This can all be tested by changing the setting in the prefs.js file, with an entry like this:

user_pref("gfx.webrender.force-disabled", true);

Some care is needed though because each app that uses the WebView has its own profile, so there's actually a different prefs.js file stored for each WebView app. This is intentional: different apps may need different settings. But for this particular setting, we really only ever want it to be set to true. For all intents and purposes the option may as well be renamed do.not.crash.

In the gecko code the gfx.webrender.force-disabled configuration option is defined in the StaticPrefList.yaml file:

# Also expose a pref to allow users to force-disable WR. This is exposed
# on all channels because WR can be enabled on qualified hardware on all
# channels.
- name: gfx.webrender.force-disabled
  type: bool
  value: false
  mirror: once

When I looked at this previously I decided not to change this default setting and instead to explicitly set the value in WebEngineSettings::initialize() (part of the sailfish-components-webview project). Unfortunately my recent testing has shown that this isn't enough. It works some of the time, but there seems to be a race condition. If the WebEngineSettings is initialised fast enough the setting goes correctly through to the gecko engine and the application runs fine. But on occasion it happens too late and a crash ensues.

My original motivation for doing it this way was to try to avoid an unnecessary patch on the gecko code. Every patch is a potential pain point, so the fewer the better. But since the alternative approach doesn't seem to be working properly in all cases, changing the default looks like it'll be both the simplest and most effective solution.

Consequently I've changed the default value in the StaticPrefList.yaml file like this:

$ git diff
diff --git a/modules/libpref/init/StaticPrefList.yaml b/modules/libpref/init/
    StaticPrefList.yaml
index 9c8d10557622..96c924f00398 100644
--- a/modules/libpref/init/StaticPrefList.yaml
+++ b/modules/libpref/init/StaticPrefList.yaml
@@ -5067,7 +5067,7 @@
 # channels.
 - name: gfx.webrender.force-disabled
   type: bool
-  value: false
+  value: true
   mirror: once
 
 #ifdef MOZ_WIDGET_GTK

Unfortunately this change requires a rebuild of the gecko library, so I've set it going and it'll run for quite a while. I won't be able to test the change until it's completed. So that gives me a chance to check out the final issue, which is the location the email app is searching for the libuxl.so library.

The directory of the library is version-encoded. So for example the ESR 78 directory is /usr/lib64/xulrunner-qt5-78.15.1 whereas on ESR 91 it's /usr/lib64/xulrunner-qt5-91.9.1.

Adding a symlink so that the email app thinks the latter is actually the former allows the app to work. But the obvious question arises as to why there's a hard-coded directory in use at all. Why doesn't the email app just include sailfish-components-webview-qt5 and let that do the work of loading libxul.so?

A bit more investigation shows that the most likely reason for this is that the libxul.so directory is contained in the binary's rpath:

$ objdump -x /usr/bin/jolla-email | grep 'R.*PATH'
  RPATH                /usr/lib64/xulrunner-qt5-78.15.1

This is most likely coming from the following pkg-config output:

$ pkg-config --cflags sailfishwebengine
-I/usr/include/libsailfishwebengine -I/usr/include/qt5/QtCore -I/usr/include/
    qt5 -I/usr/lib64 -I/usr/include/xulrunner-qt5-91.9.1 -I/usr/include/nspr4

This is happening in the jolla-email code which, unfortunately, is one of Sailfish OS's closed components. So unfortunately I can only speculate as to how or why this might manifest itself in the sourcecode. Most likely it's pulled in using a PKCONFIG statement in a qmake *.pro file.

There are tools such as chrpath that can be used to alter the rpath of an executable, but that's a bit extreme. In fact, if my speculation is correct, this issue will be fixed automatically when the jolla-email binary is built against the ESR 91 packages. In the meantime, it's easy to work around it by creating a symlink from one path to the other:

$ ln -s /usr/lib64/xulrunner-qt5-91.9.1/ /usr/lib64/xulrunner-qt5-78.15.1

That can act as a temporary workaround for users wanting to test ESR 91 until the official release (assuming there is one), when it'll no longer be an issue.

Finally, some of you may have noticed this warning has started appearing in the browser's console output ever since switching to Sailfish OS 4.6:

[W] unknown:0 - MeeGo.QOfono QML module name is deprecated and subject for 
    removal. Please adapt code to "import QOfono".

There's a bit of a complex history to this. In Sailfish OS 4.6 the MeeGo.QOfono import changed its name to QOfono. The version of ESR 78 that comes with 4.6 was therefore updated to use the new naming scheme. Pekka made this change on the development branch back in 2023:

$ git log -1 16ef5cdf4
commit 16ef5cdf44c2eafd7d93e17a41927ef5da700c2b
Author: Pekka Vuorela <pekka.vuorela@jolla.com>
Date:   Thu Jan 5 12:09:27 2023 +0200

    [components-webview] Migrate to new qofono import. JB#59690
    
    Also dependency was missing.

However this was before 4.6 was released and Sailfish OS 4.5 required the previous name to be used. Consequently I had to revert this change in order to get the browser to run on my development device:

$ git log -1 d5d10ac14
commit d5d10ac14d9a07db664d208919452559d76aeac9
Author: David Llewellyn-Jones <david.llewellyn-jones@jolla.com>
Date:   Fri Jul 19 07:44:11 2024 +0100

    [sailfish-webview] Migrate away from new ofono import
    
    Commit 16ef5cdf4 switched to the new ofono import, but this causes
    issues for me, possibly becaues I'm on an old SDK. So this commit should
    almost certainly be removed, but I'm including it for completeness.
    
    Reverts 16ef5cdf4.

Now that I'm on 4.6 and the new naming scheme works, I need to revert my revert. Which is exactly what I've now done.

$ git revert d5d10ac14
$ git log -1
commit dd86cfd1306540adcae7021c8cdeb880f433d081 (HEAD -> sailfishos-esr91)
Author: David Llewellyn-Jones <david.llewellyn-jones@jolla.com>
Date:   Mon Aug 26 16:48:27 2024 +0100

    Revert &quot;[sailfish-webview] Migrate away from new ofono import&quot;
    
    This reverts commit d5d10ac14d9a07db664d208919452559d76aeac9.

Before I wrap up, the build has now finished and I'm happy to say that the prefs.js fix seems to have done the trick. The Settings app, email app and various other WebView apps I've tried now all work correctly.

That's all quite a lot to have got through today and, by my reckoning, it brings all of my bug-squashing tasks to a close. But not quite all of my tasks. Tomorrow I'll package everything up and write instructions so that others can test out the packages. After that I'll start arranging the patches.

If you'd like to read any of my other gecko diary entries, they're all available on my Gecko-dev Diary page.