463960 - Cannot refresh cached CalDAV calendars

Status: RESOLVED FIXED

(Calendar :: Provider: CalDAV, defect)

Description

[Ludovic Marcotte]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.3) Gecko/2008092414 Firefox/3.0.3
Build Identifier: Lightning 0.9

Using Lightning 0.9, it's not possible to refresh cached CalDAV calendars.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.



In calDavCalendar.js, we have:

    get canRefresh caldav_get_canRefresh() {
      // A cached calendar doesn't need to be refreshed.
      return !this.isCached;
    },

This is wrong as events / tasks can be modified on the server and we need to refresh the cache if that's the case. We should rather have this check:

    get canRefresh caldav_get_canRefresh() {
      if (this.isCached && this.mCheckedServerInfo)
	return true;

      return !this.isCached;
    },

This has been successfully tested with SOGo.

Comment 1

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Theoretically, a refresh should be causing the caching wrapper to synchronize and therefore call replayChangesOn(). This is what the comment "A cached calendar doesn't need to be refreshed." meant.

Comment 2

[Daniel Boelzle [:dbo]]

Needs investigation. Ludovic, would you be so kind and propose a trunk patch, too?

Comment 3

[Ludovic Marcotte]

In fact, getRefresh should stay as-is. The problem occurs in safeRefresh.

If the ctag matches, we do not notify our changelog listener. So:

            } else {
                if (thisCalendar.verboseLogging()) {
                    dump("CalDAV: ctag matches, no need to fetch data for calendar "
                        + thisCalendar.name);
                }
		
                // we may still need to poll the inbox
                if (thisCalendar.firstInRealm()) {
                    thisCalendar.pollInBox();
                }

should become:

            } else {
                if (thisCalendar.verboseLogging()) {
                    dump("CalDAV: ctag matches, no need to fetch data for calendar "
                        + thisCalendar.name);
                }
		
		if (thisCalendar.isCached && aChangeLogListener) {
		  aChangeLogListener.onResult({ status: Components.results.NS_OK },
					      Components.results.NS_OK);
		}
		
                // we may still need to poll the inbox
                if (thisCalendar.firstInRealm()) {
                    thisCalendar.pollInBox();
                }

Comment 4

[Ludovic Marcotte]

Also, one thing not to forget (and it'll be included in my upcoming patch) is that if we get an error while fetching the ctag, we should just return like Lightning does right now :

            if (str.substr(0,6) == "<?xml ") {
                    str = str.substring(str.indexOf('<', 2));
            }
            try {
                var multistatus = new XML(str);
            } catch (ex) {
                LOG("CalDAV: Failed to get ctag from server");
                return;
            }

This is wrong as it'll leave pending sync operations in the queue for cached CalDAV calenders. This will prevent Lightning from ever refreshing its cache with the CalDAV server.

To avoid this, we should simply inform the changelog listener, like this:

            if (str.substr(0,6) == "<?xml ") {
                    str = str.substring(str.indexOf('<', 2));
            }
            try {
                var multistatus = new XML(str);
            } catch (ex) {
                LOG("CalDAV: Failed to get ctag from server");
		if (thisCalendar.isCached && aChangeLogListener) {
		  aChangeLogListener.onResult({ status: Components.results.NS_OK },
					      Components.results.NS_OK);
		}
                return;
            }

We don't care, really, about not reporting an error here. Refreshes are frequent enough that we shouln't mark the calendar as readOnly / disabled because one single ctag couldn't be fetched from the server.

Comment 5

[Simon Paquet [:sipaq]]

Ludovic, we're planning to do a release to accompany TB3 beta2 shortly. Any chance that you could provide a patch for this soon?

Comment 6

[Ludovic Marcotte]

Issue is still present in the latest nightly build of both Thunderbird and Lightning.

Comment 7

[Ludovic Marcotte]

Attachment: Proposed fix

Comment 8

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 363573 [details] [diff] [review]
Proposed fix

I've tested this patch and it seems to work well. I noticed a lot of flashing of events, but that may not be this patches' problem

r=philipp

Comment 9

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/2a165d74fb18>

-> FIXED

Comment 10

[Ludovic Marcotte]

(In reply to comment #8)
> (From update of attachment 363573 [details] [diff] [review])
> I've tested this patch and it seems to work well. I noticed a lot of flashing
> of events, but that may not be this patches' problem
> 
> r=philipp

It's not.. but it exposes other problems ;-)  - see my bug report about unnecessary calls to refresh views.

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".