Closed Bug 1402471 Opened 2 years ago Closed 2 years ago

Massive Xpcshell test failure for tests from toolkit/mozapps/extensions/test/xpcshell on comm-beta

Categories

(Thunderbird :: General, defect)

defect
Not set

Tracking

(thunderbird57 fixed, thunderbird58 fixed)

RESOLVED FIXED
Thunderbird 58.0
Tracking Status
thunderbird57 --- fixed
thunderbird58 --- fixed

People

(Reporter: jorgk, Unassigned)

References

Details

Attachments

(1 file, 2 obsolete files)

TEST-UNEXPECTED-FAIL | xpcshell-unpack.ini:toolkit/mozapps/extensions/test/xpcshell/test_AddonRepository_cache.js | xpcshell return code: 0 [log…]
TEST-UNEXPECTED-FAIL | xpcshell-unpack.ini:toolkit/mozapps/extensions/test/xpcshell/test_bug299716.js | xpcshell return code: 0 [log…]
TEST-UNEXPECTED-FAIL | xpcshell-unpack.ini:toolkit/mozapps/extensions/test/xpcshell/test_install_strictcompat.js | xpcshell return code: 0 [log…] 
and many more.

This started after the latest merge. Only on comm-beta.

Kris, any hints for us?
Flags: needinfo?(kmaglione+bmo)
Looking for example at test_install_strictcompat.js, that fails on line 74:

11:50:18     INFO -  TEST-PASS | xpcshell-unpack.ini:toolkit/mozapps/extensions/test/xpcshell/test_install_strictcompat.js | run_test_1/< - [run_test_1/< : 73] 737 == 737
11:50:18  WARNING -  TEST-UNEXPECTED-FAIL | xpcshell-unpack.ini:toolkit/mozapps/extensions/test/xpcshell/test_install_strictcompat.js | run_test_1/< - [run_test_1/< : 74] false == true

    do_check_true(install.addon.hasResource("install.rdf"));
    do_check_eq(install.addon.install, install);
73  do_check_eq(install.addon.size, ADDON1_SIZE);
74  do_check_true(hasFlag(install.addon.operationsRequiringRestart,
                          AddonManager.OP_NEEDS_RESTART_INSTALL));
Sorry, I don't know. My best guess is that it has something to do with the toolkit code that disables legacy extension support on release branches. I don't know how that flag affects Thunderbird, but in Firefox we get around it for tests on release branches by checking the Cu.isInAutomation flag.
Flags: needinfo?(kmaglione+bmo)
Or taking test_upgrade_strictcompat.js, fails at line 112:

11:50:44     INFO -  TEST-PASS | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_upgrade_strictcompat.js | run_test_1/< - [run_test_1/< : 111] {} != null
11:50:44  WARNING -  TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/mozapps/extensions/test/xpcshell/test_upgrade_strictcompat.js | run_test_1/< - [run_test_1/< : 112] false == true

  AddonManager.getAddonsByIDs(["addon1@tests.mozilla.org",
                               "addon2@tests.mozilla.org",
                               "addon3@tests.mozilla.org",
                               "addon4@tests.mozilla.org"],
                               function([a1, a2, a3, a4]) {

111 do_check_neq(a1, null);
112 do_check_true(isExtensionInAddonsList(profileDir, a1.id));
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #2)
> Sorry, I don't know. My best guess is that it has something to do with the
> toolkit code that disables legacy extension support on release branches. I
> don't know how that flag affects Thunderbird, but in Firefox we get around
> it for tests on release branches by checking the Cu.isInAutomation flag.
Thanks, Kris, I'm not sure what to make of this. Cu.isInAutomation is checked in toolkit
http://searchfox.org/mozilla-central/search?q=isInAutomation&case=false&regexp=false&path=
and the failing tests are from toolkit. So I don't know what else we should do in TB.

Maybe Tom can take a look.
Flags: needinfo?(mozilla)
You mean:
12:14:21     INFO -  PID 23428 | JavaScript strict warning: resource://testing-common/httpd.js, line 800: ReferenceError: reference to undefined property "_stopCallback"
12:14:21     INFO -  PID 23428 | !!! error running onStopped callback: TypeError: callback is not a function
12:14:21     INFO -  "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "_stopCallback"" {file: "resource://testing-common/httpd.js" line: 800}]"
12:14:21     INFO -  <<<<<<<

This one?
https://dxr.mozilla.org/comm-central/source/mozilla/services/sync/tps/extensions/mozmill/resource/stdlib/httpd.js#800

And why would that only fail on beta?
(In reply to Tom Prince [:tomprince] from comment #5)
> https://treeherder.mozilla.org/logviewer.html#?job_id=132740257&repo=comm-
> beta&lineNumber=14872 looks like it might be the proximate cause.

Unlikely. That's pretty normal.

(In reply to Jorg K (GMT+2) from comment #4)
> Thanks, Kris, I'm not sure what to make of this. Cu.isInAutomation is
> checked in toolkit
> http://searchfox.org/mozilla-central/
> search?q=isInAutomation&case=false&regexp=false&path=
> and the failing tests are from toolkit. So I don't know what else we should
> do in TB.

The check is, but the ways that property winds up getting set are complicated. But it's possible the failure is just related to different legacy/unsigned extension configurations for Thunderbird release builds.
See Also: → 1404638
Attached patch 1402471-legacyenable.patch (obsolete) — Splinter Review
I did a similar patch for SeaMonkey in bug 1404638. Not sure if I should ask for review or if you would like the check it out first. Setting the option locally in the mozconfig works fine.
Comment on attachment 8914013 [details] [diff] [review]
1402471-legacyenable.patch

Review of attachment 8914013 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, FRG. I'll let Tom look at this. Tom, SM deployed a similar patch successfully.

::: mail/config/mozconfigs/linux32/l10n-mozconfig
@@ +1,2 @@
>  . $topsrcdir/build/unix/mozconfig.linux32
> +. "$topsrcdir/mail/config/mozconfigs/common"

I'm confused, what should have quotes and what shouldn't?
Attachment #8914013 - Flags: review?(mozilla)
Comment on attachment 8914013 [details] [diff] [review]
1402471-legacyenable.patch

Review of attachment 8914013 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. r+ after renaming the common file, with a green build. (There are potential ordering effects from changing the order, but I don't think they apply to any of the changes here).

::: mail/config/mozconfigs/common
@@ +1,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# 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/.
> +
> +# This file is included by all mail mozconfigs

I think we should put this at .../common/base or something. Down the line, it would make sense to abstract out the commonalities in each of ../nightly, ../release ../debug and ../l10n-mozconfig, and those probably all want to live in a ../common/ directory.

::: mail/config/mozconfigs/linux32/debug
@@ +7,5 @@
>  ac_add_options --disable-webrender
>  
>  ac_add_options --enable-application=mail
>  ac_add_options --enable-debug
>  ac_add_options --enable-calendar

Probably this stanza (and several others too) could be moved into the common file, but I can do that in a follow up.

::: mail/config/mozconfigs/linux32/l10n-mozconfig
@@ +1,2 @@
>  . $topsrcdir/build/unix/mozconfig.linux32
> +. "$topsrcdir/mail/config/mozconfigs/common"

Probably ~everything should be quoted.
Attachment #8914013 - Flags: review?(mozilla) → review+
Flags: needinfo?(mozilla)
Attached patch 1402471-legacyenable.patch (v2) (obsolete) — Splinter Review
Renamed the file to common/base and added quotes everywhere. Try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c37b3ce6527f8a1d27ef46c102166827eab296da

Beta uplift if it works ;-)
Attachment #8916315 - Flags: review+
Attachment #8916315 - Flags: approval-comm-beta+
It didn't work, I can't tell what's gone wrong here:
Error loading mozconfig: /builds/slave/tb-try-c-cen-l64-d-00000000000/build/.mozconfig

Tom, can you please take a look.
Flags: needinfo?(mozilla)
It looks like when `mach` is invoked from a comm-central objdir, it evaluates mozconfig with $topsrcdir pointing at the mozilla-central directory, which causes the include to point to a non-existent file. This doesn't matter if the includes point to files shared between c-c and m-c (in build/ for example), since the results of reading the mozconfig are mostly ignored.
Thanks for the answer which I sadly don't understand. So what needs to change in the patch?

SM landed their version here
https://hg.mozilla.org/comm-central/rev/2f034b0789b5
https://hg.mozilla.org/comm-central/rev/c6fafa3db46a
and it must be working for them.
That wasn't a complete answer, just a summary of my debugging I had managed to do. The patch is probably correct, but tickles a bug in how mach finds the topdir/processes mozconfig. SM probably doesn't tickle the bug because their build automation is different. I'll have a look at this tomorrow, but it will probably end up duplicating the code, unfortunately.
Attachment #8916315 - Attachment is obsolete: true
Attachment #8916315 - Flags: approval-comm-beta+
Comment on attachment 8914013 [details] [diff] [review]
1402471-legacyenable.patch

Thanks, FRG, but it doesn't work for us.
Attachment #8914013 - Attachment is obsolete: true
Flags: needinfo?(mozilla)
(In reply to Jorg K (GMT+2) from comment #17)
> Thanks, FRG, but it doesn't work for us.

To be clear, the idea works, but due to unfortunate interaction between mach and mozconfig, we can't have mozconfig include files that don't exist in both c-c and m-c. This is because mach tries to load mozconfig's relative to m-c when run from objdir, which our build system currently does.
Comment on attachment 8917131 [details]
Bug 1402471 - Enable legacy extensions in Thunderbird configs;

Jorg, if the try runs fix enough of the test failures to your satisfaction, this can be pushed to comm-central and comm-beta.
Attachment #8917131 - Flags: review?(mozilla) → review+
Hmm, how do you do that beta try magic?

Looks like Xpcshell comes back to life with that patch which is good. You can also see that Mac builds on beta are broken, so simply uplift bug 1400533 comment #13?
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/e8fac029f7e4
Enable legacy extensions in Thunderbird configs; r=tomprince
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
I've landed that since it won't do damage on C-C and seems to fix things on C-B. As soon as I hear about the Mac build bug 1400533, I'll do a beta uplift round.
Target Milestone: --- → Thunderbird 58.0
Attachment #8917131 - Flags: approval-comm-beta+
I rebased the patch onto comm-beta, and then used https://hg.mozilla.org/try-comm-central/rev/66aa85d13a770d329a283b3e8b80607095256821 (which is a bit of a hack) to ensure that the correct repository was checked out. I've filed Bug 1408204 to fix this in a more principled way (which has a patch attached that I am testing in https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=bada39915381e77479cafbe95f90862639c7f182&selectedJob=136632647)
You need to log in before you can comment on or make changes to this bug.