1452120 - Installing a matching Lightning xpi version on top of Thunderbird beta doesn't work

Status: RESOLVED FIXED

(Calendar :: Lightning Only, defect)

Description

[Antoine Turmel [:GeekShadow]]

STR :
- Create a fresh profile of TB 60.0b2
- Remove Lightning from the extensions and restart
- Install Lightning XPI from https://ftp.mozilla.org/pub/calendar/lightning/candidates/6.2b2-candidates/build1/ and restart (same version number that comes integrated with TB 60.0b2)
- TB have some Lightning UI elements but clicking on them doesn't work
- Clicking on Calendar button gives you :
No such tab mode: calendar  tabmail.xml:467

How do you get back to the integrated Lightning on beta ?

Comment 1

[Antoine Turmel [:GeekShadow]]

Tested on Ubuntu 16.04.4 x86_64, TB with fr locale

Comment 2

[WaltS48 [:walts48]]

I can reproduce and confirm that after removing Lightning 6.2b2 from a fresh profile of 60.0b2, then reinstalling it breaks the calendar.

Clicking Events and Tasks > Calendar, doesn't open the Calendar tab, and "No such tab mode: calendar  tabmail.xml:467" appears in the error console.

Fixed it by creating a new profile.

Tested Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.0 en-US version on Ubuntu 16.04.4.

Comment 3

[Jorg K (CEST = GMT+2)]

OK, I installed TB 60 beta 2. Lightning was installed and I removed it.
Installed
https://archive.mozilla.org/pub/calendar/lightning/candidates/6.2b2-candidates/build1/win32/lightning-6.2b2.en-US.xpi

I get heaps of errors in the error console, the first one is:
ReferenceError: __LOCATION__ is not defined

The problem is that Lightning only works as an "unpacked" add-on, but mozilla60 doesn't allow "unpacked" add-ons any more, withdrawn in bug 1444502. When I tried making it "packed" in bug 1406499, hell broke loose, see bug 1406499 comment #12. So I backed out this change.

So what is happening is that when the user uninstalls the Lightning that was distributed and installs another matching one instead, this other one doesn't work.

Immediate fix: Remove the manually installed one, then reset the pref
extensions.installedDistroAddon.{e2fda1a4-762b-4020-b5ad-a41df1933103}
and restart TB.

The distributed add-on will then come back.

Philipp knows about this problem, he said on IRC on 2019-03-11:
22:03:40 - Fallen: meh, I see. looks like __LOCATION__ doesn't point right in packed xpis
22:04:48 - Fallen: heh, if all my calutils patches are pushed/reviewed then that code goes away :)

So it's up to the calendar people to fix that.

Comment 4

[Antoine Turmel [:GeekShadow]]

@jorgk
Thanks for the info and the fix !

Comment 5

[Arthur K. (he/him)]

(In reply to Jorg K (GMT+1) from comment #3)
> Immediate fix: Remove the manually installed one, then reset the pref
> extensions.installedDistroAddon.{e2fda1a4-762b-4020-b5ad-a41df1933103}
> and restart TB.

Thanks for this. Was driving me nucking futs.

Comment 6

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

Will fix this by pushing bug 905097 and friends, which I am planning to uplift.

Comment 7

[Worcester12345]

related?
https://bugzilla.mozilla.org/show_bug.cgi?id=673089

Comment 8

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

No, this issue was mostly about ReferenceError: __LOCATION__ is not defined and will be handled in the duplicate.

Comment 10

[Arthur K. (he/him)]

I was testing just now with TB 60.0b4 and Lightning 6.2b4/GData Provider 4.1b4 and there's something that got really busted. I can only run TB 60.0b4 using 6.2b3/GData Provider 4.1b3. Using Lightning 6.2b4/GData Provider 4.1b4 combo, Lightning repeatedly is asking me for a password and to Allow TB to manage the calendar. No matter how many times I choose Allow, it asks over and over again upon app start. I below out all my passwords, and still it persists. Should I open a new bug?

Comment 11

[Arthur K. (he/him)]

> I below out all my passwords

Meant to say I blew out all my passwords.

Comment 12

[Jorg K (CEST = GMT+2)]

Sadly this hasn't been fixed by the duplicate, the offending code is still there:
https://dxr.mozilla.org/comm-central/rev/8d636ad5810b5586433294c16cb2376d826fcca8/calendar/base/modules/calUtils.jsm#182
baseDir = __LOCATION__.parent.parent.clone();

Philipp, how do you intend to fix this? This is really a blocker for TB 60 ESR. I can't see myself releasing Thunderbird to millions of people where hundreds of thousands of those will have Lightning which originally came from AMO. All these installations will break. And it's quite tricky to revert back to the working distributed add-on in these cases.

Comment 13

[Stefan Sitter [:ssitter]]

Jorg, original Bug 1406499 about Lightning not working in Tb 60 and the '__LOCATION__ is not defined' error you observed while trying to fix it is still open.

Comment 14

[Jorg K (CEST = GMT+2)]

Right, but it was the duplicate of resolved bug 905097 which did NOT fix this. I don't know which way Philipp wants to play this, but it's a blocker.

Comment 15

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

Attachment: Partial fix - v1 [landed comment #25]

This should do part of the job. There are still a few other locations that assume files instead of xpis, one being the ical.js backend loader. It should work with libical though.

Comment 16

[Worcester12345]

(In reply to Jorg K (GMT+1) from comment #12)
> Sadly this hasn't been fixed by the duplicate, the offending code is still
> there:
> https://dxr.mozilla.org/comm-central/rev/
> 8d636ad5810b5586433294c16cb2376d826fcca8/calendar/base/modules/calUtils.
> jsm#182
> baseDir = __LOCATION__.parent.parent.clone();
> 
> Philipp, how do you intend to fix this? This is really a blocker for TB 60
> ESR. I can't see myself releasing Thunderbird to millions of people where
> hundreds of thousands of those will have Lightning which originally came
> from AMO. All these installations will break. And it's quite tricky to
> revert back to the working distributed add-on in these cases.

This was why I was trying to get the calendar code put directly into Thunderbird. Not only does it eliminate all the mismatches that happen, it also reduces the overall code size, since it doesn't need the add-on stuff any more. This would GREATLY streamline the application all around. As long as there is a way to turn it off for those who don't want or need it, this is the best way to go. Most everyone who uses an email client also uses a calendar, and for the few who don't, just turning it off or hiding it would be so much easier than all the mess of keeping versions matched up and always chasing your tail for the right code.

Comment 17

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

You've made that sufficiently clear in various bugs, but this bug is not the place for discussion in that direction. For Lightning to be integrated directly, there needs to be a way to hide the UI so that people who don't want it don't get bothered with it. We're not there yet, that is a project on its own. Even integrated, there will still be mismatches due to platform changes, which this is. To be clear, I think eventually we will get to a point where we can drop the extension part of Lightning, but we don't have the possibility to move in that direction at the moment.

Comment 18

[Jorg K (CEST = GMT+2)]

(In reply to Philipp Kewisch [:Fallen]  from comment #15)
> This should do part of the job. There are still a few other locations that
> assume files instead of xpis, one being the ical.js backend loader. It
> should work with libical though.
Thanks, Philipp. Could you please add the hunks from bug 1406499
https://hg.mozilla.org/comm-central/rev/6654b9108d7e#l1.12
https://hg.mozilla.org/comm-central/rev/6654b9108d7e#l2.12
and give this a try on comm-beta to make sure it works. Maybe disable ical.js if necessary.

Comment 19

[Jorg K (CEST = GMT+2)]

Here you go:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=baad966f89c36ffaed64a51f16881a0c400d47da

Comment 20

[Jorg K (CEST = GMT+2)]

Comment on attachment 8971811 [details] [diff] [review]
Partial fix - v1 [landed comment #25]

The try is green, so this seems to have worked. Philipp, could you organise a review for this asap. If MMD is not available, perhaps one of the other Calendar peers can review this.

Comment 21

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

Comment on attachment 8971811 [details] [diff] [review]
Partial fix - v1 [landed comment #25]

Asking Markus as well, whoever has time first. I'd really like to get the ical.js part fixed as well but it is turning out to be more complicated than I thought.

Comment 22

[Jorg K (CEST = GMT+2)]

But the Xpcshell tests all passed and they are run for both backends. So where's the problem? My try run also had the
-    <em:unpack>true</em:unpack>.

Comment 23

[[:MakeMyDay]]

Comment on attachment 8971811 [details] [diff] [review]
Partial fix - v1 [landed comment #25]

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

Looks good.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/22aad28388dd
remove use of __LOCATION__ so Lightning can be an unpacked add-on. r=MakeMyDay

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/07c5a80553e6
Backed out changeset 22aad28388dd for landing with an incorrect commit message. a=jorgk
https://hg.mozilla.org/comm-central/rev/11660815f2ec
remove use of __LOCATION__ so Lightning can be a packed add-on. r=MakeMyDay DONTBUILD

Comment 26

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

Attachment: Complete Fix - v1

This fixes the rest of it and also gets rid of loadingNSGetFactory to avoid the early calUtils import. I couldn't rebase it against latest comm-central, but the functions are going away so it should be an easy debitrot.

Comment 27

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

Attachment: Complete Fix - v2

rebased version

Comment 28

[Jorg K (CEST = GMT+2)]

v1 seems to apply to beta, v2 doesn't. So would v1 be the beta patch? In this case, un-obsolete it and put the beta+ there.

Comment 29

[Jorg K (CEST = GMT+2)]

Also, please reply to my question from comment #22: I have a green try with "Partial fix - v1". I understand that the Xpcshell tests run for both backends, so how come that worked without the second part?

Comment 30

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

(In reply to Jorg K (GMT+1) from comment #28)
> v1 seems to apply to beta, v2 doesn't. So would v1 be the beta patch? In
> this case, un-obsolete it and put the beta+ there.

Ideally, complete fix v2 applies on top of partial fix v1 for all branches. If this does not hold for beta then the rejections need to be resolved. What is also true is (complete fix v1 = partial fix v1 + complete fix v2). I'll see how it applies to comm-beta soon.

(In reply to Jorg K (GMT+1) from comment #29)
> Also, please reply to my question from comment #22: I have a green try with
> "Partial fix - v1". I understand that the Xpcshell tests run for both
> backends, so how come that worked without the second part?
I don't know.

Comment 31

[Jorg K (CEST = GMT+2)]

Sorry, as you said, "complete fix v2" needs to be applied on top of "partial fix v1". Since the latter isn't on beta yet, the former didn't apply. For the same reason, "complete fix v1" applied to beta. All good.

Comment 32

[Jorg K (CEST = GMT+2)]

Beta (TB 60 beta 6, Calendar 6.2):
https://hg.mozilla.org/releases/comm-beta/rev/7d57bd47deff7b7e96b9a8417bea2e5cba931d6c
Landed the "partial fix", I'll report back if that causes test failures.

Comment 33

[Jorg K (CEST = GMT+2)]

Without part 2, the push is green:
https://treeherder.mozilla.org/#/jobs?repo=comm-beta&revision=25ca94de86cfc28bc51177785f96864ce18ea323

I've installed the beta, removed Lightning, and installed the XPI from the same build. It appears to be working. Then I switched to icaljs, and it's indeed not working.

Comment 34

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

Attachment: Complete Fix - v3

fixes some eslint errors

Comment 35

[[:MakeMyDay]]

Comment on attachment 8973153 [details] [diff] [review]
Complete Fix - v3

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

Looks good, r+ with the comments checked.

::: calendar/base/backend/icaljs/calICALJSComponents.js
@@ +11,5 @@
> +        "resource://calendar/calendar-js/calDateTime.js",
> +        "resource://calendar/calendar-js/calICSService.js",
> +        "resource://calendar/calendar-js/calPeriod.js",
> +        "resource://calendar/calendar-js/calRecurrenceRule.js",
> +        "resource://calendar/calendar-js/calDuration.js",

Is there a reason why you moved calDuration to the end of the list?

@@ +20,5 @@
> +    }
> +
> +    let components = [
> +        calDuration, calDateTime, calIcalComponent, calIcalProperty, calICSService, calPeriod,
> +        calRecurrenceRule

Same question here - is the change of the order for calDuration intended?

Comment 36

[Jorg K (CEST = GMT+2)]

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

Comment 37

[Jorg K (CEST = GMT+2)]

Green try. Could be settle this patch before the merge day tomorrow?

Comment 38

[Jorg K (CEST = GMT+2)]

Oh boy, I was blind. That try has a few crashes in debug builds. However, opt is green. Strange.

Comment 39

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

Attachment: Complete Fix - v4

This will most likely fix debug. The problem was that calUtils.jsm and the backend loader getServices() was called recursively.

In comparison to before, when using a manifest file the actual js is only loaded when used. I've done this now with a lazy factory, that only calls NSGetFactory when createInstance is called. NSGetFactory will in turn load sub-scripts in calICALJSComponents.js, which include calUtils.jsm again, but at that time the initial getService() for the loader will have completed.

This needs a try, but I am about to crash. Jörg, if you have time maybe you could do this for me. Thanks in advance!

Comment 40

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

The try run needs at least bug 1457087 applied on top to pass.

Comment 41

[Jorg K (CEST = GMT+2)]

(In reply to Philipp Kewisch [:Fallen]  from comment #40)
> The try run needs at least bug 1457087 applied on top to pass.
Really? I'll try it on beta, there Xpcshell and Mozmill still work:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=faff372b5c13e666328310d2dd705f01ce280f79

If I understand it correctly, bug 1457087 brings its own set of (opt) failures, see previous trunk try here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f87978efb7b891b17c94310ed1305840767861f9&selectedJob=177177607

Comment 42

[[:MakeMyDay]]

Comment on attachment 8973562 [details] [diff] [review]
Complete Fix - v4

Looks good and since the beta try push is green, it seems to work as expected.

But please make another try push to cc to check whether a change is required in the xpcshell test patch or the factory fix resolves the failures from the other try push as well.

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6a70e3cb03c7
Make icaljs backend loader work with packed extensions. r=MakeMyDay DONTBUILD

Comment 44

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-beta/rev/1521f2ce1de5b61f08b94b746ac2970110c28c70

Comment 45

[Jorg K (CEST = GMT+2)]

Verified in beta build with libical and icaljs. Great!

Comment 46

[Phabricator Automation]

Attachment: Bug 1452120: Remove unpacked lightening extension on upgrade; r=Fallen

Comment 47

[Phabricator Automation]

Comment on attachment 8982599 [details]
Bug 1452120: Remove unpacked lightening extension on upgrade; r=Fallen

Philipp Kewisch [:Fallen]  has approved the revision.

https://phabricator.services.mozilla.com/D1491

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/fb94780cd594
Remove unpacked lightening extension on upgrade; r=Fallen

Comment 49

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-beta/rev/dba10915015b3d65f4cb3b62eb6acea740803542 (BETA_60_CONTINUATION)