605378 - Fail to parse supported-calendar-component-set results from Chandler PROPFIND

Status: RESOLVED FIXED

(Calendar :: Provider: CalDAV, defect)

Description

[glen.a.ritchie]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9.2.10) Gecko/20100914 Firefox/3.6.10
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9.2.9) Gecko/20100915 Lightning/1.0b3pre Thunderbird/3.1.4

Recently I had issues after the upgrade to lightning 1.02b and Thunderbird 3.1.4 which broke CalDAV calendar support while using Chandler(Cosmos) server. 

Specifically I was unable to create events on the calendar, as the calender was simply not listed in the available calendars when trying to create an event.





Reproducible: Always

Steps to Reproduce:
1. Add network calendar from Chandler CalDAV
2. Attempt to create event on new calendar

Actual Results:  
Unable to create event on the new calendar

Expected Results:  
Should be able to create events on the calendar

- Chandler Server 1.1.0.

While investigating the cause, I determined that the CalDAV module was not parsing the PROPFIND results correctly. 

Chandler was returning(among others) XML like the following string:
<C:comp C:name="VEVENT" xmlns:C="urn:ietf:params:xml:ns:caldav" xmlns:D="DAV:"/>

The code in calDavCalendar.js attempts to parse ( http://mxr.mozilla.org/comm-1.9.2/source/calendar/providers/caldav/calDavCalendar.js#1388 ) the XML but fails to retrieve the "name" attribute due to the attribute having a namespace associated with it and as a result, believes the server is unable to accept events.

Changing the line 1393:
let comp = [e-mail]();
to
let comp = sc.@*::name.toString();

Fixes the issue by allowing it to retrieve the "name" attribute no matter what the namespace.

I freely admit my lack of understanding of XML and namespaces, so please feel free to make a better fix instead of my workaround.

Comment 1

[Simon Vaillancourt]

That response returned by Chandler seems OK, even if I am not sure that the attributes really "inherits" the CALDAV: namespace from the xml element "C:comp"

Chandler server would probably have much better interoperability if they tried to match their responses with the examples in rfc 4791 (i.e.: never specify the namespace on attributes). 

That being said I agree with the filer's recommended fix.

Comment 3

[Stefan Fleiter (:sfleiter)]

I repeat the most important points from duplicate bug 707231 I created:

The XML Chandler/Cosmo does create is right.
At least according to "xmllint --debug" which dumps a debug tree of the in-memory 
document the C namespace prefix for the attribute is valid but redundant, so both 
kinds of writing the "name" attribute should work.

If sc is in the namespace C, shouldn't sc.@name match
to name attributes in namespace C?
C:name and name are equivalent so the parsing code should handle that.
My guess is that
   sc.@*::name.toString();
matches to name attributes in any namespace which would be wrong.

Where do I find the documentation to the XML parsing done here:
   let comp = [e-mail]();
?
I would very much like to understand that code.
Thanks in advance.

Comment 4

[Philipp Kewisch [:Fallen][☁️📆]]

The [e-mail]() thing is the javascript xml extensions called "e4x".

You could try setting:

default xml namespace = C

just under the definition of the namespace in that function. I assume that its not using the right default namespace. If that doesn't work, instead of accepting any namespace, I'd suggest using sc.@C::name because the attribute should be from the CalDAV namespace and not from some other namespace.

Stefan (Fleiter) I'd appreciate if you could put together a patch, as you seem to have a good grip of the code! A patch for that file would be a start, ideally you could clone http://hg.mozilla.org/comm-central and create a hg diff --git there.

Comment 5

[Stefan Fleiter (:sfleiter)]

Thank you Philipp for your support, the word e4x was a good starting point.
This is very much appreciated.

I tried what you suggested, here the results:

- Declaring the default namespace does not change anything:
  http://jsfiddle.net/Stefan_Fleiter/9VeXS/

- using sc.@C::name does not match on attributes which do not have
  the namespace declared themselves which is quite a surprise.
  http://jsfiddle.net/Stefan_Fleiter/9VeXS/2/

I will continue with this as time permits.

Comment 6

[Stefan Fleiter (:sfleiter)]

I had a look at the XML Namespace spec:
According to
http://www.w3.org/TR/xml-names/#defaulting
"The namespace name for an unprefixed attribute name always has no value."
That means that an attribute does *not* inherit the namespace of its element.

A good description of this fact and the reasons for it can be found here:
http://www.xmlplease.com/attributexmlns

So the output of "xmllint --debug" was misleading.

Comment 7

[Stefan Fleiter (:sfleiter)]

So that is a bug in cosmo/chandler and *not* in Lightning.

As tested now the cosmo/chandler cloud instance hub.chandlerproject.org does
not have this bug anymore.

The latest source release of cosmo still has the problem, though.

See
http://websvn.osafoundation.org/filedetails.php?repname=server&path=%2Fcosmo%2Ftrunk%2Fcosmo%2Fsrc%2Fmain%2Fjava%2Forg%2Fosaf%2Fcosmo%2Fdav%2Fcaldav%2Fproperty%2FSupportedCalendarComponentSet.java
and especially the code
DomUtil.setAttribute(e, ATTR_CALDAV_NAME,  NAMESPACE_CALDAV, type);

Since cosmo, the ClaDAV server of chandler, is a dead project I do not
think that this will be fixed in the source in the future.


I do not know how many companies/organizations have cosmo servers running
or will setup new ones and whether a workaround should be implemented.

If no workaround is wanted this should be closed as INVALID.
If a workaround would be accepted I could create one.

Comment 8

[Stefan Fleiter (:sfleiter)]

Sorry for bugspam, last comment for today:

A fix could look like the fallback logic here:
http://jsfiddle.net/Stefan_Fleiter/9VeXS/3/

Comment 9

[Philipp Kewisch [:Fallen][☁️📆]]

Ok, since @C::name doesn't work I think its ok to just workaround using @*::name, adding a comment that this is needed for cosmo. Are there any other places you know of where cosmo adds a namespace to the attribute? If so we should add workarounds there too.

Thank you for your detailed analysis

Comment 10

[Stefan Fleiter (:sfleiter)]

That would be 
http://jsfiddle.net/Stefan_Fleiter/9VeXS/4/
Especially interesting is the FOO_BAR calendar component type. ;-)

No I do not know (yet) any other place where cosmo uses a namespaced
attribute.
If I find one I will report it here.

Comment 11

[Philipp Kewisch [:Fallen][☁️📆]]

Yep, lets go with that version. Could you attach a patch to this bug and ask me for review?

Comment 12

[Stefan Fleiter (:sfleiter)]

Will do that during the next days.

Comment 13

[Stefan Fleiter (:sfleiter)]

Attachment: workaround for wrong namespace of name attribute in comp element of supported-calendar-component-set reponse

Comment 14

[Stefan Fleiter (:sfleiter)]

This patch does not help until bug 588799 is fixed, too.
So that should be next. :-)

Comment 15

[Stefan Fleiter (:sfleiter)]

Comment on attachment 581066 [details] [diff] [review]
workaround for wrong namespace of name attribute in comp element of supported-calendar-component-set reponse

Having slept a night over this the comment is quite misleading, sorry.
I will generate a better patch when I am at my private computer again.

Comment 16

[Stefan Fleiter (:sfleiter)]

Attachment: workaround for wrong namespace of name attribute in comp element of supported-calendar-component-set reponse (better comment)

This time with better (not misleading) comment.

Comment 17

[Philipp Kewisch [:Fallen][☁️📆]]

Comment on attachment 581352 [details] [diff] [review]
workaround for wrong namespace of name attribute in comp element of supported-calendar-component-set reponse (better comment)

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

r=philipp and approval for comm-aurora since its low risk

::: calendar/providers/caldav/calDavCalendar.js
@@ +1609,5 @@
>              if (supportedComponentsXml.C::comp.length() > 0) {
>                  thisCalendar.mSupportedItemTypes.length = 0;
>                  for each (let sc in supportedComponentsXml.C::comp) {
> +                    // accept name attribute from all namespaces to workaround Cosmo bug
> +                    // see https://bugzilla.mozilla.org/show_bug.cgi?id=605378#c6

I think it sufficient to write bug 605378 comment 6, as we've done this the same in other places. I'll fix this before checkin.

Comment 18

[Philipp Kewisch [:Fallen][☁️📆]]

Pushed to:

comm-central changeset 55b341cdcf82
releases/comm-aurora changeset 0274ebc20d7f