flypig.co.uk

Yesterday we started looking at the errors output from the privileged JavaScript shims, in particular the errors related to Services.appinfo like this one:

JavaScript error: resource://gre/modules/LoginRecipes.jsm, line 56:
    TypeError: Services.appinfo is undefined

What we found was that the Services object is being instantiated correctly but without the appinfo member. If we hack this back in manually, the errors go away.

So we need to figure out why Services is missing this piece. Running git blame on the Services.jsm file gives us a clue:

$ git blame toolkit/modules/Services.jsm
Blaming lines: 100% (7/7), done.
82ff7027aac0f Services.jsm (Gervase Markham 2012-05-21 12:12:37 +0100 1)
     /* This Source Code Form is subject to the terms of the Mozilla Public
82ff7027aac0f Services.jsm (Gervase Markham 2012-05-21 12:12:37 +0100 2)
      * License, v. 2.0. If a copy of the MPL was not distributed with this
82ff7027aac0f Services.jsm (Gervase Markham 2012-05-21 12:12:37 +0100 3)
      * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
eaf57306e97c0 Services.jsm (Gavin Sharp     2010-01-28 13:31:45 -0500 4) 
682b1ec3b2c92 Services.jsm (Florian Quèze   2018-02-23 20:50:01 +0100 5)
    var EXPORTED_SYMBOLS = ["Services"];
eaf57306e97c0 Services.jsm (Gavin Sharp     2010-01-28 13:31:45 -0500 6) 
a1cb850855b77 Services.jsm (Kris Maglione   2020-07-09 21:42:53 +0000 7)
    var Services = Cu.createServicesCache();

Strictly speaking we're interested in why all of the following lines were removed, but these aren't shown here because only lines that exist in the latest version are shown. But we can use the last line to find out. It's more than likely this line was changed at the same time the others were removed.

Checking the diff for this change confirms this. The amount of removed code is large, so I'm not going to show it all here, but here's an extract to provide a flavour:

$ git diff a1cb850855b77~ a1cb850855b77 toolkit/modules/Services.jsm
diff --git a/toolkit/modules/Services.jsm b/toolkit/modules/Services.jsm
index 0dc46f4fe5f8..9eb47cd45653 100644
--- a/toolkit/modules/Services.jsm
+++ b/toolkit/modules/Services.jsm
@@ -2,171 +2,6 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-/* eslint mozilla/use-services:off */
-
[...]
-XPCOMUtils.defineLazyServiceGetters(Services, initTable);
-
-initTable = undefined;
+var Services = Cu.createServicesCache();

Let's have a read of the git log associated with this change.

$ git log -1 a1cb850855b77
commit a1cb850855b77c78710637bd946ab62d14ba1b0d
Author: Kris Maglione 
Date:   Thu Jul 9 21:42:53 2020 +0000

    Bug 1464542: Part 3c - Change Services.jsm to use the C++-implemented
    services cache. r=mccr8
    
    Differential Revision: https://phabricator.services.mozilla.com/D81420

Checking this log message leads us back to the Phabricator changeset, which leads us to the Bugzilla Bug, which leads us to the full set of six changesets that are associated with the bug. One of those is changeset D81419 with the title "Part 3b - Add existing Services.jsm registrations to the new services cache". This is the one that's especially of interest to us.

If we take a look at the changes to toolkit/xre/components.conf in this changeset we can see that some additional annotations have been added to the appinfo component definition.

Referring back to the original issue in Bugzilla, we can see that the purpose was to migrate the code in Services.jsm from this JavaScript file to the C++ code. As part of that process an approach was implemented to automatically attach classes to Services, as we can see from this comment:
 
I believe this is our smoking gun.

It's early morning here and I have to head to work, so I'll continue digging in to these changes this evening.

[...]

It's evening now, time to get back to gecko debugging. So it seems that the problem is that the appinfo interface that used to be provided in JavaScript is now provided through a C++ interface. Annotations added to the components.conf file make these interfaces available in a persistent form through the Services interface.

However, with EmbedLite, for both ESR 78 and ESR 91 we specifically remove the default appinfo interface and replace it with our own. This is done through patch 0072 which has the title "Cleanup static components definitions". Here's our EmbedLite change in action:

diff --git a/toolkit/xre/components.conf b/toolkit/xre/components.conf
 Classes = [
-    {
-        'cid': '{95d89e3e-a169-41a3-8e56-719978e15b12}',
-        'contract_ids': [
-            '@mozilla.org/xre/app-info;1',
-            '@mozilla.org/xre/runtime;1',
-        ] + crash_reporter,
-        'legacy_constructor': 'mozilla::AppInfoConstructor',
-        'headers': ['nsAppRunner.h'],
-        'processes': ProcessSelector.ALLOW_IN_SOCKET_PROCESS,
-    },

Although it may not be entirely clear from this, this change is removing the appinfo class from the configuration. The upstream diff that's applied between ESR 78 and ESR 91 would have changed this section — had it not been deleted — to the following:

Classes = [
    {
        'js_name': 'appinfo',
        'cid': '{95d89e3e-a169-41a3-8e56-719978e15b12}',
        'contract_ids': [
            '@mozilla.org/xre/app-info;1',
            '@mozilla.org/xre/runtime;1',
        ] + crash_reporter,
        'interfaces': ['nsIXULRuntime', 'nsIXULAppInfo'],
        'legacy_constructor': 'mozilla::AppInfoConstructor',
        'headers': ['nsAppRunner.h'],
        'processes': ProcessSelector.ALLOW_IN_SOCKET_PROCESS,
        'overridable': True,
    },

This update version is clearer in my view, because it makes the appinfo connection explicit. In EmbedLite this gets added back in to the embedding/embedlite/components/components.conf file. But of course, currently it doesn't have the additional annotations. So my plan is to add in the annotations, perform a rebuild and see whether this helps. Here's the change I've made:

$ git diff
diff --git a/embedding/embedlite/components/components.conf
           b/embedding/embedlite/components/components.conf
index b13472df7051..a86a9430c57c 100644
--- a/embedding/embedlite/components/components.conf
+++ b/embedding/embedlite/components/components.conf
@@ -14,15 +14,18 @@ Classes = [
         'headers': ['EmbedWidgetFactoryRegister.h'],
     },
     {
+        'js_name': 'appinfo',
         'cid': '{bb11767a-9c26-11e2-bfb2-9f3b52956e00}',
         'contract_ids': [
             '@mozilla.org/xre/app-info;1',
             '@mozilla.org/xre/runtime;1'
         ],
+        'interfaces': ['nsIXULRuntime', 'nsIXULAppInfo'],
         'singleton': True,
         'type': 'mozilla::embedlite::EmbedLiteXulAppInfo',
         'categories': {'app-startup': 'EmbedLite Xul App Info Component'},
         'constructor': 'mozilla::embedlite::EmbedLiteXulAppInfo::GetSingleton',
-        'headers': ['/include/mozilla/embedlite/EmbedLiteXulAppInfo.h']
+        'headers': ['/include/mozilla/embedlite/EmbedLiteXulAppInfo.h'],
+        'overridable': True,
     },
 ]

Having made this change I now need to perform the rebuild. Unfortunately it's going to be a full rebuild because this is quite an intrusive changes. Tomorrow I'll see whether it's had any positive effect.

This is a small change. If it fixes it then that will be great, if it doesn't it'll need more investigation. But already I feel like we're making progress again. I much prefer these small, self-contained tasks compared to the lengthy open-ended debugging tasks (like the rendering fixes). I'm looking forward to more of these.

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