827078 - Items deleted server-side still appear in cached CalDAV calendars

Status: RESOLVED FIXED

(Calendar :: Provider: CalDAV, defect)

Description

[Matthew Mecca [:mmecca]]

STR:
1) Subscribe to a new Google calendar via CalDAV, select Offline Support
2) Create a new event in the calendar (using Lightning)
3) Exit Thunderbird
4) Delete the event from the calendar web interface
5) Restart Thunderbird

Results:
The deleted event still appears in the calendar.

Attempting to edit the event in Lightning results in:

Error: this.mItemInfoCache[aNewItem.id] is undefined
Source file: file:.../components/calDavCalendar.js
Line: 658

This is likely (at least one of) the root cause(s) of Bug 732393. The cal_metadata table in the cache.sqlite db contains no matching row for the cached item.

Comment 1

[Matthew Mecca [:mmecca]]

Attachment: Fix v1

Similar to the problem in Bug 700637, the new item meta-data is being deleted from the cache when the newly created item is first received. 

This patch fixes the problem for me for newly created items, but not for pre-existing items with already deleted meta-data. Is there a way we can trigger a one-time rebuild of the cache?

Comment 2

[Mohit Kanwal [:redDragon]]

(In reply to Matthew Mecca [:mmecca] from comment #1)
> Created attachment 698386 [details] [diff] [review]
> Fix v1
> 
> Similar to the problem in Bug 700637, the new item meta-data is being
> deleted from the cache when the newly created item is first received. 

Should we doing something else in relation to the metadata ? Also at many places deleteItemById(aItem) is used whereas the full signature is deleteItemById(aItem, aIsModify) will this cause a  problem?

> This patch fixes the problem for me for newly created items, but not for
> pre-existing items with already deleted meta-data. Is there a way we can
> trigger a one-time rebuild of the cache?

Perhaps a preference setting could be added to recreate the cache on the next startup of the application and this can be intercepted from the calendarManager.

Comment 3

[Matthew Mecca [:mmecca]]

(In reply to Mohit Kanwal [:redDragon] from comment #2)
> Should we doing something else in relation to the metadata ? Also at many
> places deleteItemById(aItem) is used whereas the full signature is
> deleteItemById(aItem, aIsModify) will this cause a  problem?

I was hoping you had a better answer than me to the first question ;) - maybe we can leave the meta-data alone when deleting an item at the cache level and handle deletion at the provider level?

As for the second parameter, if it's not explicitly specified as in deleteItemById(aItem) it will have the value of undefined, which in this case will have the same effect as if false was passed in, and any meta-data rows matching the item will be removed. In the case that we're actually deleting the item that should be fine, otherwise if we're only temporarily removing the item from the cache the meta-data should be preserved.

Comment 4

[Mohit Kanwal [:redDragon]]

The sequence of cached item storage/deletion happens like this: First cache layer tries to invoke the provider layer to do adding/deleting/modifying stuff, then the storage layer is invoked by the cached layer to store the necessary result [do deletion/addition etc.]

If the provider fails (due to network) or if the cached layer knows the app is offline, it stores the necessary results via flags in the cache which when the app comes back online is tried again by the cache layer. Now for the meta-data it did not assume there would be issues.

Will there be an issue if the meta-data is kept behind?

Comment 5

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

Comment on attachment 698386 [details] [diff] [review]
Fix v1

I'm glad to hear a potential solution for this is on the way!


(In reply to Mohit Kanwal [:redDragon] from comment #2)
> (In reply to Matthew Mecca [:mmecca] from comment #1)
> > Created attachment 698386 [details] [diff] [review]
> > Fix v1
> > 
> > Similar to the problem in Bug 700637, the new item meta-data is being
> > deleted from the cache when the newly created item is first received. 
> 
> Should we doing something else in relation to the metadata ? Also at many
> places deleteItemById(aItem) is used whereas the full signature is
> deleteItemById(aItem, aIsModify) will this cause a  problem?
If a parameter is not passed, then it just defaults to undefined, which is close enough to false. This technique is often used to emulate default parameters.

The only other thing we should check all the other cases where the metadata is being deleted and make sure that its intentional.


> > This patch fixes the problem for me for newly created items, but not for
> > pre-existing items with already deleted meta-data. Is there a way we can
> > trigger a one-time rebuild of the cache?
> 
> Perhaps a preference setting could be added to recreate the cache on the
> next startup of the application and this can be intercepted from the
> calendarManager.

We have to be careful here. While in general it would be good to reset the cache, there might be a set of users that complain that the new version is too slow, i.e if they have a lot of items to be cached.

A different solution would be to trigger the caldav provider to redownload the item when its metadata is missing. This is obviously the more complicated solution, but will improve the upgrade experience.

If you do prefer to rebuild the cache, I'd suggest adding some UI notifying the user that the cache will be rebuilt and it might take a while one time only. A similar solution to database versions/ui versions might be a good idea.

Codewise r+ for this patch, but I'd appreciate if you could go through all paths where metadata is deleted. Also, a patch to either update the item metadata if its missing on a modify in the caldav provider, or at least to reset the cache would be nice. Feel free to do that in a separate bug/patch. Mohit can take care of any further reviews if he likes.

Comment 6

[Mohit Kanwal [:redDragon]]

Comment on attachment 698386 [details] [diff] [review]
Fix v1

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

Looks good! Yeah I think I need to take a look into calStorageCalendar and find out if we are deleting the metadata in other places :|

Comment 7

[Matthew Mecca [:mmecca]]

Attachment: Part 1: preserve meta-data when items added - v2

We shouldn't need the extra parameter for flushItem after all, since we want to preserve the meta-data in all case we call into it.

Aside from the cases corrected in this patch I don't see any other cases where we delete meta-data improperly at the storage provider level.

Comment 8

[Matthew Mecca [:mmecca]]

Attachment: Part 2: ensure meta-data at load time

Adds in meta-data for cached items if none exists at load time, and triggers an item refresh if any meta-data was missing. 

This doesn't noticeably increase startup time for me, but I don't have any really large cached calendars, I'd appreciate some extra testing if possible.

Comment 9

[Matthew Mecca [:mmecca]]

Mohit, can you also take a look at the resetLog function at http://mxr.mozilla.org/comm-central/source/calendar/providers/caldav/calDavCalendar.js#180 ? It looks like we're explicitly deleting meta-data in that case.

Comment 10

[Mohit Kanwal [:redDragon]]

> This doesn't noticeably increase startup time for me, but I don't have any
> really large cached calendars, I'd appreciate some extra testing if possible.

I have a .NET application that can create events programmatically on Google Calendar. I am tempted to try it out on this :)

> Mohit, can you also take a look at the resetLog function at
> http://mxr.mozilla.org/comm-central/source/calendar/providers/caldav/
> calDavCalendar.js#180 ? It looks like we're explicitly deleting meta-data in
> that case.

Okay for this case  I think we should delete the meta-data because resetting the cache is only required when we are setting up the calendar. So I think this should be safe.

http://mxr.mozilla.org/comm-central/source/calendar/base/src/calCachedCalendar.js#174 ?

Comment 11

[Ludovic Marcotte]

I made these two patches available to the SOGo community and promoted them for testing. So far, I only got good feedback so they should most likely be included.

Would it also be possible to create an updated Lightning v1.9 build for Thunderbird 17 EST with these fixes in?

Thanks!

Comment 12

[Mohit Kanwal [:redDragon]]

Hi Matt,

I have tested the two patches against a google calendar with about 1000 events and seems to be pretty okay. I tried without the patch and 1000 events and loading time seems to be the same. No error or any missing meta-data issues encountered so far for me. Setting r+ for both.

Comment 13

[Mohit Kanwal [:redDragon]]

Comment on attachment 701647 [details] [diff] [review]
Part 1: preserve meta-data when items added - v2

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

Looks Good!

Comment 14

[Mohit Kanwal [:redDragon]]

Comment on attachment 701652 [details] [diff] [review]
Part 2: ensure meta-data at load time

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

Good To Go :)

::: calendar/providers/caldav/calDavCalendar.js
@@ +313,5 @@
>              } else {
> +                let itemDataArray = itemData.split("\u001A");
> +                let etag = itemDataArray[0];
> +                let resourcePath = itemDataArray[1];
> +                let isInboxItem = itemDataArray[2];

Nice!

Comment 15

[Matthew Mecca [:mmecca]]

Pushed to comm-central:
Part 1 - https://hg.mozilla.org/comm-central/rev/3a2d8a94d945
Part 2 - https://hg.mozilla.org/comm-central/rev/1618ebc93e5f

Comment 16

[Matthew Mecca [:mmecca]]

Comment on attachment 701647 [details] [diff] [review]
Part 1: preserve meta-data when items added - v2

This bug can trigger un-dismissable alarms, I think it might be a good candidate for a 1.9.1 release.

Comment 17

[Ludovic Marcotte]

What's the status for getting this in 1.9.1?

Thunderbird 17 ESR with Lightning 1.9 is unusable without this fix.

Comment 18

[wimmer]

I agree.
Please release 1.9.1 with this fix.

Comment 19

[Alessio]

Please, it's absolutely unusable.

Comment 20

[Giovanni Bechis]

(In reply to Ludovic Marcotte from comment #11)
> I made these two patches available to the SOGo community and promoted them
> for testing. So far, I only got good feedback so they should most likely be
> included.
> 
> Would it also be possible to create an updated Lightning v1.9 build for
> Thunderbird 17 EST with these fixes in?
> 
> Thanks!
I tried Ludovic's patches with SOGo and they work very well, I will love a new Lightning release with those patches applied.

Comment 21

[Matthew Mecca [:mmecca]]

Pushed to comm-aurora:
Part 1 - https://hg.mozilla.org/releases/comm-aurora/rev/c6e12759e3fe
Part 2 - https://hg.mozilla.org/releases/comm-aurora/rev/d6f1faa961a6

Comment 22

[Steven Ingram]

Those of us using Thunderbird with SOGo and so must stick with the ESR can't upgrade to Lightning 2.2. It would be much appreciated to have a 1.9.1 release with these patches.

Comment 23

[Matthew Mecca [:mmecca]]

(In reply to Steven Ingram from comment #22)
> Those of us using Thunderbird with SOGo and so must stick with the ESR can't
> upgrade to Lightning 2.2. It would be much appreciated to have a 1.9.1
> release with these patches.

We will be releasing a version 1.9.1 with this fix soon.

Comment 24

[Matthew Mecca [:mmecca]]

Pushed to comm-beta:
Part 1 - https://hg.mozilla.org/releases/comm-beta/rev/0e0ea731b268
Part 2 - https://hg.mozilla.org/releases/comm-beta/rev/02a24d3f0b04

Comment 25

[Matthew Mecca [:mmecca]]

Pushed to comm-esr17:
Part 1 - https://hg.mozilla.org/releases/comm-esr17/rev/3c20b0923d6d
Part 2 - https://hg.mozilla.org/releases/comm-esr17/rev/b1d4f46d856f

Comment 27

[steve_a]

Matthew Mecca said on 2013-02-05:

"We will be releasing a version 1.9.1 with this fix soon."

As of today, it has NOT been released as the official Lightning extension.

This has caused some confusion for those of us who use the extension.  It does not appear on the extension page but in the nightlies,  Mozilla should publish it as the official release.

Thank you.

Comment 28

[L-P]

+1

Comment 29

[julian.robbins]

This is a critical bug. We are having to roll out a nightly just to keep lightning working for us ....

Comment 30

[Stefan Sitter [:ssitter]]

Lightning 1.9.1 is already uploaded to addons.mozilla.org but is waiting for approval. Once this is done it should download and install automatically. For manual download see https://addons.mozilla.org/thunderbird/addon/lightning/versions/1.9.1

Comment 31

[steve_a]

(In reply to Stefan Sitter from comment #30)
> Lightning 1.9.1 is already uploaded to addons.mozilla.org but is waiting for
> approval. Once this is done it should download and install automatically.
> For manual download see
> https://addons.mozilla.org/thunderbird/addon/lightning/versions/1.9.1

Thank you!

I see it was released (uploaded to Add-ons) on March 6, so it should be available soon.