Last Comment Bug 1182264 - Possible dataloss after editing calendar properties
: Possible dataloss after editing calendar properties
Status: RESOLVED FIXED
: dataloss, regression
Product: Calendar
Classification: Client Software
Component: General (show other bugs)
: Lightning 4.0.0.1
: Unspecified Unspecified
-- critical (vote)
: 4.0.1.2
Assigned To: [:MakeMyDay]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2015-07-09 13:29 PDT by Robert Brand
Modified: 2015-07-28 12:36 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
FixAccidentallyEnabledCachingIfCacheIsUnsupported-V1.diff (2.47 KB, patch)
2015-07-11 03:18 PDT, [:MakeMyDay]
philipp: review+
philipp: approval‑calendar‑aurora+
philipp: approval‑calendar‑beta+
philipp: approval‑calendar‑esr+
Details | Diff | Splinter Review
FixAccidentallyEnabledCachingIfCacheIsUnsupported-V2.diff (3.36 KB, patch)
2015-07-15 14:18 PDT, [:MakeMyDay]
makemyday: review+
Details | Diff | Splinter Review

Description User image Robert Brand 2015-07-09 13:29:27 PDT
GerryB from thunderbird-mail.de reported:
1. Create a new calendar.
2. Create a new event in new calendar.
3. Right-click on new calendar and edit its color.
4. Click OK and OK.

The selection in the calendar list changes and the created event from step 2 disappears. It doesn't come back after restart (dataloss). It seems as if you have to edit the calendar properties once to get a 'healthy' calendar. Afterwards you can create events and edit calendar properties as you like, nothing is lost.
Problem is still present in Lightning 4.4a1.
Comment 1 User image Decathlon 2015-07-09 16:18:45 PDT
It seems caused by the patch for bug 768207
Comment 2 User image Philipp Kewisch [:Fallen] 2015-07-10 00:56:53 PDT
Aw man, I thought that was such a simple patch. Can you create a patch for this?
Comment 3 User image Decathlon 2015-07-10 06:58:57 PDT
I'll try.
I'll change the assignment when I'll realize that I am able to fix it. Until then if someone wants to fix, please go on.
Comment 4 User image Philipp Kewisch [:Fallen] 2015-07-10 08:07:57 PDT
Ok, let me know if you need help. I haven't taken a closer look, it sounded like you knew exactly what line is causing it.
Comment 5 User image [:MakeMyDay] 2015-07-11 03:18:29 PDT
Created attachment 8632426 [details] [diff] [review]
FixAccidentallyEnabledCachingIfCacheIsUnsupported-V1.diff

This happens only if cache is already enabled for the respective calendar, but caching is not supported. Unfortunately, since bug 768207, caching is enabled anyway - irrespective whether it's supported or not.

This patch takes care to deal with an accidentally enabled cache mode of an calendar and stops setting cache.enabled while creating new calendars, if caching is not supported.

This bug will mainly affect users, who have created their calendars in Lightning 4.0+ and change any calendar property, but there is also a risk for users with previously created calendars, as you can enable/disable cache mode for that calendars by using the property icons in the calendar list. The result in both cases is a complete dataloss for the respective calendar.

That said, I suggest not to wait another 5 weeks for 4.0.2 to get this to lelease, but spin a Lightning 4.0.1.1 immediately to avoid bad user expirience esp. for the new users and bad publicity accordingly.
Comment 6 User image [:MakeMyDay] 2015-07-11 03:24:58 PDT
One more thing, Philipp: When checking this, I came accross [1], where REMOVE_UNSUBSCRIBE is used, which is not defined anywhere - should that be REMOVE_NO_DELETE instead? You introduced that with bug 351499.

[1] http://mxr.mozilla.org/comm-central/source/calendar/base/src/calCalendarManager.js#851
Comment 7 User image Philipp Kewisch [:Fallen] 2015-07-14 15:16:16 PDT
Comment on attachment 8632426 [details] [diff] [review]
FixAccidentallyEnabledCachingIfCacheIsUnsupported-V1.diff

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

(In reply to MakeMyDay from comment #6)
> One more thing, Philipp: When checking this, I came accross [1], where
> REMOVE_UNSUBSCRIBE is used, which is not defined anywhere - should that be
> REMOVE_NO_DELETE instead? You introduced that with bug 351499.

Yes, this should be REMOVE_NO_DELETE. Probably makes sense to add it to this patch.


r/a=me with comments considered. If you upload a new patch and/or push it, I'll do a new release rsn.

::: calendar/resources/content/calendarCreation.js
@@ +174,5 @@
>      gCalendar.name = cal_name;
>      gCalendar.setProperty('color', cal_color);
>      if (!gCalendar.getProperty("cache.always")) {
> +        gCalendar.setProperty("cache.enabled", gCalendar.getProperty("cache.supported") ?
> +                                               document.getElementById("cache").checked : false);

I believe you should check !== false here. If cache.supported is not explicitly set, it should default to true.

Maybe it would make sense to also (or instead) do something like this in the calendar manager: when cache.supported is false, don't wrap the calendar in the cache wrapper, even if cache.enabled is true.

This would guard us if there are other places where calendars are created, e.g. in extensions.
Comment 8 User image [:MakeMyDay] 2015-07-15 14:18:36 PDT
Created attachment 8634374 [details] [diff] [review]
FixAccidentallyEnabledCachingIfCacheIsUnsupported-V2.diff

Updated patch with comments considered, but I haven't added the check to CalendarManager for now. The patch will take care to prevent data loss and remove inappropriate cache enabling anyway.
Comment 9 User image Philipp Kewisch [:Fallen] 2015-07-16 05:16:44 PDT
Pushed to comm-central changeset b88d84e8d337
Comment 10 User image Philipp Kewisch [:Fallen] 2015-07-16 05:25:47 PDT
Backported to releases/comm-aurora changeset 89a926b8c66f
Comment 11 User image Philipp Kewisch [:Fallen] 2015-07-16 05:30:02 PDT
Backported to releases/comm-beta changeset 3144fac037fa
Comment 12 User image Philipp Kewisch [:Fallen] 2015-07-16 05:30:53 PDT
Note this bug is NOT part of 4.1, since we never really had a beta release for it.
Comment 13 User image Philipp Kewisch [:Fallen] 2015-07-16 05:33:22 PDT
Backported to releases/comm-esr38 changeset f8f6f4767a76
Comment 14 User image Philipp Kewisch [:Fallen] 2015-07-16 05:35:44 PDT
...and will include manually in 4.0.1.1
Comment 15 User image Robert Brand 2015-07-23 10:06:10 PDT
With Lightning 4.0.1.2 (from FTP) installed in TB 38.1.0, there's something strange: When I check "Offline Support" for my Baikal calendar (https://www.nadelundhirn.de/....), the box is unchecked when I open calendar properties once again. This doesn't happen with Lightning 4.0.1. I can't do more testing now because I really have to leave. Could you check once again if the patch for this bug works as expected?
Comment 16 User image [:MakeMyDay] 2015-07-23 14:35:16 PDT
See bug 1187072.

Note You need to log in before you can comment on or make changes to this bug.