Last Comment Bug 1187072 - Cache support gets reset disabled for network calendars in calendar property dialog
: Cache support gets reset disabled for network calendars in calendar property ...
Status: RESOLVED FIXED
: regression
Product: Calendar
Classification: Client Software
Component: Dialogs (show other bugs)
: Lightning 4.0.1.2
: Unspecified Unspecified
-- normal (vote)
: 4.0.2
Assigned To: [:MakeMyDay]
:
:
Mentors:
Depends on:
Blocks: ltn402
  Show dependency treegraph
 
Reported: 2015-07-23 14:30 PDT by [:MakeMyDay]
Modified: 2015-08-10 08:07 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
StrictCheckForCacheSupport-V1.diff (1.32 KB, patch)
2015-07-23 14:33 PDT, [:MakeMyDay]
philipp: approval‑calendar‑aurora+
philipp: approval‑calendar‑beta+
philipp: approval‑calendar‑esr+
Details | Diff | Splinter Review
StrictCheckForCacheSupport-V2.diff (1.32 KB, patch)
2015-07-26 00:34 PDT, [:MakeMyDay]
philipp: review+
philipp: approval‑calendar‑aurora+
philipp: approval‑calendar‑beta+
philipp: approval‑calendar‑esr+
Details | Diff | Splinter Review

Description User image [:MakeMyDay] 2015-07-23 14:30:34 PDT
From bug 1182264 #15:

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 1 User image [:MakeMyDay] 2015-07-23 14:33:49 PDT
Created attachment 8638153 [details] [diff] [review]
StrictCheckForCacheSupport-V1.diff

Obvious fix, but untested yet.
Comment 2 User image Robert Brand 2015-07-24 12:39:50 PDT
With your patch applied, everything is working fine for me.
Comment 3 User image [:MakeMyDay] 2015-07-25 01:41:42 PDT
Comment on attachment 8638153 [details] [diff] [review]
StrictCheckForCacheSupport-V1.diff

Thanks for testing. I also had a brief test with webdav and caldav calendars meanwhile, so this patch does the trick.

I'm not sure whether we need an 4.0.1.3 for this or just can take it for 4.0.2. The bug would disable cache for users, who are editing the properties of a caldav or webdav calendar (these are the providers in cc, which not explicitely set the cache.supported property, but just fall back to default value, which is true) or eventually external providers like EWS (but not sure about this).
Comment 4 User image Philipp Kewisch [:Fallen] 2015-07-25 14:31:54 PDT
Comment on attachment 8638153 [details] [diff] [review]
StrictCheckForCacheSupport-V1.diff

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

I was looking at the code, and another potential issue came to mind:

In changeCalendarCache, if the cache is not support, setProperty gets called to change cache.enabled to false. This in turn might trigger onPropertyChanged, which calls changeCalendarCache. Wouldn't there be an endless loop?

I've been trying to get comm-central builds running again for the last few hours without success, so I haven't been able to test this too. One of us should thoroughly test the patch before we push it. I guess we'll have to create a 4.0.1.3 for this.

Leaving r? until it is tested and the infinite loop issue is discussed, but aside from that r+.

::: calendar/base/src/calCalendarManager.js
@@ +836,5 @@
>          aValue = aValue || false;
>  
>          // hack for bug 1182264 to deal with calendars, which have set cache.enabled, but in fact do
>          // not support caching (like storage calendars) - this also prevents enabling cache again
> +        if (aCalendar.getProperty('cache.supported') === false) {

While you are here can you switch to double quotes?
Comment 5 User image [:MakeMyDay] 2015-07-26 00:34:26 PDT
Created attachment 8638988 [details] [diff] [review]
StrictCheckForCacheSupport-V2.diff

Updated patch with double qoutes.

> In changeCalendarCache, if the cache is not support, setProperty gets called
> to change cache.enabled to false. This in turn might trigger
> onPropertyChanged, which calls changeCalendarCache. Wouldn't there be an
> endless loop?

I didn't expirienced such when testing the patch for bug 1182264, but I have checked again with yesterdays Daily and this patch applied - everything runs smoothly.

Codewise, it looks not obvious at a first glance, but there can't be a loop - onPropertyChanged gets only fired if the property is really changed, while setting the same value again is ignored - see [1].

[1] http://mxr.mozilla.org/comm-central/source/calendar/base/modules/calProviderUtils.jsm#763
Comment 6 User image [:MakeMyDay] 2015-07-26 12:47:10 PDT
Philipp, let's wait with 4.0.1.3 at least until the reporter on bug 1187624 provides feedback - I suspect this is related. If so, the patch here is not enough but we need separate migration code to disable cache for storage calendars automatically and not just on user action like in this patch.
Comment 7 User image Philipp Kewisch [:Fallen] 2015-07-28 07:38:08 PDT
Comment on attachment 8638988 [details] [diff] [review]
StrictCheckForCacheSupport-V2.diff

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

r=philipp

::: calendar/base/src/calCalendarManager.js
@@ +841,5 @@
>              aCalendar.setProperty("cache.enabled", false);
>              return;
>          }
>  
>          if (aOldValue != aValue) {

Ok, I guess I was fooled by this check.
Comment 8 User image Philipp Kewisch [:Fallen] 2015-08-04 08:26:16 PDT
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a142390a876f
Comment 9 User image Philipp Kewisch [:Fallen] 2015-08-04 08:27:01 PDT
Sorry about that extra try push message, unrelated to this bug.
Comment 11 User image Philipp Kewisch [:Fallen] 2015-08-08 03:26:54 PDT
url:        https://hg.mozilla.org/releases/comm-aurora/rev/46f923a77f83e2c49ef6d9d19e9cd7f3b91af419
changeset:  46f923a77f83e2c49ef6d9d19e9cd7f3b91af419
user:       MakeMyDay <makemyday@gmx-topmail.de>
date:       Fri Aug 07 11:32:31 2015 +0200
description:
Bug 1187072 - Cache support gets reset disabled for network calendars in calendar property dialog;r+a=philipp
Comment 12 User image Philipp Kewisch [:Fallen] 2015-08-08 03:34:39 PDT
url:        https://hg.mozilla.org/releases/comm-beta/rev/6a175eb77cd2b68d1211bd6f7c9df31bed9240fe
changeset:  6a175eb77cd2b68d1211bd6f7c9df31bed9240fe
user:       MakeMyDay <makemyday@gmx-topmail.de>
date:       Fri Aug 07 11:32:31 2015 +0200
description:
Bug 1187072 - Cache support gets reset disabled for network calendars in calendar property dialog;r+a=philipp
Comment 13 User image Philipp Kewisch [:Fallen] 2015-08-08 03:40:42 PDT
url:        https://hg.mozilla.org/releases/comm-esr38/rev/0be9b2ead498e11d90ce858a509e2b0a15adadd1
changeset:  0be9b2ead498e11d90ce858a509e2b0a15adadd1
user:       MakeMyDay <makemyday@gmx-topmail.de>
date:       Fri Aug 07 11:32:31 2015 +0200
description:
Bug 1187072 - Cache support gets reset disabled for network calendars in calendar property dialog;r+a=philipp

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