Closed Bug 1701296 Opened 8 months ago Closed 7 months ago

Do not throw exception on empty calendar-home-set

Categories

(Calendar :: Provider: CalDAV, defect)

Thunderbird 88
defect

Tracking

(thunderbird_esr78 unaffected)

RESOLVED FIXED
90 Branch
Tracking Status
thunderbird_esr78 --- unaffected

People

(Reporter: dpa-mozilla, Assigned: dpa-mozilla)

References

(Regression)

Details

Attachments

(2 files, 3 obsolete files)

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.

Regressed by: 1670415

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.

Status: UNCONFIRMED → NEW
Ever confirmed: true

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

Assignee: nobody → dpa-mozilla
Status: NEW → ASSIGNED

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.

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

Attached patch tb1701296-v2.patch (obsolete) — Splinter Review

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.

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.

Attachment #9211862 - Attachment is obsolete: true
Attachment #9214641 - Flags: review?(mkmelin+mozilla)
Attachment #9214641 - Flags: review?(mkmelin+mozilla) → review?(geoff)

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.

Attachment #9214641 - Flags: review?(geoff) → review-

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

Attachment #9216985 - Flags: review?(geoff)

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

Attachment #9214641 - Attachment is obsolete: true

You forgot the ... in the push call.

Attachment #9216985 - Attachment is obsolete: true
Attachment #9216985 - Flags: review?(geoff)
Attachment #9216988 - Attachment description: Calendare-home-sets containing no calendars are valid → Calendars-home-sets containing no calendars are valid
Attachment #9216988 - Flags: review?(geoff) → review+

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

Flags: needinfo?(geoff)
Flags: needinfo?(geoff)
Attachment #9219423 - Flags: review+
Target Milestone: --- → 90 Branch

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/0fc19ef345dd
Do not throw exception on empty calendar-home-set. r=darktrojan

Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.