1466430 - Massive calendar Xpcshell test failure on comm-beta

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

Everything was find on comm-beta until Sun, May 27, 2018, 14:44:21:
https://treeherder.mozilla.org/#/jobs?repo=comm-beta&revision=a68f6d0a02bba53301db19ea5fdb895a49334b0f

Failures started on Wed, May 30, 2018 18:12:26:
https://treeherder.mozilla.org/#/jobs?repo=comm-beta&revision=86ed509674fc45880be2fe101e67146d4eec3ba3

when Tom uplifted a bunch of TaskCluster fixes, amongst them:
7cfa96f86122 Tom Prince - Bug 1439469: Package lightning directly as part of thunderbird

I'm running the beta version now and Calendar works fine, so that appears to be "only" a failure in the test setup. Failures are:
TEST-UNEXPECTED-FAIL | xpcshell-icaljs.ini:comm/calendar/test/unit/test_alarm.js | xpcshell return code: 0
TypeError: Components.classes["@mozilla.org/calendar/backend-loader;1"] is undefined at /Users/cltbld/tasks/task_1527918061/build/tests/xpcshell/tests/comm/calendar/test/unit/head_consts.js:31
TypeError: Ci.calIAlarm is undefined at /Users/cltbld/tasks/task_1527918061/build/tests/xpcshell/tests/comm/calendar/test/unit/test_alarm.js:368
ReferenceError: cal is not defined at /Users/cltbld/tasks/task_1527918061/build/tests/xpcshell/tests/comm/calendar/test/unit/test_alarm.js:7
TEST-UNEXPECTED-FAIL | xpcshell-icaljs.ini:comm/calendar/test/unit/test_alarmutils.js | xpcshell return code: 0
TypeError: Components.classes["@mozilla.org/calendar/backend-loader;1"] is undefined at /Users/cltbld/tasks/task_1527918061/build/tests/xpcshell/tests/comm/calendar/test/unit/head_consts.js:31
TypeError: Components.classes['@mozilla.org/calendar/startup-service;1'] is undefined at /Users/cltbld/tasks/task_1527918061/build/tests/xpcshell/tests/comm/calendar/test/unit/head_consts.js:280
TEST-UNEXPECTED-FAIL | xpcshell-icaljs.ini:comm/calendar/test/unit/test_attachment.js | xpcshell return code: 0
TypeError: Components.classes["@mozilla.org/calendar/backend-loader;1"] is undefined at /Users/cltbld/tasks/task_1527918061/build/tests/xpcshell/tests/comm/calendar/test/unit/head_consts.js:31
ReferenceError: cal is not defined at /Users/cltbld/tasks/task_1527918061/build/tests/xpcshell/tests/comm/calendar/test/unit/test_attachment.js:90
TEST-UNEXPECTED-FAIL | xpcshell-icaljs.ini:comm/calendar/test/unit/test_alarmservice.js | xpcshell return code: 0

... and many more.

Note that comm-beta is build the THUNDERBIRD_60_VERBRANCH on M-B, so there is no chance that something changed in the underlying Mozilla code since the branch hasn't been updated for a while.

Philipp mentioned the he had seen these failures before, and that is correct: Bug 1457087 comment #31. Very similar failures were seen on
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4e9482e4eac13e8d0a1441530ccae4eb7a02e2b2&selectedJob=177224614
with the patches from bug 1452120 and bug 1457087 applied. Bug 1452120 is on comm-beta:
https://hg.mozilla.org/releases/comm-beta/rev/1521f2ce1de5b61f08b94b746ac2970110c28c70

Comment 1

[Jorg K (CEST = GMT+2)]

So basically, the patch from bug 1457087 makes trunk work again which immediately exposes this failure, the one we have on beta. Note bug 1457087 comment #32 and the one after.

Comment 2

[Jorg K (CEST = GMT+2)]

Hmm, what I didn't know until now is that Tom also pushed a whole lot of stuff to THUNDERBIRD_60_VERBRANCH on M-B on May 30th:
https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?changeset=03ca12fd5db6

So I think there is a build gremlin in the box.

Comment 3

[Geoff Lankow (:darktrojan)]

So many things going on here:

1. The tests are looking for the extension in the wrong place. Easy to fix.
2. For some reason the line "resource calendar ." is now in chrome/calendar.manifest instead of chrome.manifest.
3. calbase.xpt and calbaseinternal.xpt aren't being created, or referenced from a manifest.
4. Some tests are still failing. Probably not related, but still bad.

Comment 4

[Geoff Lankow (:darktrojan)]

I'm not actually sure the .xpt files are still supposed to exist, or if they go into Thunderbird's interfaces.xpt, but in any case they're not referenced anywhere.

Comment 5

[Jorg K (CEST = GMT+2)]

Oh, we're getting the fix on a Sunday now ;-)

(In reply to Geoff Lankow (:darktrojan) from comment #3)
> 4. Some tests are still failing. Probably not related, but still bad.
Not sure what you're referring to but
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e14b36331ebd9bdf5dff1860c9edb385571dffa3
still appears like having the same failures we see on the beta tree on BETA_60_CONTINUATION, unless I'm missing something.

(In reply to Geoff Lankow (:darktrojan) from comment #4)
> I'm not actually sure the .xpt files are still supposed to exist, ...
We stopped packaging XPT files in bug 1448121 in TB 61, that's all I know. That shouldn't affect TB 60 or Calendar 6.2.

Comment 6

[Jorg K (CEST = GMT+2)]

As you know, ideally we produce patches for trunk and then backport/uplift them. Since the beta failures seem to look the same as the trunk failures with the patch from bug 1457087 applied, perhaps we could kill two birds with one stone:
Fix trunk with the patch from bug 1457087 and some additional patch, and then uplift that additional patch to beta.

That's of course just all wishful thinking from someone who has no idea (me), so just ignore me if it's nonsense ;-)

Comment 7

[Geoff Lankow (:darktrojan)]

(In reply to Jorg K (GMT+2) from comment #5)
> Oh, we're getting the fix on a Sunday now ;-)

Nope, because it's already Monday here and I've discovered comment #3 was mostly wrong. 1 isn't an easy fix, and 2 and 3 don't appear to be actual problems.

It's too late for this, I'm going to bed.

Comment 8

[Geoff Lankow (:darktrojan)]

If we unpack the .xpi file (once we've looked in the right place for it), everything works just fine! If I could load the packed manifest with javascript, I'd do that, but there's no good way (that I know of).

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a6270b4a83f1495fdb1d7e11e7f781eaaccd5b0a

Comment 9

[Jorg K (CEST = GMT+2)]

Hmm, "unpack the .xpi file". I thought the whole idea was to *not* unpack it, see bug 1452120 and bug 1406499. That was due to the fact that unpacking stopped working in bug 1444502 in mozilla60.

As per comment #0, beta 60 was fine until Sun, May 27, 14:44:21.

Note that bug 1452120 had three parts, landed on beta here:
https://hg.mozilla.org/releases/comm-beta/rev/7d57bd47deff7b7e96b9a8417bea2e5cba931d6c (bug 1452120 comment #33, May 2nd)
https://hg.mozilla.org/releases/comm-beta/rev/1521f2ce1de5b61f08b94b746ac2970110c28c70 (bug 1452120 comment #44, May 7th)
https://hg.mozilla.org/releases/comm-beta/rev/dba10915015b3d65f4cb3b62eb6acea740803542 (bug 1452120 comment #49, June 4 when it was already broken).

Comment 10

[Jorg K (CEST = GMT+2)]

So it's unpacking only for testing purposes, right? What I'd like to understand is that running the beta I don't see a calendar failure, so what's special about testing?

Comment 11

[Geoff Lankow (:darktrojan)]

I'm unpacking it to run the test, yes. The problem is I can't register the manifest inside a packed file from JS. There's a patch in another bug that will enable us to run the test without unpacking, but that hasn't landed, and I don't want to bring it forward just for this.

Comment 12

[Jorg K (CEST = GMT+2)]

(In reply to Geoff Lankow (:darktrojan) from comment #11)
> There's a patch in another bug that
> will enable us to run the test without unpacking, but that hasn't landed,
> and I don't want to bring it forward just for this.
Please give some details. Which bug/attachment. And why not move that forward? That's the way we usually work, no?

Also, do we understand why it stopped working on the beta where it was originally working?

Comment 13

[Geoff Lankow (:darktrojan)]

(In reply to Jorg K (GMT+2) from comment #12)
> (In reply to Geoff Lankow (:darktrojan) from comment #11)
> > There's a patch in another bug that
> > will enable us to run the test without unpacking, but that hasn't landed,
> > and I don't want to bring it forward just for this.
> Please give some details. Which bug/attachment. And why not move that
> forward? That's the way we usually work, no?

Bug 1448808. The code needed is here: https://bugzilla.mozilla.org/attachment.cgi?id=8985867&action=diff#a/common/src/nsComponentManagerExtra.cpp_sec2

On a closer look (now that I've done the review), I'm not totally opposed to copying the required parts to beta, but I do think it is a bit much so close to shipping, and for little gain (passing tests that we already know pass).

> Also, do we understand why it stopped working on the beta where it was
> originally working?

If I'm reading it right, the automation used to create an unpacked version from the packed version, which then got included in target.common.tests.zip and downloaded onto the test machine. This stopped happening here: https://hg.mozilla.org/releases/comm-beta/rev/7cfa96f86122#l3.1 . We could reverse that change, but I think it would be better to use what actually ships, even if we have to unpack it on the test machine (which we'll only have to do for this version, assuming bug 1448808 lands in the next version, and we fix up the changes we make in this bug).

Comment 14

[Jorg K (CEST = GMT+2)]

Thanks for the explanation. Let's do whatever you deem fit then :-)

Can https://hg.mozilla.org/try-comm-central/rev/13592fc81a105afaa6e688aea97b9657d4426d20 be landed on beta? Once it's unbusted again, that is. I'm happy to rs (rubber-stamp). We also need a/that fix for the ESR.

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: 1466430-unpack-lightning-for-xpcshell-test.patch

I scraped this of Geoff's try run from comment #8.

Comment 16

[Jorg K (CEST = GMT+2)]

s/of/off/ :-(

Comment 17

[Jorg K (CEST = GMT+2)]

(In reply to Geoff Lankow (:darktrojan) from comment #8)
> If we unpack the .xpi file (once we've looked in the right place for it),
> everything works just fine! If I could load the packed manifest with
> javascript, I'd do that, but there's no good way (that I know of).

Wouldn't this do the trick?
https://dxr.mozilla.org/comm-central/source/common/src/extensionSupport.jsm#82

Comment 18

[Geoff Lankow (:darktrojan)]

(In reply to Jorg K (GMT+2) from comment #17)
> Wouldn't this do the trick?
> https://dxr.mozilla.org/comm-central/source/common/src/extensionSupport.
> jsm#82

You're mean to use that code on chrome.manifest, right?

That would get us the contents of the file, but we need an nsIFile to pass to the components manager. Plus we need to have all the other files in the right place relative to it.

Comment 19

[Jorg K (CEST = GMT+2)]

Hmm, then I misunderstood the "If I could load the packed manifest with javascript, I'd do that ...".

So any objection to landing your attached patch?

Comment 20

[Geoff Lankow (:darktrojan)]

Go ahead. It could land on central too, according to my try push earlier.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=1b991132423818f642a3c6728878af24187a0c16

Comment 21

[Jorg K (CEST = GMT+2)]

(In reply to Geoff Lankow (:darktrojan) from comment #20)
> Go ahead. It could land on central too, according to my try push earlier.
Thanks, will do with the next push with other fixes. As for landing on C-C: Since it doesn't fix Calendar completely and there are many bugs in progress right now with a different approach (see comment #13), we can delays this (or not do it at all).

Comment 22

[Jorg K (CEST = GMT+2)]

TB 60 beta 8 (BETA_60_CONTINUATION branch):
https://hg.mozilla.org/releases/comm-beta/rev/53aabc8387a818836387082cf50aebf1ed72cc26
Unpack Lightning for xpcshell tests. rs=jorgk,bustage-fix a=jorgk

The other thing I wanted to land got r+, so I pushed both. Let's hope Philipp agrees without his review and uplift approval ;-)

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1c4c481c3a1d
Unpack Lightning for xpcshell tests. rs=jorgk,bustage-fix

Comment 24

[Jorg K (CEST = GMT+2)]

Comment on attachment 8986040 [details] [diff] [review]
1466430-unpack-lightning-for-xpcshell-test.patch

I landed this to fix the failures on C-B and get tests going again on C-C. I hope you approve.

Geoff is working at top-speed and it's hard to keep up with him ;-)

Comment 25

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8986040 [details] [diff] [review]
1466430-unpack-lightning-for-xpcshell-test.patch

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

This seems ok for me since it works, a few minor nits.

Generally I would have used the component loader from bug 1448808, or used the build system to unpack the extension at the right time (e.g. once before tests are run, or while packaging the tests).

As mentioned earlier, unpacked extensions are removed in bug 1444502 so the fact that this works is lucky :)

::: calendar/test/unit/head_consts.js
@@ +32,5 @@
>      bindir.append("{e2fda1a4-762b-4020-b5ad-a41df1933103}");
> +    if (!bindir.exists()) {
> +        const ZipReader = Components.Constructor("@mozilla.org/libjar/zip-reader;1", "nsIZipReader", "open");
> +
> +        let zip = ZipReader(xpiFile);

Any specific reason to go with the constructor instead of the more common Cc[...].createInstance() and open() call here?

@@ +34,5 @@
> +        const ZipReader = Components.Constructor("@mozilla.org/libjar/zip-reader;1", "nsIZipReader", "open");
> +
> +        let zip = ZipReader(xpiFile);
> +        let entries = zip.findEntries(null);
> +        while (entries.hasMore()) {

Could use fixIterator here

Comment 26

[Jorg K (CEST = GMT+2)]

(In reply to Philipp Kewisch [:Fallen]  from comment #25)
> Generally I would have used the component loader from bug 1448808, or used
> the build system to unpack the extension at the right time (e.g. once before
> tests are run, or while packaging the tests).
Yes, Geoff suggested that in comment #13, but then we would have had to pull that patch apart.

> > +        const ZipReader = Components.Constructor("@mozilla.org/libjar/zip-reader;1", "nsIZipReader", "open");
> Any specific reason to go with the constructor instead of the more common
> Cc[...].createInstance() and open() call here?
I asked myself the same, seems to be copied from one of these:
https://searchfox.org/mozilla-central/search?q=Components.Constructor(%22%40mozilla.org%2Flibjar%2Fzip-reader%3B1%22&case=false&regexp=false&path=

Comment 27

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to Jorg K (GMT+2) from comment #26)
> (In reply to Philipp Kewisch [:Fallen]  from comment #25)
> > Generally I would have used the component loader from bug 1448808, or used
> > the build system to unpack the extension at the right time (e.g. once before
> > tests are run, or while packaging the tests).
> Yes, Geoff suggested that in comment #13, but then we would have had to pull
> that patch apart.
I don't mind that, the component manager extra is fairly self-contained.

Comment 28

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-esr60/rev/e2db01fa27ee