Closed Bug 827078 Opened 12 years ago Closed 12 years ago

Items deleted server-side still appear in cached CalDAV calendars

Categories

(Calendar :: Provider: CalDAV, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mmecca, Assigned: mmecca)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Summary: Items deleted server-side are still appear in cached CalDAV calendars → Items deleted server-side still appear in cached CalDAV calendars
Attached patch Fix v1 (obsolete) — — Splinter Review
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?
Assignee: nobody → matthew.mecca
Status: NEW → ASSIGNED
Attachment #698386 - Flags: review?(philipp)
Attachment #698386 - Flags: feedback?(mohit.kanwal)
(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.
(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.
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 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.
Attachment #698386 - Flags: review?(philipp) → review+
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 :|
Attachment #698386 - Flags: feedback?(mohit.kanwal) → feedback+
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.
Attachment #698386 - Attachment is obsolete: true
Attachment #701647 - Flags: review?(mohit.kanwal)
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.
Attachment #701652 - Flags: review?(mohit.kanwal)
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.
> 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 ?
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!
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 on attachment 701647 [details] [diff] [review] Part 1: preserve meta-data when items added - v2 Review of attachment 701647 [details] [diff] [review]: ----------------------------------------------------------------- Looks Good!
Attachment #701647 - Flags: review?(mohit.kanwal) → review+
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!
Attachment #701652 - Flags: review?(mohit.kanwal) → review+
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.
Attachment #701647 - Flags: approval-calendar-release?(philipp)
Attachment #701647 - Flags: approval-calendar-beta?(philipp)
Attachment #701647 - Flags: approval-calendar-aurora?(philipp)
Attachment #701652 - Flags: approval-calendar-release?(philipp)
Attachment #701652 - Flags: approval-calendar-beta?(philipp)
Attachment #701652 - Flags: approval-calendar-aurora?(philipp)
Target Milestone: --- → 2.3
Whiteboard: [wanted-1.9.x]
Target Milestone: 2.3 → ---
Target Milestone: --- → 2.3
What's the status for getting this in 1.9.1? Thunderbird 17 ESR with Lightning 1.9 is unusable without this fix.
I agree. Please release 1.9.1 with this fix.
Please, it's absolutely unusable.
(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.
Attachment #701647 - Flags: approval-calendar-release?(philipp)
Attachment #701647 - Flags: approval-calendar-release+
Attachment #701647 - Flags: approval-calendar-beta?(philipp)
Attachment #701647 - Flags: approval-calendar-beta+
Attachment #701647 - Flags: approval-calendar-aurora?(philipp)
Attachment #701647 - Flags: approval-calendar-aurora+
Attachment #701652 - Flags: approval-calendar-release?(philipp)
Attachment #701652 - Flags: approval-calendar-release+
Attachment #701652 - Flags: approval-calendar-beta?(philipp)
Attachment #701652 - Flags: approval-calendar-beta+
Attachment #701652 - Flags: approval-calendar-aurora?(philipp)
Attachment #701652 - Flags: approval-calendar-aurora+
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.
(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.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: 2.1 → 1.9.1
Blocks: 845745
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.
+1
This is a critical bug. We are having to roll out a nightly just to keep lightning working for us ....
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
(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.
Whiteboard: [wanted-1.9.x]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: