1748506 - No chrome package registered for chrome://browser/content/built_in_addons.json

Status: NEW

(WebExtensions :: Android, task, P3)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Running mach wpt tests on mobile (for at least artifact builds) the org.mozilla.geckoview.test_runner application gets killed immediately because of the missing registration of the following chrome package:

GeckoConsole: No chrome package registered for chrome://browser/content/built_in_addons.json

The crash is always reproducible since Firefox 92 when the fix on bug 1721627 got landed.

Note that this is not a problem in CI, and only affects local jobs.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Since bug 1459004 this file is auto-generated. Kris, do you have an idea why it is not registered correctly for local builds?

Comment 2

[:Gijs (he/him)]

It might not be correctly registered for any builds... see bug 1724718.

It's meant to be generated here: https://searchfox.org/mozilla-central/rev/253ae246f642fe9619597f44de3b087f94e45a2d/toolkit/mozapps/extensions/moz.build#30-33 .

I don't know why that wouldn't run for either this app or why the result doesn't get packaged for test_runner.

If this is blocking something you could just create an exception in the relevant crashy code so we ignore this file.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to :Gijs (he/him) from comment #2)

If this is blocking something you could just create an exception in the relevant crashy code so we ignore this file.

I assume this needs to go into nsNetUtil.cpp and should be something like the following?

+#ifdef ANDROID
+  // See bug 1748506
+  if (StringEndsWith(filePath, "/built_in_addons.json"_ns)) {
+    return;
+  }
+#endif

When searching for its usage I found this interesting code:
https://searchfox.org/mozilla-central/rev/fbf1e796ecead9484deced4d99f199f327ba25ab/toolkit/mozapps/extensions/internal/AddonTestUtils.jsm#1851-1874

As it looks like we override this file in automation with override.txt. Maybe this is the reason why it works in CI but not locally? But I cannot find any code that actually calls this method overrideBuiltIns().

Comment 4

[:Gijs (he/him)]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #3)

(In reply to :Gijs (he/him) from comment #2)

If this is blocking something you could just create an exception in the relevant crashy code so we ignore this file.

I assume this needs to go into nsNetUtil.cpp and should be something like the following?

+#ifdef ANDROID
+  // See bug 1748506
+  if (StringEndsWith(filePath, "/built_in_addons.json"_ns)) {
+    return;
+  }
+#endif

Yes, but please make sure the bug link points to a bug that stays open to actually fix the issue.

When searching for its usage I found this interesting code:
https://searchfox.org/mozilla-central/rev/fbf1e796ecead9484deced4d99f199f327ba25ab/toolkit/mozapps/extensions/internal/AddonTestUtils.jsm#1851-1874

As it looks like we override this file in automation with override.txt.

No, we override it when that method is called, which is only the case for a few xpcshell tests: https://searchfox.org/mozilla-central/search?q=overrideBuiltIns&path=&case=true&regexp=false .

The references to automation are to do with Cu.isInAutomation and/or xpc::IsInAutomation checks in code, which rely on the PREF_DISABLE_SECURITY pref as well as the "no external socket connections" environment variable to determine that we're running automated tests. Those are all true for local runs as well (so IsInAutomation refers to "we're running an automated test", not to "we're running in Mozilla's automation").

Maybe this is the reason why it works in CI but not locally? But I cannot find any code that actually calls this method overrideBuiltIns().

No. As I tried to point out before (see the bug I linked), I think the difference with CI might be that the "missing chrome/resource files" checks are imperfect at the moment. For desktop, I know that they currently do not check some things that they should be checking, and those checks do run for local test runs (unless you run ./mach package first and then run the test with --appname dist or similar - the cause is related to whether files get packaged into jar files or are left as file URL symlinks). But I'm not sure if this is also the case for android, because I thought that for running the test on an emulator/phone we always packaged everything in order to install it into the emulator/phone.

Anyway, you could see if with the remaining unlanded patch in that bug, the same crashes happen on CI.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1748506 - Don't crash for missing registration of "chrome://browser/content/built_in_addons.json" on Android.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to :Gijs (he/him) from comment #4)

Yes, but please make sure the bug link points to a bug that stays open to actually fix the issue.

Thanks. I'll use this bug and mark it as leave-open for Kris to follow-up.

No. As I tried to point out before (see the bug I linked), I think the difference with CI might be that the "missing chrome/resource files" checks are imperfect at the moment. For desktop, I know that they currently do not check some things that they should be checking, and those checks do run for local test runs (unless you run ./mach package first and then run the test with --appname dist or similar - the cause is related to whether files get packaged into jar files or are left as file URL symlinks). But I'm not sure if this is also the case for android, because I thought that for running the test on an emulator/phone we always packaged everything in order to install it into the emulator/phone.

Ah, I see! The packaged part makes sense for sure. It's not something that I explicitly run locally, and not sure neither if the install step is doing it behind the scenes.

So I've made the change and I'm getting further during the start-up of the application. It's no longer crashing immediately. Maybe the rest around wrong permissions has to do with wpt and should not be covered by this bug. I'll trigger a try build and ask for review. Thanks Gijs!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/89f98c8c01d4
Don't crash for missing registration of "chrome://browser/content/built_in_addons.json" on Android. r=necko-reviewers,Gijs,valentin

Comment 8

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/89f98c8c01d4

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

As discussed on Element the landed patch didn't actually fix the problem. The crash as seen might be something different and not be caused by this particular missing file.

Sheriffs please backout. Thanks.

Comment 10

[Iulian Moraru]

Backed out as per whimboo's request.

Backout link

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Updating summary to reflect that this is not the reason for the crash and as such the bug might not be that important. The underlying issue for the crash will be further covered on bug 1748626.

Comment 12

[Sandor Molnar[:smolnar]]

Backout merged to central:

Backout link: https://hg.mozilla.org/mozilla-central/rev/1f0414f5b60bd254d30eb61af03575bc5c02c89f

Comment 13

[Tomislav Jovanovic :zombie]

This is only logspam with no user impact, changing to a task.