1630943 - Problem with two caldav calendars, on same host, but different usernames

Status: RESOLVED FIXED

(Calendar :: Provider: CalDAV, defect, P1)

Description

[Kai Engert (:KaiE:)]

The following works fine with Thunderbird 68, but fails with Thunderbird 77 nightly.

I'm using the simple "radicale" server.
I have a private calendar with private credentials.
I have a second, shared calendar, with shared credentials.

Adding the first calendar works fine, regardless which one I choose.

When adding the second calendar, the configuration is accepted, but there's NO prompt for the second username.
After creation, the second calendar is shown with a disabled state.
It can manually enabled, but it doesn't work.

Error console reports:
Lightning: [calCachedCalendar] replay action failed: <unknown>, uri={calendar-url}, result=2147500037, operation=[xpconnect wrapped calIOperation] 2 calCachedCalendar.js:346

I have created two test calendars that I can share privately with a developer who wants to work on this issue.

Comment 1

[Magnus Melin [:mkmelin]]

Any idea when it broke?

Comment 2

[Kai Engert (:KaiE:)]

(In reply to Magnus Melin [:mkmelin] from comment #1)

Any idea when it broke?

No. I could try to find out, by testing every beta version between 69 and 77.

Comment 3

[Kai Engert (:KaiE:)]

Philipp pointed me to the calendar.network.multirealm pref. If I set it to true, and recreate the calendars, it works fine.

All radicale calendar URL of a deployment are identical.

Apparently TB 68 didn't have an issue with them being identical.

Comment 4

[Kai Engert (:KaiE:)]

It broke in 77.

I used a profile created with tb 68, then started all beta versions after it.
All versions up to and including 76.0b1 still worked fine, they displayed both calendars.

But starting 77.0a1 (nightly) fails. The second calendar is grayed out / disabled, and its entries are no longer shown.

Comment 5

[Magnus Melin [:mkmelin]]

For bug 1546606 perhaps?

Comment 6

[Paul Morris [:pmorris]]

I took a look using the test calendars and was able to reproduce this and get a stack trace (below). I'm out of time today, but will try again on Monday. It does look like fallout from bug 1546606.

Edit: the following log is from doing a console.trace() right before the cal.ERROR("[calCachedCalendar] replay action failed: " in calCachedCalendar.js (which is the error Kai mentioned at the top of this bug).

calCachedCalendar.js:346:23
onResult resource:///components/calCachedCalendar.js:346
completeCheckServerInfo resource:///modules/CalDavCalendar.jsm:1872
checkDavResourceType resource:///modules/CalDavCalendar.jsm:1474
(Async: promise callback)
checkDavResourceType resource:///modules/CalDavCalendar.jsm:1458
replayChangesOn resource:///modules/CalDavCalendar.jsm:235
synchronize resource:///components/calCachedCalendar.js:363
downstreamRefresh resource:///components/calCachedCalendar.js:716
popItemQueue resource:///components/calCachedCalendar.js:616
onOperationComplete resource:///components/calCachedCalendar.js:658
notifyPureOperationComplete resource:///modules/calendar/utils/calProviderUtils.jsm:515
notifyOperationComplete resource:///modules/calendar/utils/calProviderUtils.jsm:534
getItems resource:///modules/CalStorageCalendar.jsm:838
playbackOfflineItems resource:///components/calCachedCalendar.js:663
popItemQueue resource:///components/calCachedCalendar.js:616
onOperationComplete resource:///components/calCachedCalendar.js:658
notifyPureOperationComplete resource:///modules/calendar/utils/calProviderUtils.jsm:515
notifyOperationComplete resource:///modules/calendar/utils/calProviderUtils.jsm:534
getItems resource:///modules/CalStorageCalendar.jsm:838
playbackOfflineItems resource:///components/calCachedCalendar.js:663
popItemQueue resource:///components/calCachedCalendar.js:616
onOperationComplete resource:///components/calCachedCalendar.js:658
notifyPureOperationComplete resource:///modules/calendar/utils/calProviderUtils.jsm:515
notifyOperationComplete resource:///modules/calendar/utils/calProviderUtils.jsm:534
getItems resource:///modules/CalStorageCalendar.jsm:838
playbackOfflineItems resource:///components/calCachedCalendar.js:663
refresh resource:///components/calCachedCalendar.js:707
registerCalendar resource:///modules/CalCalendarManager.jsm:265
doCreateCalendar chrome://calendar/content/calendarCreation.js:248
doCreateCalendar chrome://lightning/content/lightning-calendar-creation.js:20
doCreateCalendar chrome://lightning/content/caldav-lightning-calendar-creation.js:18
_fireEvent chrome://global/content/elements/wizard.js:483
advance chrome://global/content/elements/wizard.js:292
listeners chrome://global/content/elements/wizard.js:535
(Async: EventListener.handleEvent)
connectedCallback chrome://global/content/elements/wizard.js:544
(Async: LifecycleConnectedCallback)
<anonymous> chrome://global/content/elements/wizard.js:668
<anonymous> chrome://global/content/customElements.js:839
<anonymous> chrome://global/content/customElements.js:862
observe resource://gre/modules/CustomElementsListener.jsm:24

Comment 7

[Paul Morris [:pmorris]]

Attachment: part1-fix-multiple-caldav-calendars-0.patch

1 of 2. We were passing a caldav request object to prepHttpChannel when it needed a calendar object. Type checking would have come in handy here.

Comment 8

[Paul Morris [:pmorris]]

Attachment: part2-clean-up-comments-0.patch

2 of 2: Clean up some comments while we're here.

Comment 9

[Paul Morris [:pmorris]]

Attachment: part1-fix-multiple-caldav-calendars-1.patch

Part1 revised. There was another instance of the same problem.

Comment 10

[Geoff Lankow (:darktrojan)]

Comment on attachment 9142202 [details] [diff] [review]
part1-fix-multiple-caldav-calendars-1.patch

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

Does this mean the usage in CalICSCalendar.jsm is also wrong? A quick look makes me think it is. The jsdoc on prepHttpChannel could probably be improved too (at least mentioning calICalendar would be good).

Comment 11

[Paul Morris [:pmorris]]

Attachment: part1-fix-multiple-caldav-calendars-2.patch

(In reply to Geoff Lankow (:darktrojan) from comment #10)

Does this mean the usage in CalICSCalendar.jsm is also wrong? A quick look
makes me think it is.

Ah, good catch. I did look at that but mistook this as a CalICSCalendar when it was merely an httpHooks. Fixed.

The jsdoc on prepHttpChannel could probably be
improved too (at least mentioning calICalendar would be good).

I worked on it a bit. (I wish we documented our JSDoc conventions, particularly how to format long param descriptions that need to wrap. The JSDoc docs are no help on that. This was discussed some here with no conclusion reached. In this case I wanted to avoid the narrow column of text on the right so I went with the style that does that.)

Edit: try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=434c7510fdb453cb2ec3cb48313e53c0e50279ca

Comment 12

[Paul Morris [:pmorris]]

Attachment: part3-wip-get-tests-to-pass-0.patch

The try run turned up some failing tests. I've been working on getting them to pass, but the problem and solution are eluding me.

I've added some things to the gMockCalendar so more of it is mocked, and that solved a few errors.

The failures are related to server redirect not happening fully and/or correctly. Around line 90 of CalDavRequest.jsm, if you change the "this.calendar" back to "this" (which is not correct), then the tests pass. So I've used the Firefox remote debugger to step through the code, with both "this.calendar" and "this" there, and cannot find where things are going wrong. With this the asyncOnChannelRedirect function in CalDavRequest.jsm gets called (somewhere, somehow), whereas with this.calendar it does not.

It seems odd that changes involving the calendar associated with a channel are causing problems with redirects.

Philipp, can you take a look when you have a chance? I am stumped.

Comment 13

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 9143778 [details]
part3-wip-get-tests-to-pass-0.patch

.

Comment 14

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

In order for asyncOnChannelRedirect to be called, the network code will look at httpchannel.notificationCallbacks, QI that to nsIInterfaceRequestor, call .getInterface(Ci.nsIChannelEventSink) on that, and then call asyncOnChannelRedirect on the result.

The old code assumed that the calendar was always the notification callback, hence prepHttpChannel will first make it look like it needs a calendar, and then later pass it to httpchannel.notificationCallbacks. The new code refactors that a bit, so that CalDavRequestBase is supposed to be the notification callback, but the calendar is separate.

What you'll need to do is change calProviderUtils line 49 to not assume that it can be QI'd to a calICalendar. You could for example do let calendar = aNotifcationCallbacks.getInterface(Ci.calICalendar) instead. Then you need to actually pass this instead of this.calendar in the reset() method, so you are actually passing the nsIInteraceRequestor instead of the calICalendar.

Comment 15

[Paul Morris [:pmorris]]

Attachment: part1-clean-up-comments-1.patch

Thanks Philipp for that helpful explanation. It's working now, tests and all. Marking all previous patches obsolete and uploading new ones. This is the first of two.

Comment 16

[Paul Morris [:pmorris]]

Attachment: part2-fix-multiple-caldav-calendars-3.patch

2 of 2. This fixes the problem by bringing prepHttpChannel into line with the recently refactored caldav code. I've tested it manually with the test calendars Kai provided, and the caldav xpcshell tests pass locally.

Try run, just started: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=236daf552cbe3f4e2931f5129712b571f658b7e8

Aside: XPCOM remains more mysterious to me than I'd like. Here the difference between these two ways of getting a calICalendar from the aNotificationsCallbacks argument is something I'd like to understand better. Will have to look into it further.

Edit: Here's the difference: https://searchfox.org/mozilla-central/source/xpcom/base/nsIInterfaceRequestor.idl#10-18

Comment 17

[Geoff Lankow (:darktrojan)]

Comment on attachment 9149888 [details] [diff] [review]
part1-clean-up-comments-1.patch

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

::: calendar/providers/caldav/CalDavCalendar.jsm
@@ +1714,5 @@
>    },
>  
>    /**
>     * Checks the principals namespace for scheduling info. This function should
> +   * solely be called from findPrincipalNS

.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/601e259021fb
Clean up some comments. r=darktrojan
https://hg.mozilla.org/comm-central/rev/c827ec349a36
Fix multiple caldav calendars on same host with different usernames. r=darktrojan