325215 - deleted events spuriously get resurrected

Status: RESOLVED FIXED

(Calendar :: Internal Components, defect)

Description

[Michael Büttner (no reviews TFN)]

steps to reproduce:

1) create a new event with a recurrence rule (e.g. daily for the next 5 days)
2) delete occurrences one after another, starting with the second occurrence
3) previously deleted occurrences will get resurrected after the third occurrence was deleted.
4) deleting resurrected events leads to other previously deleted events to show up again.

this behavior seems to be reproducibly in the month-view only. nevertheless, digging under the hood reveals that the problem is probably related to the implementation of the storage provider.

Comment 1

[Michael Büttner (no reviews TFN)]

wrong guess, the issue is caused by the month-view. changing component appropriately.

Comment 2

[Michael Büttner (no reviews TFN)]

Attachment: fix for this issue

the actual root cause of the bug was that addItem() in the month-view did not play well in case you're giving a new proxy for an already existing item. the correct behavior is to patch the old element of the day-box to refer to the new proxy. otherwise modified events will still refer to the original event (not the eventually already modified one), which could cause data-loss.

i also removed the obsolete 'occurrence'-property of the xul-boxes for events.

and i also fixed the compare for the recurrenceId, it refered to a property 'recurrence_id' which was obviously wrong.

Comment 3

[Joey Minta]

Comment on attachment 210343 [details] [diff] [review]
fix for this issue

(1) I can't reproduce this bug in current builds of Sunbird with a storage-calendar.

(2) When we delete an occurrence of an item, it triggers onModifyItem http://landfill.mozilla.org/mxr-test/mozilla/source/calendar/base/content/calendar-month-view.xml#1061  which should delete any boxes corresponding to the old version of the event.  I can't understand how

+          // the item we're about to add still has a chance that we already know about it.
is possible.  

Can you explain all this further?

Comment 4

[Joey Minta]

Comment on attachment 210343 [details] [diff] [review]
fix for this issue

This bug is not reproducable in Sunbird.  Furthermore, the fact that restarting Lightning persists the ghost-occurrence's return indicates that this is not merely a view problem.  It's, in fact, a problem with the way the occurrence was removed.  Compare Sunbird's http://landfill.mozilla.org/mxr-test/mozilla/source/calendar/resources/content/calendarWindow.js#88
and Lightning's http://landfill.mozilla.org/mxr-test/mozilla/source/calendar/lightning/content/calendar-management.js#185

Comment 5

[Michael Büttner (no reviews TFN)]

I was pretty sure that this one will take some more explanation... You're right with the statement that it's related to the way occurences get removed, which is obviously different between lightning and sunbird (which we should consolidate, by the way).
Anyway, let me describe what actually happens if you delete an occurrence in lightning. Assume you have a recurring event just after creation. The view knows about all occurrences by keeping references in an internal array. Those occurrences refer to the master-event through their parentItem reference. Assume you now delete any of those occurrences by calling "event.calendar.modifyItem(event, aOccurrence, null);". 'event' is the event in the modified state (with the occurrence removed), 'aOccurrence' is the one we actually want to remove (with parentItem still referring to the unmodifed event), which is pretty correct. During execution it will trigger 'onModifyItem' in the view, which will perform two steps:

1) collect all occurrences of 'aOldItem' and delete them from the view.
2) collect all occurrences of 'aNewItem' and add them to the view.

the first step will actually only delete the occurrence we actually wanted to delete, all other occurrences won't be touched. they are still alive and refering to the unmodified event through their parentItem-reference.

the second step will try to add the occurrences of the now modified event, for each occurrence the view will call 'doAddItem', which in turn will check if the item is already known by the view. and here hides the root cause of the problem. we still have the view referencing the 'old' occurrences of the event, while 'doAddItem' truly believes the the new incoming occurrences of the modified event are the same the the ones already contained in the view. 'doAddItem' will therefore reject the new items and keep the old ones. as a result, the view still knows about the old event and keeps it. if we now continue and delete one more occurrence, we will start the process with the event in the unmodified state (which has the added EXDATE still missing), since the view provides us a reference to the unmodified event. Therefore we will delete the occurrence we selected, but we work on the 'unmodified' version of the event and will forget about the previously deleted occurrence. this will result in data loss and looks like the storage provider did something wrong. this is actually not true, i hope the above explanation was able to clarify this.

my proposed patch will add the missing behavior to 'addItem', in case we detect that the incoming item is a proxy of a modified event, we patch our internal data to reflect the new situation. this will keep detruction and recreation of objects at a minimum.

one could say that a resolution to the problem is to not delete the occurrence in the first place, but to delete the whole event (calling modifyItem with aOccurrence.parentItem) as sunbird does. but this would result in the view deleting all occurrences and adding all of them (minus the one we deleted) again. first of all, this is totally unnecessary and maybe more importantly, this will destroy and recreate xul-elements and xpcom-objects and trigger a relayout for all affected day-boxes in the month-view. i can't see a reason to justify such a behavior.

i hope this explanation was able to clarify the situation.

Comment 6

[Joey Minta]

I'm of the belief that the way Lightning handles this is an abuse of the modifyItem method and we're just lucky it didn't break things more severely.  Personally, I think sending in a parent for aNewItem and an occurrence for aOldItem should throw an error.

First, semantically, when an observer's onModifyItem gets 'aOldItem', it seems like that item should be the answer to the question "What item was modified?" but that's not the case with Lightning's model.  

Second, I'm trying to imagine a scenario where simply removing the occurrence would be an insufficient update of the view (because other occurrences depended on that occurrence's location).  From what I remember of the rfc's description of rrules and exdates, I don't think this is possible (maybe with BYSETPOS?), but I'm not 100% confident.  If such a scenario could exist even in theory, then I think that would conclusively rule out Lightning's behavior as correct.

Third, allowing for an occurrence to be passed in as aOldItem while a parent is aNewItem seems inherently fragile.  Only because we rely exclusively on the id property of aOldItem (which an occurrence shares with its parent) does it work.  If we were to compare interface pointers (when bug 325636 is fixed), this would fail.  Also, if someone wanted to try to create a type of diff by comparing aOldItem and aNewItem it wouldn't work.

I think that the month view should simply do the alternative, less nice scenario you described where we delete all occurrences and replace them.  This is what the mutliday-view does.  Furthermore, our sort is stable, so the events won't jump around.

That's my opinion.  I really want mvl and dmose to weigh in on this one though.

Comment 7

[Michael Büttner (no reviews TFN)]

as i first encountered that aOldItem is simply the occurrence while aNewItem is the modified event, i had the same impression, it sounds fragile. but since you can always get to the previously modified event with aOldItem.parentItem it's a nice hint to identify that a single occurrence was deleted. if would vote for keeping this behavior since if we pump the unmodified event into onModifyItem we trigger a whole chain of unnecessary work. I personally find the proposed patch as an elegant way in resolving this issue.

Comment 8

[Daniel Boelzle [:dbo]]

When I first encountered the behaviour:

"deleting an occurence by sending a modifyItem(newModifiedParent, occurence)" (calendar-management.js)

I asked myself why not calling deleteItem( occurence ) straight forward.  The provider knows the best way to delete an occurence; and it would be much clearer.  

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

When calICalendar was initially designed, the theory was that the only method that dealt in occurrences in getItems(); everything else dealt in full ICS items.

The current incarnation of the recurrence code came along somewhat later, and was actually a re-write by Vlad of the initial implementation.

I have some thoughts about this, but I'm still in the process of getting them sorted out in my head.  In the meantime, I'd be very interested to hear Vlad's take on this issue...

Comment 10

[Michael Büttner (no reviews TFN)]

Attachment: new attempt

proposing a different way of solving the above described problem. i think we should first try to get the 0.1 release out of the way and later think about how to address the problem in a clean and elegant way.

Comment 11

[Simon Paquet [:sipaq]]

(In reply to comment #10)
> proposing a different way of solving the above described problem. i think we
> should first try to get the 0.1 release out of the way and later think about
> how to address the problem in a clean and elegant way.

Other Mozilla projects (and I guess we too) have a history of fixing things in the quick-n-ugly way and forgetting about fixing things the right way afterwards.

IMO we should fix this the right way.

Comment 12

[Joey Minta]

Comment on attachment 211014 [details] [diff] [review]
new attempt

+++ mozilla/calendar/base/content/calendar-month-view.xml

One of the questions that had been bugging me was why the multiday-view didn't have this bug as well.  I looked further and doAddEvent() for the column calls internalDeleteEvent, which removes the stale (out of date) occurrences.  On the other hand, when I wrote in the all-day event implementation for the week-view, the fact that there might be a need for such a complex checking system for addEvent never occurred to me.  So, if you want to see some really ugly behavior with this bug, try deleting instances of recurring all-day events in the week view.

At best, this patch is incomplete because it doesn't address that issue.  I still feel that the correct fix is to use .parentItem at http://landfill.mozilla.org/mxr-test/mozilla/source/calendar/lightning/content/calendar-management.js#185

Please ask dmose to review the next patch if you choose to implement a different solution than that.  I think I've gotten too close to this problem to fairly review another patch.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: chat log about items versus occurrences

So we had a chat about this yesterday (before Michael's latest patch and Joey's latest review).  Whether this chat log will really clear anything up, I'm not sure, as much of it is just us sorting out our own confusion.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

I just went and checked, and at this point in time, the storage provider is the only provider that makes any attempt to handle proxy items in modify and delete calls.  Fixing all the providers is likely to be more disruptive than just changing the controller, so I think that for 0.1, Joey's proposed fix is the right one.  Further, about five lines up in that file, it appears that modifyItem has the exact same issue, so it'd be good to make the same fix there.

I think it would be good to file a separate bug as far as what to do in the longer term about settling on the overall architecture of items vs. occurrences and which parts should be handled by the views/controllers versus the providers.  At the very least, we need to document in the IDL what is and isn't allowed, and depending on what we decide, we may want to have either the providers or views/controllers throw exceptions if something invalid is passed in.

Comment 15

[Michael Büttner (no reviews TFN)]

(In reply to comment #11)

sorry for not making myself clear, the proposed patch is not a quick-n-ugly hack but it actually hides the real problem lurking under the hood. what i wanted to say, was that we should get this bug fixed and maybe file another one where we could discuss whether this issue needs further attention or not (which wouldn't block the 0.1 release).

Comment 16

[Michael Büttner (no reviews TFN)]

Attachment: patch as proposed

this patch resolves the problem as proposed by jminta. you're right about fixing the call that causes all those problems, since it addresses the failures in the other views, too. but i've the same opinion as dmose about filing a seperate bug which could address the overall architecture.

Comment 17

[Joey Minta]

Comment on attachment 211124 [details] [diff] [review]
patch as proposed

r=jminta  I completely support a follow-up bug for the architecture review/discussion.

I don't think the modifyOccurrence area that dmose pointed to is troublesome, because they will either both be occurrences, or both be parents in that case.

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

Joey and I discussed this in IRC last week; the modifyOccurrence case is still a problem, because most providers are not expecting to get any proxy items at all.

Comment 19

[Michael Büttner (no reviews TFN)]

As far as I understood it from https://bugzilla.mozilla.org/show_bug.cgi?id=326077#c7 the proposal is to only allow parentItem's to be passed to modifyOccurrence(). the locations comment #18 is refering to is probably http://lxr.mozilla.org/seamonkey/source/calendar/lightning/content/calendar-management.js#213

In my opinion it's not the right way to modify the parentItem instead of the occurence passed as argument to the above mentioned location. modifyOccurrence means that we just want to modify this single occurrence, not the whole event.

Of course I understand that we want to ship the 0.1 release as soon as possible, but I don't believe it's a good strategy to disable complete feature-sets to achieve that goal. Moreover there are several reasons to not follow that direction:

1) changing modifyOccurrence to work with the parentItem would also mean to modify the incoming startDate/endDate since we're given a date that means the date of the occurrence, not of the first event. This would mean to invoke calculations with calIDuration, which in turn will trigger bugs in the implementation of the duration object (negative Dates).

2) The idea to disable editing of recurring events seams to be related to the fact, that there are still bugs in that area. But there are also patches that fix those problems, see https://bugzilla.mozilla.org/show_bug.cgi?id=326077 I'm not sure if this fixes all issues with editing of recurring events, but my question is then, why aren't they on the blocker list?

The whole discussion is related to the question 'fix the root of a problem or fix a symptom?', this just the same as the discussion whether to fix the call of modifyOccurrence() or fix what the views expect. I personally feel that it's nearly almost better to fix the root cause of the problem, otherwise it will never disappear.

I would suggest the following strategy:

1) close bug #325215
2) fix the providers to correctly handle passed proxyItems (the patch would require 3 lines of code, see storageProvider)
3) integrate the patch for bug #326077
4) allow for editing of recurring events instead of disabling it introduce regressions.

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #19)
> 1) changing modifyOccurrence to work with the parentItem would also mean to
> modify the incoming startDate/endDate since we're given a date that means the
> date of the occurrence, not of the first event. This would mean to invoke
> calculations with calIDuration, which in turn will trigger bugs in the
> implementation of the duration object (negative Dates).

The patch for bug 315051 will fix that.

> 2) The idea to disable editing of recurring events seams to be related to the
> fact, that there are still bugs in that area. But there are also patches that
> fix those problems, see https://bugzilla.mozilla.org/show_bug.cgi?id=326077 I'm
> not sure if this fixes all issues with editing of recurring events, but my
> question is then, why aren't they on the blocker list?

Part of the answer to that is because we're not convinced that we know what all those bugs are.  However, we're working on that.  If you find any bugs related to occurrences, please mark them as blocking bug 327889.

> The whole discussion is related to the question 'fix the root of a problem or
> fix a symptom?', this just the same as the discussion whether to fix the call
> of modifyOccurrence() or fix what the views expect. I personally feel that it's
> nearly almost better to fix the root cause of the problem, otherwise it will
> never disappear.

In the slightly longer term, this is absolutely the right strategy.  We're trying to figure out which strategy has the least risk for 0.1, however.

> I would suggest the following strategy:
> 1) close bug #325215

Agreed.  Bug 327889 already covers disabling editing of occurrences, and if we decide to that, there is that place.  I should have filed a separate bug and not re-opened this.

> 2) fix the providers to correctly handle passed proxyItems (the patch would
> require 3 lines of code, see storageProvider)

Filed as bug 327990.

> 3) integrate the patch for bug #326077

Done.  mvl and I talked this over today, and he has since landed that patch.

> 4) allow for editing of recurring events instead of disabling it introduce
> regressions.

That's bug 327889.