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

RESOLVED FIXED in 6.2

Status

defect
--
blocker
RESOLVED FIXED
Last year
Last year

People

(Reporter: GeekShadow, Assigned: Fallen)

Tracking

(Blocks 1 bug)

Lightning 6.2
Dependency tree / graph

Details

Attachments

(3 attachments, 3 obsolete attachments)

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 ?
Tested on Ubuntu 16.04.4 x86_64, TB with fr locale
Blocks: tb60found
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.
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.
Flags: needinfo?(philipp)
@jorgk
Thanks for the info and the fix !
(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.
Will fix this by pushing bug 905097 and friends, which I am planning to uplift.
Status: NEW → RESOLVED
Closed: Last year
Flags: needinfo?(philipp)
Resolution: --- → DUPLICATE
Duplicate of bug: 905097
No, this issue was mostly about ReferenceError: __LOCATION__ is not defined and will be handled in the duplicate.
Duplicate of this bug: 1454435
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?
> I below out all my passwords

Meant to say I blew out all my passwords.
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.
Severity: normal → blocker
Status: RESOLVED → REOPENED
Flags: needinfo?(philipp)
Flags: needinfo?(makemyday)
Resolution: DUPLICATE → ---
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.
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.
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.
Assignee: nobody → philipp
Status: REOPENED → ASSIGNED
(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 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.
Attachment #8971811 - Flags: review?(makemyday)
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.
Attachment #8971811 - Flags: review?(Mozilla)
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>.
Flags: needinfo?(philipp)
Flags: needinfo?(makemyday)
Comment on attachment 8971811 [details] [diff] [review]
Partial fix - v1 [landed comment #25]

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

Looks good.
Attachment #8971811 - Flags: review?(makemyday)
Attachment #8971811 - Flags: review?(Mozilla)
Attachment #8971811 - Flags: review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/22aad28388dd
remove use of __LOCATION__ so Lightning can be an unpacked add-on. r=MakeMyDay
Status: ASSIGNED → RESOLVED
Closed: Last yearLast year
Resolution: --- → FIXED
Target Milestone: --- → 6.3
Attachment #8971811 - Flags: approval-calendar-beta?(philipp)
Pushed by mozilla@jorgk.com:
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
Posted patch Complete Fix - v1 (obsolete) β€” β€” Splinter Review
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.
Attachment #8972283 - Flags: review?(makemyday)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Posted patch Complete Fix - v2 (obsolete) β€” β€” Splinter Review
rebased version
Attachment #8972283 - Attachment is obsolete: true
Attachment #8972283 - Flags: review?(makemyday)
Attachment #8972289 - Flags: review?(makemyday)
Attachment #8971811 - Flags: approval-calendar-beta?(philipp) → approval-calendar-beta+
Attachment #8972289 - Flags: approval-calendar-beta+
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.
Flags: needinfo?(philipp)
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?
(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.
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.
Attachment #8971811 - Attachment description: Partial fix - v1 → Partial fix - v1 [landed comment #25]
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.
Flags: needinfo?(philipp)
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.
Posted patch Complete Fix - v3 (obsolete) β€” β€” Splinter Review
fixes some eslint errors
Attachment #8972289 - Attachment is obsolete: true
Attachment #8972289 - Flags: review?(makemyday)
Attachment #8973153 - Flags: review?(makemyday)
Attachment #8973153 - Flags: approval-calendar-beta+
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?
Attachment #8973153 - Flags: review?(makemyday) → review+
Green try. Could be settle this patch before the merge day tomorrow?
Oh boy, I was blind. That try has a few crashes in debug builds. However, opt is green. Strange.
Posted patch Complete Fix - v4 β€” β€” Splinter Review
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!
Attachment #8973153 - Attachment is obsolete: true
Attachment #8973562 - Flags: review?(makemyday)
The try run needs at least bug 1457087 applied on top to pass.
(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 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.
Attachment #8973562 - Flags: review?(makemyday) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/6a70e3cb03c7
Make icaljs backend loader work with packed extensions. r=MakeMyDay DONTBUILD
Status: REOPENED → RESOLVED
Closed: Last yearLast year
Resolution: --- → FIXED
Verified in beta build with libical and icaljs. Great!
Status: RESOLVED → VERIFIED
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
Attachment #8982599 - Flags: review+
Attachment #8982599 - Attachment description: Bug 1452120: Remove unpacked lightening extension on upgrade; r?Fallen → Bug 1452120: Remove unpacked lightening extension on upgrade; r=Fallen
Pushed by mozilla@hocat.ca:
https://hg.mozilla.org/comm-central/rev/fb94780cd594
Remove unpacked lightening extension on upgrade; r=Fallen
Status: VERIFIED → RESOLVED
Closed: Last yearLast year
You need to log in before you can comment on or make changes to this bug.