Closed Bug 605378 Opened 14 years ago Closed 12 years ago

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

Categories

(Calendar :: Provider: CalDAV, defect)

Lightning 1.0b2
defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: glen.a.ritchie, Assigned: sfleiter)

References

Details

(Whiteboard: [CalDAV server: Chandler])

Attachments

(1 file, 1 obsolete file)

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 = sc.@name.toString();
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.
Version: unspecified → Lightning 1.0b2
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [CalDAV server: Chandler]
OS: Windows XP → All
Hardware: x86 → All
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 = sc.@name.toString();
?
I would very much like to understand that code.
Thanks in advance.
The sc.@name.toString() 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.
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.
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.
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.
Sorry for bugspam, last comment for today:

A fix could look like the fallback logic here:
http://jsfiddle.net/Stefan_Fleiter/9VeXS/3/
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
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.
Yep, lets go with that version. Could you attach a patch to this bug and ask me for review?
Will do that during the next days.
This patch does not help until bug 588799 is fixed, too.
So that should be next. :-)
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.
Attachment #581066 - Flags: review?(philipp)
This time with better (not misleading) comment.
Attachment #581066 - Attachment is obsolete: true
Attachment #581352 - Flags: review?(philipp)
Assignee: nobody → stefan.fleiter
Status: NEW → ASSIGNED
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.
Attachment #581352 - Flags: review?(philipp) → review+
Pushed to:

comm-central changeset 55b341cdcf82
releases/comm-aurora changeset 0274ebc20d7f
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.2
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: