flypig.co.uk

It's the morning, the world is waking up after a night of torrential rain here in Cambridgeshire. And my build has completed.

I've copied the packages over to my development phone, installed them and have them ready to run. The thing I'm testing today is the change I made over the last couple of days to fix the preferences that were using the VarCache, but which are now using static preferences.

There are a bunch of user-defined preferences kept in the ~/.local/share/org.sailfishos/browser/.mozilla/prefs.js file which I want to take a look at first.

$ cat prefs.js 
// Mozilla User Preferences

// DO NOT EDIT THIS FILE.
//
// If you make changes to this file while the application is running,
// the changes will be overwritten when the application exits.
//
// To change a preference value, you can either:
// - modify it via the UI (e.g. via about:config in the browser); or
// - set it within a user.js file in your profile.

user_pref("app.update.lastUpdateTime.addon-background-update-timer", 1701505984);
user_pref("app.update.lastUpdateTime.region-update-timer", 1701624416);
user_pref("app.update.lastUpdateTime.search-engine-update-timer", 1701624536);
user_pref("app.update.lastUpdateTime.services-settings-poll-changes", 1701505864);
user_pref("app.update.lastUpdateTime.user-agent-updates-timer", 1701419251);
user_pref("app.update.lastUpdateTime.xpi-signature-verification", 1701505588);
[...]

I want to remove all the preferences in the embedlite group, as well as the keyword.enabled preference, since these are the ones I made changes to:

user_pref("embedlite.compositor.external_gl_context", true);
user_pref("embedlite.inputItemSize", "50");
user_pref("embedlite.zoomMargin", "20");
[...]
user_pref("keyword.enabled", true);

All four are now purged from the file. Time to fire up the browser to see what happens. My plan here is to test a couple of things:

1. Whether changing the values sticks across execution runs.
2. Whether changing the values affects behaviour.

Both of these should be the case. Let's see.

The results, I would say, are interesting. First of all the embedlite static preferences. I'm testing with the embedlite.azpc.handle.longtap preference because this has a very clear and testable impact on the user experience. When active, pressing and holding on an image will bring up a menu that allows you to save the image to disk. When disabled, pressing and holding on an image should have no effect.
 
From testing, the preference is working as expected. Long tap on an image works when the preference is set to true, but does nothing when it's set to false. That's the most important part of the test and gets a confirmatory tick: test passed.

Checking between execution runs I can see the preference is also sticky. Closing and reopening the browser doesn't affect the setting; on reopening it's set to false if it was disabled before closing and set to true otherwise: test passed.

I also checked that the preference value gets written to the prefs.js file and it does when it's set to false. When it's set to true it's removed because that's the default value. Great!

The static preferences are looking good. But one of the changes we made used a normal preference rather than a static preference and that was the keyword.enabled flag. This controls whether search is allowed in the address bar.

For this test I set the preference to true, typed some text in the address bar and checked that search was performed using my chosen search provider. I then set the preference to false and checked that the error page is displayed. All of this worked as expected: test passed.
 
Finally, does the keyword.enabled value stick across runs? I'm surprised to discover that it doesn't; every time the browser is restarted it's set back to true. I can also see that it's always set to true in the prefs.js file: test failed.

This last failure surprises me and it makes me wonder whether there's some code somewhere that's forcing it to be true? A quick grep of the code throws the following up:

$ grep -rIn "keyword.enabled" * -B1
apps/core/declarativewebutils.cpp-138-    // Enable internet search
apps/core/declarativewebutils.cpp:139:
    webEngineSettings->setPreference(QString("keyword.enabled"), QVariant(true));

I guess that explains it then: every time the browser is started it forcefully sets this value to true. I don't really know why this is done, it could be that the original value was false and it had to be enabled somewhere, or that it was to protect users from accidentally messing up their own settings. The commit message for the change from over a decade ago doesn't really help either:

$ git log -1 eeea59ae2
commit eeea59ae23466204efbbafca417f3f7542deff66
Author: Dmitry Rozhkov <dmitry.rozhkov@jollamobile.com>
Date:   Thu Aug 22 11:13:41 2013 +0300

    [sailfish-browser] Enable internet search. Fixes JB#5503, JB#5504, JB#5505,
        JB#1200

Nevertheless it does explain the behaviour we're seeing now, so that means everything is working as it should after all.

I've pushed my changes to the remote repository and so have also closed the issue.

This evening my plan is to move on to issue #1030 to try to fix printing. You might think that printing on a mobile browser is a bit redundant, but in sailfish-browser we use it to support export to PDF. I broke the printing functionality quite badly by essentially removing chunks of it to ensure the code would build. It'll be good to go back and set that straight.

[...]

So it's now the evening and time to start looking into the printing situation. The easiest way to check this is to try out the "Save web page as PDF" feature of the browser. But when I press the menu option to try this out nothing happens and I see the following output at the console:

CONSOLE message:
[JavaScript Error: "aSerializable.url is undefined"
    {file: "resource://gre/modules/DownloadCore.jsm" line: 1496}]
DownloadSource.fromSerializable@resource://gre/modules/DownloadCore.jsm:1496:5
Download.fromSerializable@resource://gre/modules/DownloadCore.jsm:1282:38
D_createDownload@resource://gre/modules/Downloads.jsm:108:39
observe/<@file:///usr/lib64/mozembedlite/components/EmbedliteDownloadManager.js:257:48

That immediately gives us a few leads to follow up. Looking inside the EmbedliteDownloadManager.js file, which is part of the embedlite-components repository, we can see the following code which relates to this:

          case "saveAsPdf":
            if (Services.ww.activeWindow) {
              (async function() {
                let list = await Downloads.getList(Downloads.ALL);
                let download = await Downloads.createDownload({
                  source: Services.ww.activeWindow,
                  target: data.to,
                  saver: "pdf",
                  contentType: "application/pdf"
                });
                download["saveAsPdf"] = true;
                download.start();
                list.add(download);
              })().then(null, Cu.reportError);
            } else {
              Logger.warn("No active window to print to pdf")
            }
            break;

The call that's causing the error is this one:

                let download = await Downloads.createDownload({
                  source: Services.ww.activeWindow,
                  target: data.to,
                  saver: "pdf",
                  contentType: "application/pdf"
                });

That takes us here:

  createDownload: function D_createDownload(aProperties) {
    try {
      return Promise.resolve(Download.fromSerializable(aProperties));
    } catch (ex) {
      return Promise.reject(ex);
    }
  },

Which takes us here:

Download.fromSerializable = function(aSerializable) {
  let download = new Download();
  if (aSerializable.source instanceof DownloadSource) {
    download.source = aSerializable.source;
  } else {
    download.source = DownloadSource.fromSerializable(aSerializable.source);
  }

Which ends up here:

DownloadSource.fromSerializable = function(aSerializable) {
  let source = new DownloadSource();
  if (isString(aSerializable)) {
    // Convert String objects to primitive strings at this point.
    source.url = aSerializable.toString();
  } else if (aSerializable instanceof Ci.nsIURI) {
    source.url = aSerializable.spec;
  } else {
    // Convert String objects to primitive strings at this point.
    source.url = aSerializable.url.toString();

Leaving us with the error "aSerializable.url is undefined". Unravelling all of this we can see that what went in as aProperties ended up as aSerializable. The properties set in aProperties are source, target, saver and contentType. Definitely no url.

In practice though, what it really wants is the source parameter sent in at the start to be either an instance of DownloadSource or a serialised version of it. We're setting it to Services.ww.activeWindow so it's probably also worth working out what Services.ww.activeWindow is returning.

The Services.ww reference appears to be to an implementation of nsIWindowWatcher. Here's the property in question:

  /**
      Retrieves the active window from the focus manager.
  */
  readonly attribute mozIDOMWindowProxy activeWindow;

I did have to make changes to some of the window referencing code, so it's possible I broke something. But it's also possible that the DownloadSource interface has changed. To check the latter I'm going to compare it against the equivalent ESR 78 code.

And there is a difference. It's right down at the DownloadSource.fromSerializable() level where there used to be a check for whether source was an instance of Ci.nsIDOMWindow and which now does something slightly different.

Here's the commit that made the change:

$ git log -1 -S "Ci.nsIDOMWindow" -- toolkit/components/downloads/DownloadCore.jsm
commit 258369999a13027375f4fa496a7d4b23fb1eddfa
Author: Jonathan Kew <jkew@mozilla.com>
Date:   Mon Jul 20 16:04:35 2020 +0000

    Bug 1641805 - Remove support for creating a DownloadSource from an
    nsIDOMWindow. r=mak
    
    Differential Revision: https://phabricator.services.mozilla.com/D84137

This is actually quite a significant change and the issue that describes it is even more alarming.
 
As noted further down in the issue description, the changes that introduced this code may be a useful template.

It's been a long day for me today already, so I'm going to call it a night here. This clearly needs some more investigation which I'll need to pick up tomorrow.

I note with some trepidation that it's Day 99 today. That means tomorrow is the next order of magnitude of time spent working of this, from a base 10 perspective at least (my preference would be to work to base 2, which would make 128 the next big event). That may feel like an awfully long time and I know there are many Sailfish OS users who would just like this to be done and ready. All I can say is that every step brings us closer to a proper release and the quality really does improve with each one. I'm confident we'll get there.

If you'd like to catch up on all the diary entries, they're all available on my Gecko-dev Diary page.

When I woke up this morning my build had failed. The changes I made to the preferences yesterday introduced an error into the C++ code:

224:55.80 mobile/sailfishos
224:57.07 ${PROJECT}/gecko-dev/mobile/sailfishos/embedthread/
          EmbedLiteCompositorBridgeParent.cpp:19:10: fatal error:
          mozilla/StaticPrefs_embedlite.h: No such file or directory
224:57.07  #include "mozilla/StaticPrefs_embedlite.h" // for StaticPrefs::embedlite_*()
224:57.07           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
224:57.07 compilation terminated.

So the StaticPrefs_embedlite.h header file, which I'd assumed would be generated by the build process, can't be found. Maybe it wasn't generated after all?

Although my assumptions were wrong, they were only partially wrong, because there is a file called StaticPrefList_embedlite.h that's been generated and it does contain the preferences I added to StaticPrefList.yaml. Here's a snippet:

// This file was generated by generate_static_pref_list.py 
// from modules/libpref/init/StaticPrefList.yaml. DO NOT EDIT.

ALWAYS_PREF(
  "embedlite.azpc.handle.viewport",
   embedlite_azpc_handle_viewport,
   embedlite_azpc_handle_viewport,
  bool, true
)
[...]

And there is also a file called StaticPrefs_embedlite.h which also contains what we'd expect based on what's there for the other groups:

// This file was generated by generate_static_pref_list.py from
// modules/libpref/init/StaticPrefList.yaml. DO NOT EDIT.
// Include it to gain access to StaticPrefs::embedlite_*.

#ifndef mozilla_StaticPrefs_embedlite_h
#define mozilla_StaticPrefs_embedlite_h

#include "mozilla/StaticPrefListBegin.h"
#include "mozilla/StaticPrefList_embedlite.h"
#include "mozilla/StaticPrefListEnd.h"

#endif  // mozilla_StaticPrefs_embedlite_h

So we're not missing the StaticPrefs_embedlite.h file, it's being correctly generated by generate_static_pref_list.py, it's just not being picked up by our #include directive.

I guess my task for today will be to figure out why not.

Doing a quick grep on the source tree shows that other examples of these headers are appearing in more of the generated output than the embedlite version is appearing in. For example in obj-build-mer-qt-xr/modules/libpref/backend.mk and in obj-build-mer-qt-xr/generated-sources.json. That must be a warning sign.

A bit more searching turns up the fact that the various groups are all listed in the pref_groups variable of the gecko-dev/modules/libpref/moz.build file. This turns out to be confirmed by the documentation at the head of the StaticPrefList.yaml file:

# Please follow the existing prefs naming convention when considering adding a
# new pref, and don't create a new pref group unless it's appropriate and there
# are likely to be multiple prefs within that group. (If you do, you'll need to
# update the `pref_groups` variable in modules/libpref/moz.build.)

It feels like there's a moral to be had about reading documentation here, but I can't quite put my finger on it. The good news is that there's nothing else in the documentation about other places the group needs to be added, so with any luck making this one change should be enough to do the trick.

I've added embedlite to the list of groups in the moz.build file.

pref_groups = [
    "accessibility",
    "alerts",
[...]
    "editor",
    "embedlite",
    "extensions",
    "findbar",
[...]

I'll need to do another full rebuild to see whether this has fixed anything.

Before I do that and call it a day, I want to try to pick off another of the JavaScript errors that are being output on startup. We have this issue #1041 about the EmbedLiteChromeManager component not being able to find a file:

JavaScript error: file:///usr/lib64/mozembedlite/components/
    EmbedLiteChromeManager.js, line 213: NS_ERROR_FILE_NOT_FOUND: 

Maybe, just maybe, this will be an easy one to track down and fix.

This file is part of the embedlite-components repository. Line 213, the place that's generating the error, doesn't look too auspicious:

      AboutCertViewerHandler.init();

Here's that line with a bit more context:

  observe(aSubject, aTopic, aData) {
    let self = this;
    switch (aTopic) {
[...]
    case "browser-delayed-startup-finished":
      AboutCertViewerHandler.init();
      Services.obs.removeObserver(this, "browser-delayed-startup-finished");
      break;
[...]

This AboutCertViewerHandler is defined lazily, so it could well be that the problem is a missing AboutCertViewerHandler.jsm source file:

XPCOMUtils.defineLazyModuleGetters(this, {
  AboutCertViewerHandler: "resource://gre/modules/AboutCertViewerHandler.jsm",
  ContentLinkHandler: "chrome://embedlite/content/ContentLinkHandler.jsm",
  Feeds: "chrome://embedlite/content/Feeds.jsm"
});

In that list, both ContentLinkHandler.jsm and Feeds.jsm can be found in the embedlite-components code.

On the other hand, there is a file called AboutCertViewerHandler.jsm in the ESR 78 gecko source that's been removed from the ESR 91 source. Well, not removed, but renamed:

$ git diff 92c10e5f503ad8e~ 92c10e5f503ad8e
[...]
diff --git a/toolkit/components/certviewer/AboutCertViewerHandler.jsm
           b/toolkit/components/certviewer/AboutCertViewerParent.jsm
similarity index 50%
rename from toolkit/components/certviewer/AboutCertViewerHandler.jsm
rename to toolkit/components/certviewer/AboutCertViewerParent.jsm
index 390fa3113836..16cedeafee9d 100644
--- a/toolkit/components/certviewer/AboutCertViewerHandler.jsm
+++ b/toolkit/components/certviewer/AboutCertViewerParent.jsm

What was AboutCertViewerHandler.jsm is apparently now AboutCertViewerParent.jsm. For reference, here's the commit message, although I'm not sure this on its own is super-enlightening.

$ git log -1 92c10e5f503ad8e16424e554f8fb6393ff77152f
commit 92c10e5f503ad8e16424e554f8fb6393ff77152f
Author: Neil Deakin <neil@mozilla.com>
Date:   Thu Jun 25 01:13:05 2020 +0000

    Bug 1646197, convert about:certificate to JSWindowActor instead of old RPM,
    r=johannh
    
    Differential Revision: https://phabricator.services.mozilla.com/D80017

The actual changeset D80017 associated with this is more helpful though. It makes clear that both the init() and uninit() have both been removed and don't need to be called any more. So we can remove them from our embedlite-components code too. We do need to check whether we're using this certificate code anywhere though.

A swift grep of the code shows it is being used, but only in the existing gecko code (none of the EmbedLite additions) and only in EmbedLiteChromeManager.js. In the latter it's only being used for init() and uninit(), so hopefully that means there are no other changes to make apart from removing these references.

The truth of whether it's needed in some other way may only become apparent later if there's some other broken functionality, but my gut tells me this won't happen.

These two calls that we've removed seem to be the only reason why we're interested in the "browser-delayed-startup-finished" and "xpcom-shutdown" event messages as well, so I've also removed the observers related to these:

$ git diff
diff --git a/jscomps/EmbedLiteChromeManager.js b/jscomps/EmbedLiteChromeManager.js
index da6151b..60e2aef 100644
--- a/jscomps/EmbedLiteChromeManager.js
+++ b/jscomps/EmbedLiteChromeManager.js
@@ -17,7 +17,6 @@ const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");
 const { NetErrorHelper } = ChromeUtils.import("chrome://embedlite/content/NetErrorHelper.jsm")
 
 XPCOMUtils.defineLazyModuleGetters(this, {
-  AboutCertViewerHandler: "resource://gre/modules/AboutCertViewerHandler.jsm",
   ContentLinkHandler: "chrome://embedlite/content/ContentLinkHandler.jsm",
   Feeds: "chrome://embedlite/content/Feeds.jsm"
 });
@@ -137,8 +136,6 @@ EmbedLiteChromeManager.prototype = {
     Services.obs.addObserver(this, "embed-network-link-status", true)
     Services.obs.addObserver(this, "domwindowclosed", true);
     Services.obs.addObserver(this, "keyword-uri-fixup", true);
-    Services.obs.addObserver(this, "browser-delayed-startup-finished");
-    Services.obs.addObserver(this, "xpcom-shutdown");
   },
 
   onWindowOpen(aWindow) {
@@ -209,13 +206,6 @@ EmbedLiteChromeManager.prototype = {
       Services.io.offline = network.offline;
       Services.obs.notifyObservers(null, "network:link-status-changed",
                                    network.offline ? "down" : "up");
-    case "browser-delayed-startup-finished":
-      AboutCertViewerHandler.init();
-      Services.obs.removeObserver(this, "browser-delayed-startup-finished");
-      break;
-    case "xpcom-shutdown":
-      AboutCertViewerHandler.uninit();
-      break;
     default:
       Logger.debug("EmbedLiteChromeManager subject", aSubject, "topic:", aTopic);
     }

Removing code from our downstream changes always fills me with a warm and fuzzy feeling. Less code to maintain in future.

It's worth noting that this change is essentially reversing commit b1510a7a that added the initialisation and deinitialisation calls in to the EmbedLite code.

The commit message doesn't provide a huge amount of context, but it doesn't appear to be a smaller part of a larger changeset, so hopefully this is an atomic change without other consequences elsewhere.

$ git log -1 b1510a7a
commit b1510a7a5771906e44b4247bd75271c1bb5c54f6 (upstream/jb56094, andrew-korolev-omp/jb56094)
Author: Andrew den Exter <andrew.den.exter@jolla.com>
Date:   Mon Nov 15 06:46:16 2021 +0000

    [embedlite-components] Initialize the certificate viewer handler.
    JB#56094 OMP#JOLLA-492

Okay, I've made and committed these changes; I've built and installed the resulting embedlite-components packages. The errors seen in the console output are now gone and I don't see any new issues arising.

I think that's enough for today. I'm going to hit the main gecko-dev build off and see what happens with it tomorrow.

If you'd like to catch up on all the diary entries, they're all available on my Gecko-dev Diary page.

It's another freezing cold day today. It certainly feels like winter now. All the better to be snug inside and doing some coding. As I write this it's already the evening; over the last few days I've been working on fixing address bar search. That's now working (up to a point, because not all of the available search providers' sites quite work correctly yet), but while investigating it I hit the issue of the VarCache having been removed. These need to be converted into statis preferences, as detailed in task #1027.

I've been psyching myself up for tackling these preferences. I'm not sure, but I suspect this task is going to require a bit of investigation and thought. Here's a bit of code that needs fixing. I've helpfully left some comments there for myself to pick up, which offers me some solid ground to start from.

static void ReadAZPCPrefs()
{
  // TODO: Switch these to use static prefs
  // See https://firefox-source-docs.mozilla.org/modules/libpref/index.html#static-prefs
  // Example: https://phabricator.services.mozilla.com/D40340

  // Init default azpc notifications behavior
  //Preferences::AddBoolVarCache(&sHandleDefaultAZPC.viewport,
    "embedlite.azpc.handle.viewport", true);
  //Preferences::AddBoolVarCache(&sHandleDefaultAZPC.singleTap,
    "embedlite.azpc.handle.singletap", false);
  //Preferences::AddBoolVarCache(&sHandleDefaultAZPC.longTap,
    "embedlite.azpc.handle.longtap", false);
  //Preferences::AddBoolVarCache(&sHandleDefaultAZPC.scroll,
    "embedlite.azpc.handle.scroll", true);

  //Preferences::AddBoolVarCache(&sPostAZPCAsJson.viewport,
    "embedlite.azpc.json.viewport", true);
  //Preferences::AddBoolVarCache(&sPostAZPCAsJson.singleTap,
    "embedlite.azpc.json.singletap", true);
  //Preferences::AddBoolVarCache(&sPostAZPCAsJson.doubleTap,
    "embedlite.azpc.json.doubletap", false);
  //Preferences::AddBoolVarCache(&sPostAZPCAsJson.longTap,
    "embedlite.azpc.json.longtap", true);
  //Preferences::AddBoolVarCache(&sPostAZPCAsJson.scroll,
    "embedlite.azpc.json.scroll", false);

  //Preferences::AddBoolVarCache(&sAllowKeyWordURL, "keyword.enabled",
    sAllowKeyWordURL);

  sHandleDefaultAZPC.viewport = true; // "embedlite.azpc.handle.viewport"
  sHandleDefaultAZPC.singleTap = false; // "embedlite.azpc.handle.singletap"
  sHandleDefaultAZPC.longTap = false; // "embedlite.azpc.handle.longtap"
  sHandleDefaultAZPC.scroll = true; // "embedlite.azpc.handle.scroll"

  sPostAZPCAsJson.viewport = true; // "embedlite.azpc.json.viewport"
  sPostAZPCAsJson.singleTap = true; // "embedlite.azpc.json.singletap"
  sPostAZPCAsJson.doubleTap = false; // "embedlite.azpc.json.doubletap"
  sPostAZPCAsJson.longTap = true; // "embedlite.azpc.json.longtap"
  sPostAZPCAsJson.scroll = false; // "embedlite.azpc.json.scroll"

  sAllowKeyWordURL = sAllowKeyWordURL;
    // "keyword.enabled" (intentionally retained for clarity)
}

In this text I've suggested to convert these into static prefs. Like normal prefers static prefs are handled via libpref, but as I understand it they're more efficient because they're defined as variables which can be requested to mirror the preference value, but which can't be set within the code. Normal prefs use a hashtable lookup which makes them somewhat slower to access.

I think static prefs are going to work better in the majority of cases for what we need because many of them will actually be referenced quite regularly. The one case that doesn't fit this pattern, somewhat ironically given it's the one that sparked this journey, is keywords.enabled. That's only called on the few occassions when something is entered into to address bar.

But for the APZ preferences, I've created static preferences in StaticPrefList.yaml like this:

-   name: embedlite.azpc.handle.viewport
    type: bool
    value: true
    mirror: always

-   name: embedlite.azpc.handle.singletap
    type: bool
    value: false
    mirror: always
[...]

These will get pulled into the build process which will generate a header file "StaticPrefs_embedlite.h" which I've added to the top of the EmbedLiteViewChild.cpp which should then allows me to write code like this:

  if (StaticPrefs::embedlite_azpc_handle_viewport()) {
    mHelper->UpdateFrame(aRequest);
  }

These will be super-efficient. For the keyword search preference I've done something slightly different, like this:

  if (Preferences::GetBool("keyword.enabled", true)) {
    flags |= nsIWebNavigation::LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP;
    flags |= nsIWebNavigation::LOAD_FLAGS_FIXUP_SCHEME_TYPOS;
  }

One final thing to do with these preferences is note that they also appear in embedding.js. The libpref documentation has this to say about this:
 
This makes me wonder whether embedding.js is more like all.js, or more like firefox.js. My instinct says the latter, but even then this explanation implies that there are only some situations in which it might make sense to include them in an app-specific file. An obvious case might be when different apps need different default values.

I think there's no need to store them in embedding.js. Removing them from this won't prevent the user changing them for storage within their profile, so I don't see any downsides. It just removes one more place to have to maintain the details. So I've also deleted the preferences from embedding.js.

Apart from these preferences in EmbedLiteViewChild.cpp there are a few other preferences that need dealing with as a consequence of my earlier destruction too:

$ grep -rIn "AddBoolVarCache" *
embedding/embedlite/utils/BrowserChildHelper.cpp:82:
  //Preferences::AddBoolVarCache(&sPostAZPCAsJsonViewport, "embedlite.azpc.json.viewport", false);
embedding/embedlite/embedthread/EmbedLiteCompositorBridgeParent.cpp:68:
  //Preferences::AddBoolVarCache(&mUseExternalGLContext,
embedding/embedlite/embedshared/nsWindow.cpp:63:
  //Preferences::AddBoolVarCache(&sUseExternalGLContext,
embedding/embedlite/embedshared/nsWindow.cpp:65:
  //Preferences::AddBoolVarCache(&sRequestGLContextEarly,

I've gone through each of these and also turned them into static prefs. With this process complete, the next step is to build the code and see whether this produces good results. The build process will need to generate the header files from StaticPrefList.yaml for this to work, otherwise the C++ code won't compile. So doing the build is going to be essential and likely it'll need a full rebuild as well.

[...]

Except the build has failed:

 7:15.53 ${PROJECT}/gecko-dev/modules/libpref/init/StaticPrefList.yaml: error:
 7:15.53   `embedlite.azpc.handle.viewport` pref must come before
     `zoom.minPercent` pref

It turns out the preferences have to be in alphabetical order based on the first word in the name. Interesting! So I've reordered the preferences and set the build going again.

So that has to be it for today. Tomorrow morning, once the build has hopefully completed, we can see whether it works or not.

Before signing off I want to note down one other point that I think is interesting. It's something that I've wondered for a long time but never got around to checking. If we look in the embedding.js file we see lots of lines like this:

pref("dom.w3c_touch_events.enabled", 1);
pref("dom.w3c_touch_events.legacy_apis.enabled", true);
[...]

I've always been curious to know what these lines actually do. In my exploration around the code today I noticed that this function is defined in prefcalls.js. So mystery solved, here's what this is doing:

function pref(prefName, value) {
  try {
    var prefBranch = getPrefBranch();

    if (typeof value == "string") {
      if (gIsUTF8) {
        prefBranch.setStringPref(prefName, value);
        return;
      }
      prefBranch.setCharPref(prefName, value);
    } else if (typeof value == "number") {
      prefBranch.setIntPref(prefName, value);
    } else if (typeof value == "boolean") {
      prefBranch.setBoolPref(prefName, value);
    }
  } catch (e) {
    displayError("pref", e);
  }
}

That all rather makes sense. Okay, that really is it for today.

If you'd like to catch up on all the diary entries, they're all available on my Gecko-dev Diary page.

This morning I'm on the train headed for work. It's a very cold and frosty morning — it could almost be Finland — and my development environment isn't ideal for SSH-ing into my phone, but it's not busy so I've experienced worse!
 
We finished up yesterday with the realisation that search provider installation was being blocked by changes that prevented the OpenSearch configuration being loaded from disk. The download scheme was being checked by a regex that only allowed "https" to be used.

This morning I overrode the regex to allow the "file" scheme to be used. Here's the resulting output.

Frame script: embedhelper.js loaded
SEARCH: getFixupURIInfo() fixupFlags: 8
SEARCH: engine num: 0
SEARCH: addOpenSearchEngine():
        file:///usr/lib64/mozembedlite/chrome/embedlite/content/bing.xml
SEARCH: stack start
addOpenSearchEngine@resource://gre/modules/SearchService.jsm:1869:10
observe@file:///usr/lib64/mozembedlite/components/EmbedLiteSearchEngine.js:56:29
SEARCH: stack end
SEARCH: _install: uri:
        file:///usr/lib64/mozembedlite/chrome/embedlite/content/bing.xml
SEARCH: _install: Downloading engine from:
        file:///usr/lib64/mozembedlite/chrome/embedlite/content/bing.xml

This shows that the code is getting past the check, but not much else. And crucially, the search providers still aren't being installed correctly. We're also getting the following errors a little later:

[JavaScript Error: "TypeError: this._engineToUpdate is null"
    {file: "resource://gre/modules/OpenSearchEngine.jsm" line: 138}]
_onLoad@resource://gre/modules/OpenSearchEngine.jsm:138:5
onStopRequest@resource://gre/modules/SearchUtils.jsm:92:10

This error is actually coming from one of the debug output lines I added myself:

    dump("SEARCH: _onLoad: Downloaded engine from:" + this._engineToUpdate.name + "\n");

I've removed the reference to this._engineToUpdate from this line and now it passes correctly.

But not only that, now the search functionality is back too!

This is great, but I'm a little nervous about the changes: there were some big changes made to remove the "file" scheme and I'll need to look carefully into why this was and whether it's safe to work around it. The changes I've made to allow it are minimal and I'd feared I'd have to restore far more of the changes in changeset D104010.

The good news is I can stop debugging my phone while I travel. Looking like someone from Mr Robot on the train isn't something I'm totally comfortable with! I'll return to these changes and put together a proper fix this evening.
 
[...]

It's evening now and time to turn the hacky changes I made earlier into a proper fix. Before I do that I want to reflect on a discussion with Ville Nummela on Mastodon. Ville — vige — is one of my ex-colleagues from Jolla and, you won't be surprised to hear, someone I have a lot of time and respect for. Ville pointed out that OpenSearch is still supported on Firefox, so if it's causing trouble like this for our version for the sailfish-browser, shouldn't it cause similar issues on Firefox?
 
This is such a good question and something I should have thought about and investigated over the last few days. It turns out there is a good answer and that, as you might expect, Firefox isn't downloading the OpenSearch provider XML files every time it starts up.

It took me a while to dig through the code, but the answer is in the SearchSettings.jsm file from upstream gecko. This provides two crucial methods: SearchSettings._write() and SearchSettings.get(). The first of these writes the OpenSearch providers out to disk, but not in their original XML format, but rather in a compressed JSON format. The second of these reads them back in again from disk.

The reason why it's _write() and not write() is that it's not supposed to be called directly. Instead it's called automatically via _delayedWrite() when something changes:

  // nsIObserver
  observe(engine, topic, verb) {
    switch (topic) {
      case SearchUtils.TOPIC_ENGINE_MODIFIED:
        switch (verb) {
          case SearchUtils.MODIFIED_TYPE.ADDED:
          case SearchUtils.MODIFIED_TYPE.CHANGED:
          case SearchUtils.MODIFIED_TYPE.REMOVED:
            this._delayedWrite();
            break;
        }
        break;
[...]

Here are the first few lines including docstrings of each of the methods for reference. First for writing out to disk:

  /**
   * Writes the settings to disk (no delay).
   */
  async _write() {
    if (this._batchTask) {
      this._batchTask.disarm();
    }

    let settings = {};

    // Allows us to force a settings refresh should the settings format change.
    settings.version = SearchUtils.SETTINGS_VERSION;
    settings.engines = [...this._searchService._engines.values()];
    settings.metaData = this._metaData;
[...]

Second for reading in from disk. This is the part that replaces the code that we had to patch to allow loading of the XML file:

  /**
   * Reads the settings file.
   *
   * @param {string} origin
   *   If this parameter is "test", then the settings will not be written. As
   *   some tests manipulate the settings directly, we allow turning off writing to
   *   avoid writing stale settings data.
   * @returns {object}
   *   Returns the settings file data.
   */
  async get(origin = "") {
    let json;
    await this._ensurePendingWritesCompleted(origin);
    try {
      let settingsFilePath = PathUtils.join(
        await PathUtils.getProfileDir(),
        SETTINGS_FILENAME
      );
[...]

In both of these snippets of code we can see this important constant, which is the name of the file to save out or load in and which we can see initialised at the top of the same file:

const SETTINGS_FILENAME = "search.json.mozlz4";

If we look inside the ESR 91 settings folder we can see that this file is indeed being saved out:

 ls -l ~/.local/share/org.sailfishos/browser/.mozilla/search.json.mozlz4 
-rw-rw-r--    1 defaultu defaultu     17715 Dec  2 20:03 search.json.mozlz4

We can compare this to the XML files that sailfish-browser is saving out to be loaded at start up to reinitialise the OpenSearch providers:

$ ls -l ~/.local/share/org.sailfishos/browser/searchEngines/
total 16
-rw-rw-r--    1 defaultu defaultu      1949 Dec  2 19:08 duckduckgo.com.xml
-rw-rw-r--    1 defaultu defaultu       688 Dec  2 19:05 forum.sailfishos.org.xml
-rw-rw-r--    1 defaultu defaultu       549 Nov 26 14:48 github.com.xml
-rw-rw-r--    1 defaultu defaultu      3493 Dec  2 19:06 www.openstreetmap.org.xml

In conclusion, Firefox is using a different mechanism to save out and load in the search providers. The file is being saved out to our profile, but at no point are we loading it back in. Updating the code to load it in using the Firefox approach would be a nice fix: it would mean we wouldn't have to patch the XML loading code, it would mean less to maintain in future and it would also mean we're not saving out the same data to two different places.

This also helps answers my earlier question about why loading of these XML files was restricted to the https scheme. The good news is that the change I've made is probably fine, it just isn't being used by Firefox any more. But I've also created ticket #1048 suggesting that we update our code to move to the Firefox approach.

Now back to the changes we've already made. The fixes I added to allow loading using the file scheme as well as https are still just hacked onto my phone. With these added the output from the search installation at start up looks much cleaner:

SEARCH: engine num: 6
SEARCH: engine name: Bing
SEARCH: engine name: DuckDuckGo
SEARCH: engine name: GitHub
SEARCH: engine name: Yandex
SEARCH: engine name: Google
SEARCH: engine name: Yahoo
SEARCH: get defaultEngine: 
SEARCH: get defaultEngine: 
SEARCH: EmbedLiteSearchEngine setdefault received
SEARCH: EmbedLiteSearchEngine engine: DuckDuckGo

After removing all the debug output I'm left with just four simple changes. First in EmbedLiteViewChild.cpp we have this:

-static bool sAllowKeyWordURL = false;
+static bool sAllowKeyWordURL = true;

In OpenSearchEngine.jsm, mimicking the approach originally used to allow both "http" and "ftp" schemes, I've adjusted it to be both "http" and "file" schemes:

-    if (!/^https?$/i.test(loadURI.scheme)) {
+    if (!/^(?:https?|file)$/i.test(loadURI.scheme)) {

Both of these changes are in the gecko-dev repository, but there are also changes needed in the embedlite-components repository as well. In EmbedLiteSearchEngine.js:

-            Services.search.addEngine(data.uri, null, data.confirm).then(
+            Services.search.addOpenSearchEngine(data.uri, null).then(

And finally in PromptService.js:

+const { ComponentUtils } = ChromeUtils.import("resource://gre/modules/ComponentUtils.jsm");
[...]
-this.NSGetFactory = XPCOMUtils.generateNSGetFactory([PromptService, AuthPromptAdapterFactory]);
+this.NSGetFactory = ComponentUtils.generateNSGetFactory([PromptService, AuthPromptAdapterFactory]);

Apart from the first instance all of these changes are to the JavaScript. Since I've already compiled the single small C++ change I can, in effect, make all of these changes now without having to rebuild gecko.

Nevertheless to ensure everything has been changed correctly I'll leave the build running overnight and install it tomorrow morning.

One thing you may notice is that the sAllowKeyWordURL isn't actually how it's supposed to be for a proper fix. Rather than setting this statically to be true we should really be reading the value of the keyword.enabled preference. In fact there are a whole bunch of preferences like this that we need to fix.

Because of this it makes more sense to do them all together rather than finding a solution for just this particular case. So maybe fixing all of these preferences would make a sensible task to start tomorrow.

Step by step, slowly but surely, the browser functionality is improving where we're getting to the point that the browser is quite usable. But clearly there is still more to be done.

If you'd like to catch up on all the diary entries, they're all available on my Gecko-dev Diary page.

Unfortunately I don't have as much time today as I've had the last few days to work on Gecko, but hopefully there's still scope to make some meaningful progress.

I'm still trying to track down the address bar issue that's preventing Web search from working. In theory, when you enter a phrase that's clearly not a URL (or begins with a question mark) the browser should delegate the action to your chosen search provider (as configured in the Settings).

Over the last couple of days we got to the point where a number of issues have been fixed (so no overt error messages) but during initialisation the list of search engines is empty. It should be full of stuff, so today I want to try to find out why.

So, I've added a load of debug dumps to various important files related to the OpenSearch capability: SearchService.jsm, SearchUtils.jsm and OpenSearchEngine.js. These will generate debug output in the log (output to the console) so that we can see what's happening when and in what sequence, as well as what's not happening.

From this we can clearly see that addOpenSearchEngine() is being called for each of the stored search providers:

SEARCH: getFixupURIInfo() fixupFlags: 8
CONSOLE message:
[JavaScript Error: "NS_ERROR_FILE_NOT_FOUND: "{file: "file:///usr/lib64/
    mozembedlite/components/EmbedLiteChromeManager.js" line: 213}]
observe@file:///usr/lib64/mozembedlite/components/EmbedLiteChromeManager.js:213:7
SEARCH: engine num: 0
SEARCH: addOpenSearchEngine(): file:///usr/lib64/mozembedlite/chrome/
        embedlite/content/bing.xml
SEARCH: stack start
addOpenSearchEngine@resource://gre/modules/SearchService.jsm:1869:10
observe@file:///usr/lib64/mozembedlite/components/EmbedLiteSearchEngine.js:56:29
SEARCH: stack end
SEARCH: _install: uri: file:///usr/lib64/mozembedlite/chrome/
        embedlite/content/bing.xml
SEARCH: addOpenSearchEngine(): file:///home/defaultuser/.local/share/
        org.sailfishos/browser/searchEngines/duckduckgo.com.xml
SEARCH: stack start
addOpenSearchEngine@resource://gre/modules/SearchService.jsm:1869:10
observe@file:///usr/lib64/mozembedlite/components/EmbedLiteSearchEngine.js:56:29
SEARCH: stack end
SEARCH: _install: uri: file:///home/defaultuser/.local/share/
        org.sailfishos/browser/searchEngines/duckduckgo.com.xml
SEARCH: addOpenSearchEngine(): file:///home/defaultuser/.local/share/
        org.sailfishos/browser/searchEngines/github.com.xml
SEARCH: stack start
addOpenSearchEngine@resource://gre/modules/SearchService.jsm:1869:10
observe@file:///usr/lib64/mozembedlite/components/EmbedLiteSearchEngine.js:56:29
SEARCH: stack end
SEARCH: _install: uri: file:///home/defaultuser/.local/share/
        org.sailfishos/browser/searchEngines/github.com.xml
SEARCH: addOpenSearchEngine(): file:///usr/lib64/mozembedlite/chrome/
        embedlite/content/google.xml
SEARCH: stack start
addOpenSearchEngine@resource://gre/modules/SearchService.jsm:1869:10
observe@file:///usr/lib64/mozembedlite/components/EmbedLiteSearchEngine.js:56:29
SEARCH: stack end
SEARCH: _install: uri: file:///usr/lib64/mozembedlite/chrome/
        embedlite/content/google.xml
SEARCH: addOpenSearchEngine(): file:///usr/lib64/mozembedlite/chrome/
        embedlite/content/yahoo.xml
SEARCH: stack start
addOpenSearchEngine@resource://gre/modules/SearchService.jsm:1869:10
observe@file:///usr/lib64/mozembedlite/components/EmbedLiteSearchEngine.js:56:29
SEARCH: stack end
SEARCH: _install: uri: file:///usr/lib64/mozembedlite/chrome/
        embedlite/content/yahoo.xml
SEARCH: addOpenSearchEngine(): file:///usr/lib64/mozembedlite/chrome/
        embedlite/content/yandex.xml
SEARCH: stack start
addOpenSearchEngine@resource://gre/modules/SearchService.jsm:1869:10
observe@file:///usr/lib64/mozembedlite/components/EmbedLiteSearchEngine.js:56:29
SEARCH: stack end
SEARCH: _install: uri: file:///usr/lib64/mozembedlite/chrome/
        embedlite/content/yandex.xml
JavaScript error: chrome://embedlite/content/embedhelper.js, line 259:
    TypeError: sessionHistory is null
CONSOLE message:
[JavaScript Error: "TypeError: sessionHistory is null"
    {file: "chrome://embedlite/content/embedhelper.js" line: 259}]
receiveMessage@chrome://embedlite/content/embedhelper.js:259:29

We can also see from the stack where it's being called from, but that's not really the interesting part. The interesting part is what's not being output. Calls to addOpenSearchEngine() should almost immediately call the following:

      errCode = await new Promise(resolve => {
        engine._install(engineURL, errorCode => {
          resolve(errorCode);
        });
      });

I've added a bunch of dump outputs to the _install() method like this:

  _install(uri, callback) {
    dump("SEARCH: _install: uri: " + uri + "\n");
    let loadURI = uri instanceof Ci.nsIURI ? uri : SearchUtils.makeURI(uri);
    if (!loadURI) {
      throw Components.Exception(
        loadURI,
        "Must have URI when calling _install!",
        Cr.NS_ERROR_UNEXPECTED
      );  
    }   
    if (!/^https?$/i.test(loadURI.scheme)) {
      throw Components.Exception(
        "Invalid URI passed to SearchEngine constructor",
        Cr.NS_ERROR_INVALID_ARG
      );  
    }

    logConsole.debug("_install: Downloading engine from: ", loadURI.spec);
    dump("SEARCH: _install: Downloading engine from:" + loadURI.spec + "\n");

So the first dump() inside the _install() method is generating output, but the second isn't. Why not? I'll put some extra dump output inside those conditionals to see if the exceptions are firing.

SEARCH: addOpenSearchEngine(): file:///usr/lib64/mozembedlite/chrome/
        embedlite/content/bing.xml
SEARCH: stack start
addOpenSearchEngine@resource://gre/modules/SearchService.jsm:1869:10
observe@file:///usr/lib64/mozembedlite/components/EmbedLiteSearchEngine.js:56:29
SEARCH: stack end
SEARCH: _install: uri: file:///usr/lib64/mozembedlite/chrome/
        embedlite/content/bing.xml
SEARCH: _install: Invalid URI passed to SearchEngine constructor

And now things are beginning to take shape.

The file itself exists:

$ file /usr/lib64/mozembedlite/chrome/embedlite/content/bing.xml
/usr/lib64/mozembedlite/chrome/embedlite/content/bing.xml:
    exported SGML document, ASCII text, with very long lines (1815)

But this bit of code is causing the failure here:

if (!/^https?$/i.test(loadURI.scheme)) {
  dump("SEARCH: _install: Invalid URI passed to SearchEngine constructor\n");
  throw Components.Exception(
    "Invalid URI passed to SearchEngine constructor",
    Cr.NS_ERROR_INVALID_ARG
  );
}

The sailfish-browser code is passing in a URL that's a local file here, using the file scheme, but it's performing a regex test so that it will only accept an https scheme to download the data. The obvious question that springs to mind is what changed between ESR 78 and ESR 91 to be causing this?

In ESR 78 there is no OpenSearchEngine.jsm file because it was renamed from SearchEngine.jsm. The two are close enough to be able to compare though. Here's the equivalent piece of code from ESR 78:

    switch (optionsURI.scheme) {
      case "https":
      case "http":
      case "ftp":
      case "data":
      case "file":
      case "resource":
      case "chrome":
        uri = optionsURI;
        break;
      default:
        throw Components.Exception(
          "Invalid URI passed to SearchEngine constructor",
          Cr.NS_ERROR_INVALID_ARG
        );
    }

That's quite a difference. Let's find out why this was changed.

Checking using git blame shows this was the most recent change to this line:

$ git log -1 3760df94f0b94
commit 3760df94f0b94d8aab5ed10c6177e8fad6345cc3
Author: Valentin Gosu <valentin.gosu@gmail.com>
Date:   Wed Apr 7 10:20:58 2021 +0000

    Bug 1692018 - Remove support for installing OpenSearch engines via ftp
    r=Standard8
    
    Differential Revision: https://phabricator.services.mozilla.com/D107790

With the actual change looking like this:

--- a/toolkit/components/search/OpenSearchEngine.jsm
+++ b/toolkit/components/search/OpenSearchEngine.jsm
@@ -91,7 +91,7 @@ class OpenSearchEngine extends SearchEngine {
         Cr.NS_ERROR_UNEXPECTED
       );
     }
-    if (!/^(?:https?|ftp)$/i.test(loadURI.scheme)) {
+    if (!/^https?$/i.test(loadURI.scheme)) {
       throw Components.Exception(
         "Invalid URI passed to SearchEngine constructor",
         Cr.NS_ERROR_INVALID_ARG

We'll need to go back a bit further than this. Here's the change that happened before:

$ git log -1 269a6fbae9928
commit 269a6fbae9928d4fef5ca4cc9ee2b112b2772191
Author: Mark Banner <standard8@mozilla.com>
Date:   Wed Feb 10 18:12:08 2021 +0000

    Bug 1690750 - Simplify OpenSearchEngine to only allow loading engines from
                  protocols where users can load them from. r=mak
    
    The urls where an OpenSearch engine can be loaded from are already limited
    in LinkHandlerChild. This is cleaning up and simplifying what the
    OpenSearchEngine allows, and as a result allows the load path handling to be
    greatly simplified.
    
    The test changes are due to no longer allowing chrome or file protocols. For
    future, we probably want to move away from OpenSearch for most of these, but
    the changes will make it easier to find the places to update.
    
    Differential Revision: https://phabricator.services.mozilla.com/D104010

This is a much more significant change, which includes the removal of the file scheme.

This is all useful stuff, even if it's not yet a solution to the problem. I'm going to have to leave it here for today, but will return to this tomorrow. The next thing will be to try letting the file protocol through the regex to see what happens. I'm not expecting that to be enough, and if not, we'll have to dig into the whole of the D104010 diff to find out what's changed and whether we'll need to revert it, or find some other solution.

The underlying issue here seems to be the phasing out of OpenSearch as a way of managing search providers. So we should probably look seriously at the Web Extension alternative as well. But that's for tomorrow.

If you'd like to catch up on all the diary entries, they're all available on my Gecko-dev Diary page.