1701296 - Do not throw exception on empty calendar-home-set

Status: RESOLVED FIXED

(Calendar :: Provider: CalDAV, defect)

Description

[Дилян Палаузов]

Attachment: Do not throw exception on empty calendar-home-set

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:86.0) Gecko/20100101 Firefox/86.0

Steps to reproduce:

In Thunderbird 88.0b1 I add a new calendar. I do not enter username, as Location type aaa.bapha.be, click ”This location doesn’t require credentials” and “Offline support”.

As work-around of bug 1599602 the server returns two calendar-home-sets, in order to serve TB ESR 78 with TbSync. The second calendar-home-set is empty.

CalDavProvider.handleHomeSet(location) returns in this case null. This leads in handlePrincipal() the execution of
calendars.push(...(await this.handleHomeSet(homeSetUrl)));
which is equivalent to
calendars.push(...null)
this throws exception back to detectCalendars().

The provided patch enhances Thunderbird on „Create New Calendar” → “On the Network” → No username, Location aaa.bapha.be, [YES] This location doesn’t require credentials → “Find Calendars” to find the calendars.

Comment 1

[Martin Schröder [:mschroeder]]

I would propose to change the one caller of handleHomeSet, which destructs the array using the spread operator, to treat a return value of null as an empty array. This way the method signature stays as-is and conforms, in terms of the return value, to most of the other functions in this module.

Comment 2

[Magnus Melin [:mkmelin]]

Thanks for the patch. I agree it could be better to do what Martin proposes in comment 1.

Can you provide a real hg patch (not to .orig)? Or even better, set up phabricator and submit through that:

./mach install-moz-phab

See https://github.com/mozilla-conduit/review and https://moz-conduit.readthedocs.io/en/latest/mozphab-linux.html

Comment 3

[Дилян Палаузов]

There are two callers of handleHomeSet within CalDAVProvider.jsm. I do not understand the implications of changing both callers, so I am not going to change them.

This PR shows a use-case, where the WebDAV discovery cannot deal correctly with edge cases, and proposes one-line code correction, that fixes it.

I have not compiled for this Thunderbird, but unpacked omni.ja, changed the code, repacked omni.ja, and then called thunderbird --purgecaches. I do not use Mercurial.

Having to setup now Phabricator and so on, to get a one line change, is disproportional amount of work.

Comment 4

[Magnus Melin [:mkmelin]]

FWIW, if you select to use artifact builds, the building step is very fast. I think basically you'd be saving also your own time after a one-time 10-30min time investment to get set up. We're hoping for many more patches going forwards ;)

See https://developer.thunderbird.net/thunderbird-development/getting-started

Comment 5

[Дилян Палаузов]

Attachment: tb1701296-v2.patch

This proposed patch considers the suggestion to keep CalDavDetector.handeHomeSet unchanged, and instead to interpret the result null of the method in 50% of its invocation as an empty array.

Comment 6

[Дилян Палаузов]

The line // If one homeSet fails, it's likely they all will, so don't handle failures here. after the changed line should perhaps be removed. handleHomeSets() either throws exception, returns an array or returns null. If it returns null, it is interpreted as empty array, not as failure. If it throws an exception, it is rethrown by CalDavDetector.handlePrincipal. So there are no means to get the case “one homeset has failed“ there.

Comment 7

[Geoff Lankow (:darktrojan)]

Comment on attachment 9214641 [details] [diff] [review]
tb1701296-v2.patch

This is really ugly. Please put the result in a variable and if it's truthy add it to calendars. You can go ahead and remove that comment too.

Comment 8

[Дилян Палаузов]

Attachment: It is valid for a calendar-home-set to contain no calendars

This patch puts the result in a variable and if it is truthy, adds it to the calendars array.

Comment 9

[Дилян Палаузов]

The discovery fails for TB88b3 in prepareNetworkCalendar():if (!calendar.getProperty("cache.always")) with TypeError: calendar.getProperty is not a function.

Comment 10

[Geoff Lankow (:darktrojan)]

You forgot the ... in the push call.

Comment 11

[Дилян Палаузов]

Attachment: Calendars-home-sets containing no calendars are valid

Comment 12

[Geoff Lankow (:darktrojan)]

NI'ing myself so I remember to turn this into a real patch and land it.

Comment 13

[Geoff Lankow (:darktrojan)]

Attachment: 1701296-empty-home-set.diff

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0fc19ef345dd
Do not throw exception on empty calendar-home-set. r=darktrojan