458828 - Many errors when setting up zimbra calendar

Status: RESOLVED FIXED

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

Description

[David Ascher (:davida)]

I just built lightning & tb from trunk.

I setup my zimbra caldav calendar.

1) it asked me for my credentials twice.  AFAIK i entered them correctly the first time, but I don't know that for a fact.  Having a real error message if they were incorrect would be good.

2) I got 30 reminders for past events (somehow we should not bring up reminders on first load of calendar, ISTM).  I hit the dismiss all button, and got:

 Warning: There has been an error reading data for calendar: Zimbra.  However, this error is believed to be minor, so the program will attempt to continue. Error code: DAV_PUT_ERROR. Description: There was an error storing the item on the server.

on console (30 times), and an error dialog, whose details button doesn't properly expand the dialog.  

I'm happy to provide more data if that helps figure out the errors.

Comment 1

[Bruno Browning]

If you don't already have them, two boolean (true) prefs calendar.debug.log and calendar.debug.log.verbose will get a good bit of diagnostic information spewed to the error console. It sounds like the write to the server to dismiss the alarm is failing; that'll tell us for sure.

Comment 2

[David Ascher (:davida)]

I'm seeing a lot of the following in the logs w/ those prefs enabled:

X-LIC-ERROR;X-LIC-ERRORTYPE=VALUE-PARSE-ERROR:No value for LOCATION property
 . Removing entire property:

Comment 3

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to comment #2)
> I'm seeing a lot of the following in the logs w/ those prefs enabled:
> 
> X-LIC-ERROR;X-LIC-ERRORTYPE=VALUE-PARSE-ERROR:No value for LOCATION property
>  . Removing entire property:

This should "go away" with the v3 patch of bug 438964. I'm not sure if X-LIC-ERROR properties are auto-removed by libical, but the empty LOCATION property should now no longer be written.

Comment 4

[Daniel Boelzle [:dbo]]

Google seems to send out empty LOCATION properties, too, on iTIP/iMIP. I don't read anything in the RFC against doing so, thus I think the following libical code needs fixing: <http://mxr.mozilla.org/comm-central/source/calendar/libical/src/libical/icalparser.c#1009>.
However, I don't think this parsing error is related to the Zimbra problem. It only concerns empty properties.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

I'm bumping this to P1, because it essentially means that neither davida nor I can dogfood CalDAV effectively, and since we need good dogfooding experience to make sound product driving decisions...

Comment 6

[Daniel Boelzle [:dbo]]

Attachment: fix - v1

I am not entirely sure, but to me it seems e4x doesn't handle default namespaces correctly. Zimbra's returned multistatus correctly has all hrefs in "DAV:" namespace:

<multistatus xmlns="DAV:">
  <response>
    <href>/principals/users/dbo/</href>
    <propstat>
      <status>HTTP/1.1 200 OK</status>
      <prop>
        <calendar-home-set xmlns="urn:ietf:params:xml:ns:caldav">
          <href xmlns="DAV:">/dav/dbo/</href>
        </calendar-home-set>
        <calendar-user-address-set xmlns="urn:ietf:params:xml:ns:caldav">
          <href xmlns="DAV:">mailto:[e-mail]</href>
          <href xmlns="DAV:">/principals/users/dbo/</href>
        </calendar-user-address-set>
        <schedule-inbox-URL xmlns="urn:ietf:params:xml:ns:caldav">
          <href xmlns="DAV:">/dav/dbo/Inbox/</href>
        </schedule-inbox-URL>
        <schedule-outbox-URL xmlns="urn:ietf:params:xml:ns:caldav">
          <href xmlns="DAV:">/dav/dbo/Sent/</href>
        </schedule-outbox-URL>
      </prop>
    </propstat>
  </response>
</multistatus>

Iterating the response elements the hrefs come unqualified, lacking their correct namespace ("DAV:") either per default xmlns or prefix, and inherit the wrong default xmlns of their parents (i.e. caldav):

response: <response xmlns="DAV:">
  <href>/principals/users/dbo/</href>
  <propstat>
    <status>HTTP/1.1 200 OK</status>
    <prop>
      <calendar-home-set xmlns="urn:ietf:params:xml:ns:caldav">
        <href>/dav/dbo/</href>
      </calendar-home-set>
      <calendar-user-address-set xmlns="urn:ietf:params:xml:ns:caldav">
        <href>mailto:[e-mail]</href>
        <href>/principals/users/dbo/</href>
      </calendar-user-address-set>
      <schedule-inbox-URL xmlns="urn:ietf:params:xml:ns:caldav">
        <href>/dav/dbo/Inbox/</href>
      </schedule-inbox-URL>
      <schedule-outbox-URL xmlns="urn:ietf:params:xml:ns:caldav">
        <href>/dav/dbo/Sent/</href>
      </schedule-outbox-URL>
    </prop>
  </propstat>
</response>

This patch should take care and consolidates some more mixed ns trial code, but needs further testing. I've tested on Zimbra only.

Comment 7

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 349321 [details] [diff] [review]
fix - v1

>             for each (let response in multistatus.*::response) {
> 
>+                function getSafeHrefs(baseElem, tag) {
>+                    function getSafeElem(baseElem, tag) {
>+                        // tries all mixed ns combinations current servers seem to reply...
>+                        let elem = baseElem..C::[tag];
>+                        if (elem.toString().length) {
>+                            return elem;
>+                        }
>+                        elem = baseElem..D::[tag];
>+                        return (elem.toString().length ? elem : null);
>+                    }
>+                    let elem = getSafeElem(baseElem, tag);
>+                    return (elem ? getSafeElem(elem, "href") : []);
>                 }
I'd prefer defining this funtion outside of the loop, maybe even top level. Aside from that I think we should rather be less restrictive on the namespace. Something like the following should also do:

let responseCHS = response..*::["calendar-home-set"]..*::href[0];


It will probably bork if the server puts in some other elements, like:

<D:multistatus xmlns:MC="urn:mycompany:mystuff">
  <D:response>
    <D:href>abc</D:href>
    <MC:href>top-secret-url-link</MC:href>
  </D:response>
</D:multistatus>

but I don't really think thats probable. If you really want you can get the elements using wildcard operators (but no child index) and then retrieve the preferred element using a function similar to this one:

function getPreferredElement(aXMLList, aPreferedNamespace) {
  if (!aPreferredNamespace) {
    aPreferredNamespace = new Namespace("D", "DAV:");
  }
  for (let tag in aXMLList) {
    if (tag.name().uri == aPreferredNamespace.uri) {
      return tag;
    }
  }
  return aXMLList[0];
}

>-                    if (responseCHS.charAt(responseCHS.toString().length -1) != "/") {
>+                    responseCHS = responseCHS[0].toString();
>+                    if (responseCHS.charAt(responseCHS.length -1) != "/") {
>                         responseCHS += "/";
>                     }
I'd prefer using |responseCHS = responseCHS[0].toString().replace(/([^/])$/, "$1/")| (please test, I usually get the syntax wrong!) and no try/catch block.


r- for now, since I'd rather see the wildcard approach, but I'm sure I can be talked into using a function too.

Comment 8

[Daniel Boelzle [:dbo]]

Attachment: fix - v2

Cool, I didn't know that e4x supports namespace wildcards :)

I agree with using a wildcard. Even though it allows more malformed server responses, it makes our code leaner and less obscure.

Comment 9

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 349716 [details] [diff] [review]
fix - v2

Looks good, r=philipp

Comment 10

[Daniel Boelzle [:dbo]]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/394ec192af25>

-> FIXED

Comment 11

[Philipp Kewisch [:Fallen][📱📆]]

These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".