flypig.co.uk

Over the last couple of days we were looking at an error coming from the URLQueryStrippingListService JavaScript code. Although I committed some sort of "fix", it was a bit unsatisfactory because the fix was essentially to suppress the error. The error itself was harmless, but maybe it was masking some more serious issue?

This morning I committed the changes and recorded the details on the issue. Today I want to move on to something else. This is another JavaScript issue with error output showing in the console:

JavaScript Error: "TypeError: Services.search.addEngine is not a function"
    {file: "file:///usr/lib64/mozembedlite/components/EmbedLiteSearchEngine.js" line: 53}

This looks like it should be an easy one, but you never know. You really never know.

The first thing to note is that this error is coming from EmbedLiteSearchEngine.js which isn't part of the gecko-dev source tree, but rather comes from embedlite-components. Here's what it looks like:

      case "embedui:search": {
        var data = JSON.parse(aData);
        switch (data.msg) {
          case "loadxml": {
            Services.search.addEngine(data.uri, null, data.confirm).then(
              engine => {
                var message = {
                  "msg": "search-engine-added",
                  "engine": (engine && engine.name) || "",
                  "errorCode": 0,
                }
                Services.obs.notifyObservers(null, "embed:search",
                  JSON.stringify(message));
              },
              errorCode => {
                // For failure conditions see nsISearchService.idl
                var message = {
                  "msg": "search-engine-added",
                  "engine": "",
                  "errorCode": errorCode
                }
                Services.obs.notifyObservers(null, "embed:search",
                  JSON.stringify(message));
              }
            );
            break;
          }

It's the Services.search.addEngine() line that's causing the error. It's not complaining about search existing, so this would imply that Services.search exists but doesn't have a method called addEngine().

For the curious, this code is used to add "new" search engines to the browser. Some search provider sites (e.g. Wikipedia or Qwant) supply a small piece of XML code conforming to the OpenSearch standard. This explains to the browser the REST API that can be used to access the search capability. For example, it details what URL to use, what parameter to call the search term and how to get suggestions.

The Sailfish Browser identifies these OpenSearch hints when you visit a page that provides one and adds them to the list of available search engines in the Settings. This particular bit of code asks for the search provider to be installed using this OpenSearch XML file.

But as we've seen, in ESR 91 this no longer exists. To find out why not we have to check the Services.search module that's supposed to be offering the functionality.

You'll recall from a couple of days back that there was something similar happening with Services.appinfo. It's quite possible we have the same thing happening again.

The first thing to do is find out where this search module is. A quick grep of the code shows that, at least in a standard gecko build, it's defined in gecko-dev/toolkit/components/search/components.conf like this:

    {
        'js_name': 'search',
        'cid': '{7319788a-fe93-4db3-9f39-818cf08f4256}',
        'contract_ids': ['@mozilla.org/browser/search-service;1'],
        'interfaces': ['nsISearchService'],
        'jsm': 'resource://gre/modules/SearchService.jsm',
        'constructor': 'SearchService',
        'processes': ProcessSelector.MAIN_PROCESS_ONLY,
    },

I can't find any evidence that we override this for sailfish-browser, so we should take a look at the SearchService.jsm file referenced in the components configuration.

In the ESR 78 version of the file we can see the method that we need:

  async addEngine(engineURL, iconURL, confirm, extensionID) {
    SearchUtils.log('addEngine: Adding "' + engineURL + '".');
    await this.init();
    let errCode;
    try {
      var engine = new SearchEngine({
        uri: engineURL,
        isBuiltin: false,
      });
[...]

The method doesn't exist in ESR 91, which is likely why the error is appearing. There is however this method which looks like it should be doing something similar:

  async addOpenSearchEngine(engineURL, iconURL) {
    logConsole.debug("addEngine: Adding", engineURL);
    await this.init();
    let errCode;
    try {
      var engine = new OpenSearchEngine();

Before switching over to it, I'm going to check the history to check that this really is what we want. I also need to figure out what to do with the confirm parameter that is no longer passed in.

Here's the git blame from ESR 78:

$ git blame toolkit/components/search/SearchService.jsm -L 2707
Blaming lines:  29% (1138/3844), done.
94bb650cd4eea toolkit/components/search/SearchService.jsm
    (Andreea Pavel            2020-04-22 22:13:03 +0300 2707)
    async addEngine(engineURL, iconURL, confirm, extensionID) {
4a06c925ac8ae toolkit/components/search/SearchService.jsm
    (Victor Porof             2019-07-05 11:14:05 +0200 2708)
    SearchUtils.log('addEngine: Adding "' + engineURL + '".');
caceac87e1c78 toolkit/components/search/SearchService.jsm
    (Dale Harvey              2020-02-04 22:22:43 +0000 2709)
    await this.init();
481ae95c00e75 toolkit/components/search/nsSearchService.js
    (Mike de Boer             2019-02-02 11:27:21 +0000 2710)
    let errCode;
56ee1324c98ca browser/components/search/nsSearchService.js
    (gavin%gavinsharp.com     2006-03-17 07:16:00 +0000 2711)
    try {
7fa6125c63f18 toolkit/components/search/SearchService.jsm
    (Mark Banner              2019-05-01 18:51:04 +0000 2712)
    var engine = new SearchEngine({
7fa6125c63f18 toolkit/components/search/SearchService.jsm
    (Mark Banner              2019-05-01 18:51:04 +0000 2713)
    uri: engineURL,
690733b0642c3 toolkit/components/search/SearchService.jsm
    (Mark Banner              2020-03-20 16:44:05 +0000 2714)
    isBuiltin: false,
7fa6125c63f18 toolkit/components/search/SearchService.jsm
    (Mark Banner              2019-05-01 18:51:04 +0000 2715)
    });

And here's the git blame from the new method in ESR 91:

$ git blame toolkit/components/search/SearchService.jsm -L 1866
Blaming lines:  37% (1104/2969), done.
e70637b6d9207 toolkit/components/search/SearchService.jsm
    (Mark Banner              2020-07-09 09:56:25 +0000 1866)
    async addOpenSearchEngine(engineURL, iconURL) {
ed636bbf8b0b5 toolkit/components/search/SearchService.jsm
    (Mark Banner              2020-06-04 21:47:57 +0000 1867)
    logConsole.debug("addEngine: Adding", engineURL);
caceac87e1c78 toolkit/components/search/SearchService.jsm
    (Dale Harvey              2020-02-04 22:22:43 +0000 1868)
    await this.init();
481ae95c00e75 toolkit/components/search/nsSearchService.js
    (Mike de Boer             2019-02-02 11:27:21 +0000 1869)
    let errCode;
56ee1324c98ca browser/components/search/nsSearchService.js
    (gavin%gavinsharp.com     2006-03-17 07:16:00 +0000 1870)
    try {
269a6fbae9928 toolkit/components/search/SearchService.jsm
    (Mark Banner              2021-02-10 18:12:08 +0000 1871)
    var engine = new OpenSearchEngine();

What's encouraging here is that the following three lines don't just share code, they also share commit history:

caceac87e1c78 toolkit/components/search/SearchService.jsm
    (Dale Harvey              2020-02-04 22:22:43 +0000 1868)
    await this.init();
481ae95c00e75 toolkit/components/search/nsSearchService.js
    (Mike de Boer             2019-02-02 11:27:21 +0000 1869)
    let errCode;
56ee1324c98ca browser/components/search/nsSearchService.js
    (gavin%gavinsharp.com     2006-03-17 07:16:00 +0000 1870)
    try {

That suggests that the addOpenSearchEngine() method was built around the code from addEngine(). The fact that nobody bothered to update the log messages, which still use "addEngine" is also a good sign. The changes to the method name appear to have been made in commit e70637b6d9207:

$ git log -1 e70637b6d9207
commit e70637b6d92078216ad927bf23fb83e6619ef2e1
Author: Mark Banner <standard8@mozilla.com>
Date:   Thu Jul 9 09:56:25 2020 +0000

    Bug 1632448 - Remove now unused confirm and extensionID parameters for
    nsISearchService.addEngine, and rename it. r=daleharvey
    
    Renaming to addOpenSearchEngine to make it more explicit about what it is
    actually doing.
    
    Depends on D82524
    
    Differential Revision: https://phabricator.services.mozilla.com/D82525

Okay, that's very encouraging indeed. Based on all this I'm convinced that switching to use addOpenSearchEngine() is the fix we need. From the commit description it's also clear that the confirm parameter can also be safely dropped, but because that parameter is being passed in to EmbedLiteSearchEngine.js by whoever is calling it, that may need a little extra work to unravel.

Here's the simple fix that I'm applying in embedlite-components:

$ git diff
diff --git a/jscomps/EmbedLiteSearchEngine.js b/jscomps/EmbedLiteSearchEngine.js
index b224f10..b3fc3a3 100644
--- a/jscomps/EmbedLiteSearchEngine.js
+++ b/jscomps/EmbedLiteSearchEngine.js
@@ -50,7 +50,7 @@ EmbedLiteSearchEngine.prototype = {
         var data = JSON.parse(aData);
         switch (data.msg) {
           case "loadxml": {
-            Services.search.addEngine(data.uri, null, data.confirm).then(
+            Services.search.addOpenSearchEngine(data.uri, null).then(
               engine => {
                 var message = {
                   "msg": "search-engine-added",

The embedui:search event is triggered from a few places, all of which are in the sailfish-browser code. That makes sense as this is essentially the interface used to allow the QML front end to interact with the gecko backend. Here are the cases where it appears:

$ grep -rIn "embedui:search" *
apps/core/datafetcher.cpp:169:
    SailfishOS::WebEngine::instance()->notifyObservers(QLatin1String(
    "embedui:search"), QVariant(loadsearch));
apps/core/settingmanager.cpp:126:
    webEngine->notifyObservers(QLatin1String("embedui:search"),
    QVariant(defaultSearchEngine));
apps/core/settingmanager.cpp:160:
    webEngine->notifyObservers(QLatin1String("embedui:search"),
    QVariant(loadsearch));
apps/core/settingmanager.cpp:170:
    webEngine->notifyObservers(QLatin1String("embedui:search"),
    QVariant(removeMsg));
apps/browser/settings/searchenginemodel.cpp:168:
    SailfishOS::WebEngine::instance()->notifyObservers(QLatin1String(
    "embedui:search"), QVariant(message));

Of these, only the following use the loadxml topic. These are the ones we're really interested in:

apps/core/datafetcher.cpp:169:
    SailfishOS::WebEngine::instance()->notifyObservers(QLatin1String(
    "embedui:search"), QVariant(loadsearch));
apps/core/settingmanager.cpp:160:
    webEngine->notifyObservers(QLatin1String("embedui:search"),
    QVariant(loadsearch));

These all pass in a confirm parameter. Strictly speaking we could leave these as they are since the parameter will now just be ignored, but for simplicity and cleanliness of code it makes sense to remove them. So I've also made these changes to the sailfish-browser code:

$ git diff
diff --git a/apps/core/datafetcher.cpp b/apps/core/datafetcher.cpp
index 8aea2874..bcef476f 100644
--- a/apps/core/datafetcher.cpp
+++ b/apps/core/datafetcher.cpp
@@ -165,7 +165,6 @@ void DataFetcher::saveAsSearchEngine()
                 QVariantMap loadsearch;
                 loadsearch.insert(QLatin1String("msg"), QVariant(QLatin1String("loadxml")));
                 loadsearch.insert(QLatin1String("uri"), QVariant(url.toString()));
-                loadsearch.insert(QLatin1String("confirm"), QVariant(false));
                 SailfishOS::WebEngine::instance()->notifyObservers(QLatin1String("embedui:search"), QVariant(loadsearch));
 
                 updateStatus(Ready);
diff --git a/apps/core/settingmanager.cpp b/apps/core/settingmanager.cpp
index fee5eb28..e84afa57 100644
--- a/apps/core/settingmanager.cpp
+++ b/apps/core/settingmanager.cpp
@@ -156,7 +156,6 @@ void SettingManager::handleObserve(const QString &message, const QVariant &data)
                     // load opensearch descriptions
                     loadsearch.insert(QLatin1String("msg"), QVariant(QLatin1String("loadxml")));
                     loadsearch.insert(QLatin1String("uri"), QVariant(QString("file://%1").arg(configs[searchName])));
-                    loadsearch.insert(QLatin1String("confirm"), QVariant(false));
                     webEngine->notifyObservers(QLatin1String("embedui:search"), QVariant(loadsearch));
                 }
             }

Straightforward stuff, but making these changes still feels good.

Making these changes fixes the error output, which is great. While testing the changes I also experienced the following error, which I've created a new issue for:

JavaScript Error: "TypeError: XPCOMUtils.generateNSGetFactory is not a function"
    {file: "file:///usr/lib64/mozembedlite/components/PromptService.js" line: 955}

In fact, if I try to search in the address bar I get the following couple of errors:

JavaScript error: resource://gre/modules/URIFixup.jsm, line 443:
    NS_ERROR_MALFORMED_URI: Couldn't build a valid uri
JavaScript error: file:///usr/lib64/mozembedlite/components/PromptService.js,
    line 955: TypeError: XPCOMUtils.generateNSGetFactory is not a function

These two errors prevent the search from working at all so will need to be fixed.

I reckon the second of these about XPCOMUtils.generateNSGetFactory not being a function may be the same as the issue we saw on Day 88. In that case the error was coming from EmbedLiteGlobalHelper.js and we fixed it by switching XPCOMUtils to ComponentUtils. Maybe the same thing will work again.

The PromptService.js file can be found in embedlite-components, so I've made the equivalent changes to that:

$ git diff jscomps/PromptService.js
diff --git a/jscomps/PromptService.js b/jscomps/PromptService.js
index 86117f4..996d8fb 100644
--- a/jscomps/PromptService.js
+++ b/jscomps/PromptService.js
@@ -6,6 +6,7 @@ const Cc = Components.classes;
 const Cr = Components.results;
 const Cu = Components.utils;
 
+const { ComponentUtils } = ChromeUtils.import("resource://gre/modules/ComponentUtils.jsm");
 const { XPCOMUtils } = ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
 const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");
 const { PrivateBrowsingUtils } = ChromeUtils.import("resource://gre/modules/PrivateBrowsingUtils.jsm");
@@ -952,4 +953,4 @@ AuthPromptAdapter.prototype = {
   }
 };
 
-this.NSGetFactory = XPCOMUtils.generateNSGetFactory([PromptService, AuthPromptAdapterFactory]);
+this.NSGetFactory = ComponentUtils.generateNSGetFactory([PromptService, AuthPromptAdapterFactory

The next error I need to address is the NS_ERROR_MALFORMED_URI that's coming from docshell/base/URIFixup.jsm. More specifically it's happening in a call to the getFixupURIInfo(uriString, fixupFlags = FIXUP_FLAG_NONE) method.

The method is trying — really quite hard in fact — to interpret the text I enter as a URL of some sort. After all other attempts to force it into something coherent have failed it falls through into this bit of code, which is the code generating the error:

    if (!info.preferredURI) {
      // We couldn't salvage anything.
      throw new Components.Exception(
        "Couldn't build a valid uri",
        Cr.NS_ERROR_MALFORMED_URI
      );
    }

Rather than doing this it should be interpreting the text as a search string and calling the search provider. The URL bar search won't work without this, so it'll need to be fixed.

But it's getting quite late here now, so time to call it a day. I'll return to this in the morning.

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