Undo is not possible for task/event modification

VERIFIED FIXED in 0.8

Status

--
blocker
VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: thetux.moz, Assigned: dbo)

Tracking

({dataloss, regression})

Mozilla 1.8 Branch
dataloss, regression
Bug Flags:
blocking-calendar0.8 +

Details

Attachments

(2 attachments)

(Reporter)

Description

11 years ago
Steps to Reproduce :
====================

1) Create an event or task
2) Modify this event/task
3) Click on "Edit | Undo"

Actual Results:
===============

Undo does not work.

Expected Results:
=================

It should be possible to undo every modification.

Build Date & Platform:
======================

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.13pre) Gecko/20080220 Calendar/0.8pre 20080220
Flags: blocking-calendar0.8?
Regression range: Works in Lightning 0.8pre (2007-12-15-03)
                  Fails in Lightning 0.8pre (2007-12-16-04)

Checkins during regression range: http://tinyurl.com/3x7qmy
                                  Bug 408237, Bug 407574, Bug 405418

Could this be caused by the fix for Bug 408237?
Keywords: regression
Confirming on mac as well.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.13pre)
Gecko/20080220 Calendar/0.8pre
Hardware: PC → All
Confirmed as regession from Bug 408237, removing the change from Bug 408237 makes Undo work again. 

Or maybe Bug 408237 only revealed a new error. The Undo command calls modifyItem() that exits the method early after comparing the generation attribute, e.g. http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/calendar/providers/storage/calStorageCalendar.js&rev=1.132&mark=517#515 or http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/calendar/providers/memory/calMemoryCalendar.js&rev=1.67&mark=241#234

Unfortunately the errors reported by reportError() show up nowhere.
This needs to be fixed before we release 0.8
Flags: blocking-calendar0.8? → blocking-calendar0.8+
Daniel, do you think temporary reverting Bug 408237 might be a solution for now?
(In reply to comment #5)
> Daniel, do you think temporary reverting Bug 408237 might be a solution for
> now?

Stefan, Daniel is away on vacation until the beginning of March. I'd say we backout the patch for bug 408237 temporarily for the release and fix this correctly after 0.8 is out of the door.

Philipp, Clint, what do you think?

I think we should just fix this issue. Temporary fixes are always a bad plan. Especially when the temporary fixes checks in obviously wrong code.
If you really want to fix the 'temporary', just remove the check at the links in comment 3 and add a bunch of code comments. Don't put the broken code back in.
The question for really fixing this bug is: what should happen with the generation number on undo? Should the complete old item be put back, including the generation, or should the generation be increased?
The current transaction code assumes the former (by not doing anything with generation), while the providers assume the latter (by making sure the generation always increases). One of them has to be changed.
I think that the generation should be increased on undo, and a check should be made that the old generation is exactly one less then the currently stored generation.
I'm not sure though where this should happen, mainly due to the possibility of redo. For redo, the same should happen, except for the first 'do'. But the first 'do' goes through the same codepath as 'redo'...
Guys, we need to get some traction on this. Either we go one of the routes that mvl is discussing in comment 8 *right now* or we backout the patch from bug 408237  now, reapply it after 0.8 is out and fix the bug early in the 0.9 timeframe.

Can you please comment on this issue ASAP, so that we have a decision by tomorrow (Monday, 3rd).
(Reporter)

Comment 10

11 years ago
Taking correct code out of the programm is a bad idea (and a waste of resources). Since we have limited developer resources it is even more worse.

Comment 11

11 years ago
As we stated in our last status meeting from February 27th we want to remove this from the blocking list. Maybe Daniel can take care of it when he is back from holiday next week.
Flags: wanted-calendar0.8?
Flags: blocking-calendar0.8-
Flags: blocking-calendar0.8+
Wow, that a real bad decision. Sorry to be so hard. But undo is a mayor feature and very, very important. (Not having undo is actually a dataloss bug.)
You can't just leave it broken. No discussion really possible.
Flags: blocking-calendar0.8- → blocking-calendar0.8?
Well, of course this needs to be fixed before the release. IMO the question only is, how we fix it. Do we

- go the temporary fix road by backing out the patch from bug 408237 now, 
  reapplying it after 0.8 is out and fixing the bug for real early in the 0.9
  timeframe?
- do we fix the bug right now by going one of the routes that mvl proposed in 
  comment 8, having in mind that this might turn up other serious regressions 
  very late in the game?

From a process perspective, I would favor option 2. From a risk perspective, option 1 is clearly better. Therefore I would propose to choose option 1.

Comment 14

11 years ago
I backed out the patch of bug 408237 and gave this Andreas to test. He found that Undo-Redo worked again without complaints. So I will check this in.
Please, NO!!! You are putting in code with known problems that might very well cause other regressions.
I urge you to consider one of the other fixes I proposed earlier.
(And I think that the real fix won't be that hard, if you spend a few minutes thinking about how to do it exactly)
Yes, this blocks the final 0.8 release. Also per the confcall decision, backing out Daniel's patch will break iTip. We will release the RC1 without this, but we should really get this fixed for the final.
Flags: wanted-calendar0.8?
Flags: blocking-calendar0.8?
Flags: blocking-calendar0.8+
A possible real solution:
- Return the modified items from the providers
- Store that return item, instead of the original new item (because it is not the same: the generation changed)
- in the transaction manager, copy the generation of the old item to the new item before do and undo

I _think_ that this should work. didn't do any testing yet.
mvl, it would be really great, if you could hook up a first WIP patch for your proposal. Then Philipp or Daniel (when he returns next week from vacation) can take a look at it and we can ask Andreas to test it thoroughly.

Comment 19

11 years ago
This way you will save some time to avoid regressions that may turn up.
Created attachment 307550 [details] [diff] [review]
WIP patch

a work-in-progress patch. It does what I described in comment 17. (the first two items are already implemented).
This patch makes undo work for me, at least with storage calendars. There is still a problem though: the generation decreases when you do multiple undos. Older transactions still store the generation as it was at the time of executing them. But at undo, that's no longer the current generation. Undo still works, because of the storage provider only comparing the oldItem as passed in, not as what's really currently stored. I don't know what other providers do.
Comment on attachment 307550 [details] [diff] [review]
WIP patch

Although this patch has known problems, it does improve the situation. If the problems with Itip are not too big, I think we should go for this for now.
Attachment #307550 - Flags: review?(philipp)
Severity: major → blocker
Keywords: dataloss
Comment on attachment 307550 [details] [diff] [review]
WIP patch

Since I was not on the confcall, I don't have an idea how this could break itip.

Maybe Clint can bring more light into this?

From a code standpoint, I'd like to see the XXX comment a bit more elaborate and if possible with a bug reference. Just looking at code, r+ with the above taken care.
Attachment #307550 - Flags: review?(philipp) → review?(ctalbert)
(Assignee)

Comment 23

11 years ago
The WIP patch doesn't work for me when performing multiple undos on the same item (ics file). I can imagine this is due to the independent transaction objects, e.g. if one transaction is undone, it updates its own items (implementing an calIOperationListener), but leaves the prior and next transaction objects untouched (one transaction object's mItem is the next transaction object's mOldItem...). Moreover I spotted that calMemoryCalendar's deleteItem() always checks against the generation of it's stored item, I think this is another problem.
(Assignee)

Comment 24

11 years ago
Created attachment 308719 [details] [diff] [review]
allow generation downgrades

My understanding of the generation attribute has always been that it serves as a version number when modifying an item, somehow similar to the SEQUENCE number. Quite helpful due to an item's value semantics, to check whether a modification is performed on that latest version of an item.
The only feasible solution I can imagine (keeping the generation for that purpose) is to also allow generation downgrades, thus related transaction objects stick to their original generation. For the purpose of version safety, it's IMO sufficient to check only the oldItem's generation against the stored one of an item.
So this patch modifies
- storage/memory to act like that; and increment only in case of a direct modification (cloned oldItem of same generation)
- removes caldav's check: IMO not needed since etag checking is performend in caase of modifyItem()
(neither wcap nor gdata implement generation)
Rudimental tests work well for me for storage and ics.
Assignee: nobody → daniel.boelzle
Status: NEW → ASSIGNED
Attachment #308719 - Flags: review?(mvl)
(Assignee)

Comment 25

11 years ago
Looking further on the transaction code, I can think of race conditions due the async nature of some providers: Even if we could update related items of other transaction objects, multiple transactions would need to be queued until a store has been performed (onOperationComplete).

Comment 26

11 years ago
Comment on attachment 307550 [details] [diff] [review]
WIP patch

From comment 23, it looks like this doesn't completely solve the problem so we're going to r- on it.  Thanks for the patch.
Attachment #307550 - Flags: review?(ctalbert) → review-
(Assignee)

Comment 27

11 years ago
mvl, do you have cycles left for this or should I pick someone else for review?
Comment on attachment 308719 [details] [diff] [review]
allow generation downgrades

Looks good, r=philipp
Attachment #308719 - Flags: review?(mvl) → review+
(Assignee)

Comment 29

11 years ago
Checked in on HEAD, MOZILLA_1_8_BRANCH and SUNBIRD_0_8_BRANCH => FIXED.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 0.8

Comment 30

11 years ago
Checked with Sunbird and Lightning builds 20080330 -> task is fixed and verified. 
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.