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

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.

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

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,

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?

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,

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

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,

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.

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

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?

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

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?

Third patch proposal, fixing indentation.

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.

Patch version fixing tab and modifying comment.

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

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,

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 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.

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

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

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.

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

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 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 = ...

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,

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?

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

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,

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?

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,

It's enough to get Lightning if you build with ac_add_options --enable-calendar in your mozconfig.

(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.

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.

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.

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?

Hello, Please find a new patch proposal following comments. Thanks,

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.

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,

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,

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 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 ); } },

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

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 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).

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?

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

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.

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)

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.

This is all that's required.

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.

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.

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.

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

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

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