758585 - Fix all creations of nsITimer that are locally-scoped and susceptible to GC before firing, in Calendar

Status: RESOLVED FIXED

(Calendar :: General, defect)

Description

[Serge Gautherie (:sgautherie)]

It looks like at least some SeaMonkey tests would be affected...
(I didn't check further.)

Comment 1

[Wayne Mery (:wsmwk)]

based on the issues covered at bug 640629 and friends, it seems to me like this should be fairly straighforward, and be important and prioritized for us.

Comment 2

[jbonnafo]

Hello,

I would be happy to help on this issue.
Could you please assign it to me ?

Thanks,

Comment 3

[jbonnafo]

Hello,

Could you please tell me if an appropriate fix would be to have class members for nsITimer instead of local variables declared with 'let' or 'var'.

Thanks,

Comment 4

[Jorg K (CEST = GMT+2)]

I don't actually know what this bug is all about. I don't even understand the abbreviation "GC" in the summary. Is this a continuation of bug 640629?

By the looks of it, nsITimer is defined in M-C, so I assume we don't want to modify it:
searchfox.org/mozilla-central/source/xpcom/threads/nsITimer.idl

Aceman, Magnus, do you know more?

Comment 5

[jbonnafo]

Hello,

I think GC is for Garbage Collection. in Bug 640629, the proposed patch was to replace in JS code local variables of type nsITimer by classes data member.

I want to be sure this is the route to follow for this issue.

Thanks,

Comment 6

[Magnus Melin [:mkmelin]]

Yes GC is Garbage Collection. 
And yes that is the path to follow.

Comment 7

[jbonnafo]

Hello,

Thanks for the info.
Should i prepare a patch for all files affected (lots of files involved) or can i start with a sub set (eg: JS files for "calendar").

Thanks,

Comment 8

[Jorg K (CEST = GMT+2)]

It's better to split the work into parts. Different parts will need different reviewers. So parts for calendar/, mail/, mailnews/, suite/ (if applicable) would be best, or any subdivision thereof you deem fit.

Comment 9

[jbonnafo]

Attachment: bug758585-v1.patch

First patch proposal, fixing local timers in the calendar folder.
Please note that i am quite new to JS.
Thanks,

Comment 10

[Jorg K (CEST = GMT+2)]

Comment on attachment 8822141 [details] [diff] [review]
bug758585-v1.patch

Thanks, that looks good. The indentation looks off here:
+	mTimer: null,
Perhaps you used tabs. We only use spaces.

Who's the right reviewer for this?

Comment 11

[jbonnafo]

Attachment: bug758585-v2.patch

Second patch proposal for calendar folder.
Fixed the use of TAB.

Comment 12

[Jorg K (CEST = GMT+2)]

Sorry, there is still some wrong indentation at
  _timer: null,

Also, in Thunderbird is organised into modules with owners and peers. Only those are allowed to do reviews. I can't review calendar changes, but MakeMyDay can.

When you have more patches, I'll find the right reviewer for them, OK?

Comment 13

[jbonnafo]

Attachment: bug758585-v3.patch

Third patch proposal, fixing indentation.

Comment 14

[Jorg K (CEST = GMT+2)]

This must seem tedious to you, but there is still a tab in
+	 _timer: null,
Doesn't you editor show them?

Also, our coding standard says that comments need to be full sentences, where possible, terminated with a full stop. So instead of
  use global scope to prevent timer from being GC'ed
  Use global scope to prevent timer from being GC'ed.
Also, I had trouble understanding "GC", so perhaps better:
  Use global scope to prevent timer from being garbage-collected.
That makes it so long that it should go onto a line by itself.

Thanks for your contribution and patience. We just need to make sure patches conform to the coding standard.

Comment 15

[jbonnafo]

Attachment: bug758585-v4.patch

Patch version fixing tab and modifying comment.

Comment 16

[Jorg K (CEST = GMT+2)]

Great. Thanks. The next patches will the spot on from the beginning ;-)

Comment 17

[jbonnafo]

Thanks Jorg for spotting mistakes/errors in my patch proposal.

What are the next steps ? 
IMHO i should wait for a module owner approval before starting to tackle the next bundle of files. :-)

Thanks,

Comment 18

[Jorg K (CEST = GMT+2)]

I think you could prepare a few more patches, say something for mail/. The reviewer for Calendar, MakeMyDay, will most likely look at your patch in the next couple of days. He might be slower due to the holiday season.

Comment 19

[[:MakeMyDay]]

Comment on attachment 8822157 [details] [diff] [review]
bug758585-v4.patch

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

Thanks for working on this bug. Please see my comments below. r- for now to get an updated patch.

::: calendar/base/modules/calProviderUtils.jsm
@@ +147,5 @@
>   */
>  cal.BadCertHandler = function(thisProvider) {
>      this.thisProvider = thisProvider;
>  };
> +var t; // Use global scope to prevent timer from being garbage-collected.

Why can't you make this a property of BadCertHandler here? Defining timer: null in the prototyp and assigning the timer to this.timer then should suffice.

::: calendar/base/src/calAlarmService.js
@@ +24,3 @@
>                             aDelay,
>                             (aRepeating ? timer.TYPE_REPEATING_PRECISE : timer.TYPE_ONE_SHOT));
> +    return this.mTimer;

newTimerWithCallback is used twice in this file. You should extend the function in a way that you can pass in the timer variable and use that to assign the timer to it. With your current approach one timer may be overwritten by another.

Regarding style, please also take care to fix the indentation if you change a multi-line peace of code.

@@ +111,5 @@
>      mStarted: false,
>      mTimerMap: null,
>      mObservers: null,
>      mTimezone: null,
> +    mTimer: null,

See above - you should use this.mUpdateTimer and mTimerMap to hold the respective timers.

::: calendar/providers/gdata/modules/shim/Timer.jsm
@@ +9,3 @@
>                            .createInstance(Components.interfaces.nsITimer);
>  
> +    _timer.initWithCallback({ notify: func }, timeout, timer.TYPE_ONE_SHOT);

Leave out the gdata shim completely in this patch, as this is subject to removal with bug 1323332 anyway.

Comment 20

[jbonnafo]

Thanks for this detailed review of proposed patch. 
I will work on a new patch version following your comments.

Comment 21

[jbonnafo]

Attachment: bug758585-v5.patch

Please find a new parch proposal following the previous review.
I have tried to follow comments from that review.
Thanks,

Comment 22

[[:MakeMyDay]]

Comment on attachment 8822724 [details] [diff] [review]
bug758585-v5.patch

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

Thanks for preparing the patch. In general, this looks good. Just one style nit, I forgot to mention in my previous review - sorry for that. r+ with that fixed.

One additional remark: if you plan to upload patches for different parts of CC in the same bug (which require different reviewers accordingly), as I understand you want to do here, it is good practise to indicate the part of cc the patch is for in the patch name (calendar in this case) to make it easier to distinguish the patches.

::: calendar/base/modules/calProviderUtils.jsm
@@ +184,5 @@
> +        this.timer = Components.classes["@mozilla.org/timer;1"]
> +                               .createInstance(Components.interfaces.nsITimer);
> +        this.timer.initWithCallback(timerCallback,
> +                                    0,
> +                                    Components.interfaces.nsITimer.TYPE_ONE_SHOT);

Can you please convert this to

this.timer.initWithCallback(
    timerCallback,	
    0,
    Components.interfaces.nsITimer.TYPE_ONE_SHOT
);	

And use the similar pattern for other occurrcences of initWithCallback and newTimerWithCallback in this patch.

Comment 23

[jbonnafo]

Attachment: Bug 758585 - Fix all creations of nsITimer that are locally-scoped, module: calendar, review:makemyday

Hello,
Thanks for your review. I have amended the patch proposal following your comments.
Thanks,

Comment 24

[[:MakeMyDay]]

Comment on attachment 8822901 [details] [diff] [review]
Bug 758585 - Fix all creations of nsITimer that are locally-scoped, module: calendar, review:makemyday

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

Thanks for your endurance, you're almost there - just two comments remaining ;-) and r+ with that fixed. Sorry for being nit-picking here, but we recently did a major code reformatting for calendar.

Please also append r=makemyday to the commit message of the patch, this makes it easier for others to checkin the patch for you. With that, uploading the fixed patch is sufficient, no need to ask for review again, you or others can carry forward the r+.

Once again, thank you for your work, much apprreciated!

::: calendar/base/src/calAlarmService.js
@@ +263,5 @@
> +        newTimerWithCallback(
> +        timerCallback,
> +        kHoursBetweenUpdates * 3600000,
> +        true,
> +        this.mUpdateTimer

Your missing the indentation here:

newTimerWithCallback(
    timerCallback,
    kHoursBetweenUpdates * 3600000,
    true,
    this.mUpdateTimer
);

@@ +443,5 @@
> +        alarmTimerCallback,
> +        aTimeout,
> +        false
> +        this.mTimerMap[aItem.calendar.id][aItem.hashId][aAlarm.icalString]
> +        );

Same here.

Comment 25

[Jorg K (CEST = GMT+2)]

Comment on attachment 8822901 [details] [diff] [review]
Bug 758585 - Fix all creations of nsITimer that are locally-scoped, module: calendar, review:makemyday

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

Drive-by comment.

::: calendar/base/src/calAlarmService.js
@@ +16,5 @@
>      return cal.jsDateToDateTime(new Date()).getInTimezone(cal.UTC());
>  }
>  
> +function newTimerWithCallback(aCallback, aDelay, aRepeating, aTimer) {
> +    aTimer = Components.classes["@mozilla.org/timer;1"]

Are you sure this is right? My very humble JS skills tell me that aTimer (being this.mUpdateTimer) is passed by value here and whatever you assign will be garbage-collected when the function exits. If you passed 'this' (by reference), then you could go:
aObject.mUpdateTimer = ...

Comment 26

[jbonnafo]

Hello,

I am also very new to JS.

From what i found on the web:

* Javascript is always pass by value, but when a variable refers to an object (including arrays), the "value" is a reference to the object.
* Changing the value of a variable never changes the underlying primitive or object, it just points the variable to a new primitive or object.
* However, changing a property of an object referenced by a variable does change the underlying object.

Let's see what makemyday thinks.

Happy new year 2017,

Comment 27

[Jorg K (CEST = GMT+2)]

Indeed:
http://stackoverflow.com/questions/518000/is-javascript-a-pass-by-reference-or-pass-by-value-language
My conclusion from the example is that your patch isn't right. What's your conclusion?

Comment 28

[jbonnafo]

Well, i also think there is something wrong, let's see what makemyday thinks.

Comment 29

[jbonnafo]

Hello,

I think that instead of using newTimerWithCallback function, i could simply use direct calls to assign timers exactly the way i did for some other JS files.

Thanks,

Comment 30

[[:MakeMyDay]]

Sorry for the delay in replying, it has been a quite busy start into 2017.

I think the patch will work as is (apart from the style nits). You can test this on your self build TB, if you setup some event reminders and see wether they get fired appropriately.

But to play safe here, Philipp, can you please take a look at the patch, too?

Comment 31

[jbonnafo]

Hello,

I am going to test with my build of TB. Does the calendar / event stuff available from simple build or should i use some dedicated options or add-on ?

Thanks,

Comment 32

[[:MakeMyDay]]

It's enough to get Lightning if you build with

ac_add_options --enable-calendar

in your mozconfig.

Comment 33

[Jorg K (CEST = GMT+2)]

(In reply to [:MakeMyDay] from comment #30)
> I think the patch will work as is (apart from the style nits). You can test
> this on your self build TB, if you setup some event reminders and see wether
> they get fired appropriately.
Is this really a valid test? The software works now although the timers might get garbage-collected as some stage. So even if it works, that doesn't prove that the code is right. As I said in comment #25, an assignment to a "scalar" parameter doesn't carry the value outside. A valid test would be to dump out the value in the function and after the call in the caller.

(In reply to jbonnafo from comment #31)
> I am going to test with my build of TB. Does the calendar / event stuff
> available from simple build or should i use some dedicated options or add-on
You need to have
  ac_add_options --enable-calendar
in your .mozconfig (or mozconfig). Have to built TB before? With the arrival of Rust it's a little more complicated now, but
  ac_add_options --disable-rust
  unset RUSTC
  unset CARGO
should take care of it.

Comment 34

[[:MakeMyDay]]

Yes, but what is passed in here is always the property of an object, so the timer is bound to the outside object and therefor shouldn't be garbage collected that way, imho.

Comment 35

[:aceman]

AFAIK argument passing in JS works by value. Only if you pass in an object, you can change its internals and that survises end of the function.

A quick test in FF's scratchpad shows that with a code like:
obj = { x:1, y:2 };
function alter(z) {
  z = 4;
}
alter(obj.y);

Value of obj.y at the end is still 2, not 4.

Comment 36

[[:MakeMyDay]]

Ok, you guys convinced me - thanks for being persistent here.

Jbonnafo, can you please convert newTimerWithCallback to a _newTimerWithCallback method of the object and pass in an distinguishing argument instead of aTimer to decide whether to this.mUpdateTimer or the other property?

Comment 37

[jbonnafo]

Attachment: Bug 758585 - Fix all creations of nsITimer that are locally-scoped, module: calendar, review:makemyday

Hello,

Please find a new patch proposal following comments.

Thanks,

Comment 38

[[:MakeMyDay]]

Comment on attachment 8826857 [details] [diff] [review]
Bug 758585 - Fix all creations of nsITimer that are locally-scoped, module: calendar, review:makemyday

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

Thanks for your work, your almost there. Still some style nits, but r+ with that fixed.

::: calendar/base/modules/calProviderUtils.jsm
@@ +147,5 @@
>   */
>  cal.BadCertHandler = function(thisProvider) {
>      this.thisProvider = thisProvider;
>  };
> +

Remove the blank line here

::: calendar/base/src/calAlarmService.js
@@ +194,5 @@
>  
>      removeObserver: function(aObserver) {
>          this.mObservers.remove(aObserver);
>      },
> +    

Please remove the whitespaces at the start of the line above.

@@ +195,5 @@
>      removeObserver: function(aObserver) {
>          this.mObservers.remove(aObserver);
>      },
> +    
> +    newTimerWithCallback: function (aCallback, aDelay, aRepeating, aMode, aItem, aAlarm) {

please make this _newTimerWithCallback and remove the whitespace between function key word and the opening parenthesis.

@@ +199,5 @@
> +    newTimerWithCallback: function (aCallback, aDelay, aRepeating, aMode, aItem, aAlarm) {
> +        if (aMode) {
> +            this.mTimerMap[aItem.calendar.id][aItem.hashId][aAlarm.icalString] =
> +                Components.classes["@mozilla.org/timer;1"]
> +                .createInstance(Components.interfaces.nsITimer);

Please make this
this.mTimerMap[aItem.calendar.id][aItem.hashId][aAlarm.icalString] =
    Components.classes["@mozilla.org/timer;1"]
              .createInstance(Components.interfaces.nsITimer);

@@ +201,5 @@
> +            this.mTimerMap[aItem.calendar.id][aItem.hashId][aAlarm.icalString] =
> +                Components.classes["@mozilla.org/timer;1"]
> +                .createInstance(Components.interfaces.nsITimer);
> +            this.mTimerMap[aItem.calendar.id][aItem.hashId][aAlarm.icalString]
> +            .initWithCallback(

also here:

this.mTimerMap
    .initWithCallBack

@@ +204,5 @@
> +            this.mTimerMap[aItem.calendar.id][aItem.hashId][aAlarm.icalString]
> +            .initWithCallback(
> +                aCallback,
> +                aDelay,
> +                (aRepeating ? timer.TYPE_REPEATING_PRECISE : timer.TYPE_ONE_SHOT));

And move the closing parenthesis to the next line. You don't need parentheses around aRepeating ? timer.TYPE_REPEATING_PRECISE : timer.TYPE_ONE_SHOT

@@ +209,5 @@
> +
> +        } else {
> +            this.mUpdateTimer = Components.classes["@mozilla.org/timer;1"]
> +                .createInstance(
> +                    Components.interfaces.nsITimer);

Please make this

this.mUpdateTimer = Components.classes["@mozilla.org/timer;1"]
                              .createInstance(Components.interfaces.nsITimer);

@@ +215,5 @@
> +            (aCallback,
> +                aDelay,
> +                (aRepeating
> +                     ? timer.TYPE_REPEATING_PRECISE
> +                     : timer.TYPE_ONE_SHOT));

this.mUpdateTimer.initWithCallback(
    aCallback,
    aDelay,
    aRepeating
        ? timer.TYPE_REPEATING_PRECISE
        : timer.TYPE_ONE_SHOT
);

@@ +276,5 @@
> +        this.newTimerWithCallback(
> +            timerCallback,
> +            kHoursBetweenUpdates * 3600000,
> +            true,
> +            false);

Please move the closing parenthesis to the next line:

    true,
    false
);

@@ +456,5 @@
> +            aTimeout,
> +            false,
> +            true,
> +            aItem,
> +            aAlarm);

Again, please move the closing parenthesis to the next line.

Comment 39

[jbonnafo]

Attachment: Bug 758585 - Fix all creations of nsITimer that are locally-scoped, module: calendar, review:makemyday

Thanks for reviewing my patch proposal.
Please find attached a new version trying to fix all items raised.

Quick question: is there any tool available that automatically formats JS code according to the mozilla standard ?

Thanks,

Comment 40

[jbonnafo]

Hello,

Another question, since this bug will involve more than one patch (a least one per module), should i request code check-in each time a patch is Ok or should i wait to have all patches ready and approved ?

Thanks,

Comment 41

[Jorg K (CEST = GMT+2)]

No fixed rules, usually patches get checked in when they are ready.

If would be good if you could name your patches accordingly, or even open multiple bugs.

Example for the first approach:
Bug 1303722. Here the patches are named according to the directory they apply to.

Example for the second approach:
bug 1293006, bug 1293007, bug 1293008.

Comment 42

[[:MakeMyDay]]

Comment on attachment 8827275 [details] [diff] [review]
Bug 758585 - Fix all creations of nsITimer that are locally-scoped, module: calendar, review:makemyday

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

Sorry for the delay. You're almost there, just some remaining formatting issues in _newTimerWithCallback need to be fixed. If you upload an updated patch with that fixed, this patch will be ready for checkin.

For JS formatting, for calendar we have implemented ESlint rules  (for Mail and Chat there are currently none, iirc) and done a code re-formatting, however other then for m-c, it still is quite hard to run the linter for c-c.

::: calendar/base/src/calAlarmService.js
@@ +219,5 @@
> +                     : timer.TYPE_ONE_SHOT
> +            );
> +        }
> +    },
> +

There are still some indentation issues within _newTimerWithCallback. Please make that:

    _newTimerWithCallback: function(aCallback, aDelay, aRepeating, aMode, aItem, aAlarm) {
        if (aMode) {
            this.mTimerMap[aItem.calendar.id][aItem.hashId][aAlarm.icalString] =
                Components.classes["@mozilla.org/timer;1"]
                          .createInstance(Components.interfaces.nsITimer);
            this.mTimerMap[aItem.calendar.id][aItem.hashId][aAlarm.icalString]
                .initWithCallback(
                    aCallback,
                    aDelay,
                    aRepeating ? timer.TYPE_REPEATING_PRECISE : timer.TYPE_ONE_SHOT
                );
        } else {
            this.mUpdateTimer = Components.classes["@mozilla.org/timer;1"]
                                          .createInstance(Components.interfaces.nsITimer);
            this.mUpdateTimer.initWithCallback (
                aCallback,
                aDelay,
                aRepeating ? timer.TYPE_REPEATING_PRECISE
                           : timer.TYPE_ONE_SHOT
            );
        }
    },

Comment 43

[jbonnafo]

Attachment: Bug 758585 - Fix all creations of nsITimer that are locally-scoped, module: calendar, review:makemyday

Hello,
Thanks for the code review. Please find attached a new patch proposal.
Thanks,

Comment 44

[[:MakeMyDay]]

Comment on attachment 8833780 [details] [diff] [review]
Bug 758585 - Fix all creations of nsITimer that are locally-scoped, module: calendar, review:makemyday

Mission accomplished \o/ Thanks for the patch!

If you prefer to get all patches checked in at once, you can just start the next one right now. If you want the patches to get landed as they are ready, you can set the keywords checkin-needed and leave-open (until the last patch gets ready).

Comment 45

[Jorg K (CEST = GMT+2)]

Comment on attachment 8833780 [details] [diff] [review]
Bug 758585 - Fix all creations of nsITimer that are locally-scoped, module: calendar, review:makemyday

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

::: calendar/base/src/calAlarmService.js
@@ +196,5 @@
>              newParent.deleteProperty("X-MOZ-SNOOZE-TIME");
>          }
>          return newParent.calendar.modifyItem(newParent, oldParent, null);
>      },
> +    

Nit: Trailing space. I'll fix this when checking it in (which can be delayed due to current bustage).

Comment 46

[Jorg K (CEST = GMT+2)]

I've come to check this in and before I do, I usually look at the patches. This still doesn't seem right.

In calAlarmService.js you define a new function _newTimerWithCallback() but you don't remove the old one newTimerWithCallback() which is now unused.

MMD?

Comment 47

[jbonnafo]

Attachment: bug758585-v11.patch

Please find attached a new patch proposal, removing unused code and fixing trailing space.
Thanks,

Comment 48

[[:MakeMyDay]]

Comment on attachment 8836406 [details] [diff] [review]
bug758585-v11.patch

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

Jörg, thanks for the cross check and jbonnafo for the updated patch. r+ with the whitespaces removed.

::: calendar/base/src/calAlarmService.js
@@ +186,5 @@
>              newParent.deleteProperty("X-MOZ-SNOOZE-TIME");
>          }
>          return newParent.calendar.modifyItem(newParent, oldParent, null);
>      },
> +    

The whitespaces are still there. Please recheck.

Comment 49

[Jorg K (CEST = GMT+2)]

Don't worry about the trailing spaces, I'll remove them when checking this in RSN(TM).
(If you don't get the pun: Real Soon Now)

Comment 50

[Jorg K (CEST = GMT+2)]

I've come to check this in and before I do, I usually look at the patches. This still doesn't seem right.

Why are the changes in calAlarmService.js necessary?

The previous code was:
this.mUpdateTimer = newTimerWithCallback(timerCallback, kHoursBetweenUpdates * 3600000, true);
and
let timer = newTimerWithCallback(alarmTimerCallback, aTimeout, false);
this.mTimerMap[aItem.calendar.id][aItem.hashId][aAlarm.icalString] = timer;

There is not risk of GC since the object holds a reference to the timer. The new function doesn't do anything else. The 'aMode' parameter isn't exactly elegant and it's also not documented. I think you're better off not changing anything in this file. If this were my code, I wouldn't accept this change.

Comment 51

[Jorg K (CEST = GMT+2)]

Attachment: bug758585-v12.patch (JK).

This is all that's required.

Comment 53

[[:MakeMyDay]]

Comment on attachment 8839151 [details] [diff] [review]
bug758585-v12.patch (JK).

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

Sorry, I forgot about this. You're right, we only need to change these occurences.

Comment 54

[Jorg K (CEST = GMT+2)]

Good, I'll land this and then we should move on to other modules needing the same treatment since the bug set out to fix all of comm-central.

Jean Luc, are you still interested? I can take the review in all other areas of comm-central within 48 hours of requesting it.

Comment 55

[Jorg K (CEST = GMT+2)]

Is there a recipe to find the ones that need fixing? I guess we look at
https://dxr.mozilla.org/comm-central/search?q=mozilla.org%2Ftimer&redirect=true
and when we see
  this.timer = Cc["@mozilla.org/timer;1"].createInstance()
there isn't any need to fix anything, so we're left with the ones that don't have the 'this.'.
In C++ code,
  mTimer = do_CreateInstance("@mozilla.org/timer;1", &rv);
indicated assignment to a member variable, so that should be OK already.

There appear very few cases left that need fixing.

Comment 56

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/ad44eb6ab134
Fix all creations of nsITimer that are locally-scoped, module: calendar. r=MakeMyDay

Comment 57

[jbonnafo]

Hello,

Thanks a lot for your patch, please feel free to move forward on this topic.

Thanks,

Comment 58

[Jorg K (CEST = GMT+2)]

2017-10-24 makes it Thunderbird 58 or Calendar 6.0.