TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_theme_update.js | xpcshell return code: 0 TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_theme_update.js | test_theme_updates - [test_theme_updates : 123] Have 2 lightweight themes - 3 == 2 New in the last merge, so M-C last good: 35bd1a865ff5262e05f2e421d499e8c42a M-C first bad: 21bc698a4d9c52fa2d9f6fb04e554ad4ec https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=35bd1a865ff5262e05f2e421d499e8c42a&tochange=21bc698a4d9c52fa2d9f6fb04e554ad4ec Certainly from: e2c7f43bb207 Andrew Swan - Bug 1430276 Provide a method to update LWTs to xpi-packaged themes r=kmag which create that test: test_theme_update.js https://hg.mozilla.org/mozilla-central/rev/e2c7f43bb207#l3.3 Andrew and Kris, how can we get this to pass in Thunderbird? Look like this fails on line 123: + equal(filterThemes(LightweightThemeManager.usedThemes).length, 2, + "Have 2 lightweight themes"); Richard, how many LW themes are there in TB?

(In reply to Jorg K (GMT+2) from comment #0) > Andrew and Kris, how can we get this to pass in Thunderbird? Without knowing why its failing, its hard to say. What is the third (unexpected) theme when the test fails? Will thunderbird participate in the upcoming transition from LWTs to xpi-packaged themes? If not, its probably easier to just skip this test.

Thanks Andrew, I'll check it locally once my build has completed. I know nothing about the LWT to xpi-packaged transition, perhaps Richard does. Which bug is that?

(In reply to Jorg K (GMT+2) from comment #0) > > Richard, how many LW themes are there in TB? We have no LW-themes included in TB itself like FX has. This is probably why it's failing. (In reply to Andrew Swan [:aswan] from comment #1) > Will thunderbird participate in the upcoming transition from LWTs to > xpi-packaged themes? If not, its probably easier to just skip this test. Is this for all LW-themes, also on AMO, or only the included ones?

I've changed that block to: let themes = filterThemes(await AddonManager.getAddonsByTypes(["theme"])); equal(themes.length, 3, "Still have a total of 3 themes"); 123 equal(filterThemes(LightweightThemeManager.usedThemes).length, 3, // <== changed to 3 "Have 2 lightweight themes"); for (let ii = 0; ii <3; ii++) dump(LightweightThemeManager.usedThemes[ii].name+"\n"); theme1 = themes.find(t => t.name == "Theme 1"); 127 notEqual(theme1, undefined, "Found theme1"); 128 ok(!theme1.id.endsWith("@personas.mozilla.org"), "Theme 1 has been updated to an XPI packaged theme"); equal(theme1.userDisabled, true, "Theme 1 is not active"); and get this: 0:03.89 PASS test_theme_updates - [test_theme_updates : 122] Still have a total of 3 themes - 3 == 3 0:03.89 PASS test_theme_updates - [test_theme_updates : 123] Have 2 lightweight themes - 3 == 3 0:03.89 pid:2476 Theme 2 0:03.89 pid:2476 Theme 3 0:03.89 pid:2476 Theme 1 0:03.90 PASS test_theme_updates - [test_theme_updates : 127] Found theme1 - {} != "undefined" 0:03.90 FAIL test_theme_updates - [test_theme_updates : 128] Theme 1 has been updated to an XPI packaged theme - false == true Looks like "Theme 1" should have been converted but wasn't. So what's the best way forward here? Please also note the questions in comment #2 and comment #3.

Firefox only has one built-in LWT for xpcshell tests, which is the stub default theme. Thunderbird should have the same.

(In reply to Richard Marti (:Paenglab) from comment #3) > We have no LW-themes included in TB itself like FX has. This is probably why > it's failing. The error indicates it is finding too many LWTs, not too few, so I don't think that's the problem. > > Will thunderbird participate in the upcoming transition from LWTs to > > xpi-packaged themes? If not, its probably easier to just skip this test. > > Is this for all LW-themes, also on AMO, or only the included ones? This is for LWTs hosted on AMO. (In reply to Jorg K (GMT+2) from comment #2) > Thanks Andrew, I'll check it locally once my build has completed. I know > nothing about the LWT to xpi-packaged transition, perhaps Richard does. > Which bug is that? Most of this work is happening on AMO so it is not tracked in bugzilla. This github project covers the work: https://github.com/mozilla/addons-server/labels/project%3A%20static%20themes (In reply to Jorg K (GMT+2) from comment #4) > Looks like "Theme 1" should have been converted but wasn't. It does look like it. Does thunderbird even support xpi packaged themes?

(In reply to Andrew Swan [:aswan] from comment #6) > (In reply to Jorg K (GMT+2) from comment #4) > > Looks like "Theme 1" should have been converted but wasn't. > It does look like it. Does thunderbird even support xpi packaged themes? I have no idea. Richard, do you know? In which bug where they introduced?

It was a long-ish process but eg bug 1330337

(In reply to Andrew Swan [:aswan] from comment #6) > > Does thunderbird even support xpi packaged themes? If you mean something like this, yes.

So if it does, we should get the test to work instead of adding yet another disabled test to the pile we already have. How can we achieve this and where would we start investigating?

Re comment 4, the update from a LWT to an xpi theme didn't work for some reason. That test should already run with pretty detailed debug logs, is there anything in them to indicate why the update wasn't applied?

Here's the log file. A few Warning: "ReferenceError: reference to undefined property "description" Maybe this is relevant: WARN Download of http://example.com/theme1.xpi failed: Error: Cannot find id for addon c:\\users\\jorgk\\appdata\\local\\temp\\firefox\\xpcshellprofile\\temp\\tmp-6qe.xpi (resource://gre/modules/addons/XPIInstall.jsm:1543:19) JS Stack trace: loadManifest@XPIInstall.jsm:1543:19 I vaguely recall another test failure where we had to add an ID for TB: https://hg.mozilla.org/mozilla-central/rev/bb912ad63b04

This does the trick. Same as: https://hg.mozilla.org/mozilla-central/rev/bb912ad63b04#l1.12

I'd prefer that we find a way to avoid adding unnecessary ids to all our test extensions. Is there a good reason thunderbird can't build with ADDON_SIGNING enabled? (not REQUIRE_SIGNING...)

I don't know. Philipp, FRG? See bug 1458682 comment #11 (quote): Talked to :Fallen on IRC, Thunderbird is built with ADDON_SIGNING=0 so the logic for extracting the id from a synthetic certificate in xpcshell doesn't kick in. I'm not sure what a good long-term strategy is for Thunderbird but this is fine for now.

> I'm not sure what a good long-term strategy is for Thunderbird but this is fine for now. Can't speak for TB but I am quite sure nobody from the team wants to enable addon signing in SM. It would just be a nuisance for our users.

Jolly good, now I'm caught up in the add-on signing discussion when all I wanted to do is fixing some tests as part of sheriffing the C-C tree :-( Here's an update patch which clearly marks the code, also the one inserted in bug 1458682. Since we've already gone down this road "for now", I don't see whether we can't go a little further.

Alternative patch to disable the test.

Let's see what switching to MOZ_ADDON_SIGNING=1 does: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=915d6b2c9de57fb756b1628199d37262636439f2

OK, that fixed the test failures, so now we need to decide whether we want to go with that. Andrew, what are the consequences of add-on signing?

(In reply to Frank-Rainer Grahl (:frg) from comment #16) > > I'm not sure what a good long-term strategy is for Thunderbird but this is fine for now. > > Can't speak for TB but I am quite sure nobody from the team wants to enable > addon signing in SM. It would just be a nuisance for our users. I think there may be some confusion about ADDON_SIGNING versus REQUIRE_SIGNING. The former just controls whether the add-ons manager looks at signatures. Even if ADDON_SIGNING is enabled, unsigned (or incorrectly signed) addons may still be loaded, depending on the type of the addon and various preferences. The REQUIRE_SIGNING option tightens those rules so that unsigned extensions cannot be installed, even with a preference flip. With ADDON_SIGNING enabled but xpinstall.signatures.required set to false, I don't believe there should be any behavior difference. The try run from comment 19 looks pretty clean, landing that looks like the ideal route to go here..

Thanks Andrew. I've added pref("xpinstall.signatures.required", false); I'll follow up to remove https://hg.mozilla.org/mozilla-central/rev/bb912ad63b04#l1.12 again.

Hmm, that appears to be the default in toolkit: mozilla/modules/libpref/init/all.js 4903 pref("xpinstall.signatures.required", false);

Comment on attachment 8984959 [details] [diff] [review] 1468018-MOZ_ADDON_SIGNING.patch Actually, maybe better this one that doesn't repeat the toolkit preference.

Comment on attachment 8985036 [details] [diff] [review] 1468018-MOZ_ADDON_SIGNING.patch (v2) Review of attachment 8985036 [details] [diff] [review]: ----------------------------------------------------------------- I'm not aware of any disadvantage to doing this. r=mkmelin ::: mail/config/mozconfigs/l10n-common @@ +6,5 @@ > > # Needed to enable breakpad in application.ini > export MOZILLA_OFFICIAL=1 > > # Disable checking that add-ons are signed by the trusted root You'll want to fix these comments.

Let's go with this, Magnus gave his OK in our developers' meeting.

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/6b07fdab82bf Require MOZ_ADDON_SIGNING in Thunderbird to fix M-C test failures. r=mkmelin