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

RESOLVED FIXED in 6.0

Status

Calendar
General
RESOLVED FIXED
6 years ago
5 months ago

People

(Reporter: sgautherie, Assigned: jbonnafo)

Tracking

(Depends on: 2 bugs)

Dependency tree / graph

Details

(URL)

Attachments

(1 attachment, 10 obsolete attachments)

4.35 KB, patch
MakeMyDay
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
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.
(Assignee)

Comment 2

a year ago
Hello,

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

Thanks,
Assignee: nobody → jeanluc.bonnafoux
(Assignee)

Comment 3

a year ago
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?
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(acelists)
(Assignee)

Comment 5

a year ago
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

a year ago
Yes GC is Garbage Collection. 
And yes that is the path to follow.
Flags: needinfo?(mkmelin+mozilla)
(Assignee)

Comment 7

a year ago
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.
(Assignee)

Comment 9

a year ago
Created attachment 8822141 [details] [diff] [review]
bug758585-v1.patch

First patch proposal, fixing local timers in the calendar folder.
Please note that i am quite new to JS.
Thanks,
Attachment #8822141 - Flags: review?(kaie)
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?
Attachment #8822141 - Flags: review?(kaie) → review?(makemyday)
Flags: needinfo?(acelists)
(Assignee)

Comment 11

a year ago
Created attachment 8822148 [details] [diff] [review]
bug758585-v2.patch

Second patch proposal for calendar folder.
Fixed the use of TAB.
Attachment #8822141 - Attachment is obsolete: true
Attachment #8822141 - Flags: review?(makemyday)
Attachment #8822148 - Flags: review?(jorgk)
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?
(Assignee)

Comment 13

a year ago
Created attachment 8822153 [details] [diff] [review]
bug758585-v3.patch

Third patch proposal, fixing indentation.
Attachment #8822148 - Attachment is obsolete: true
Attachment #8822148 - Flags: review?(jorgk)
Attachment #8822153 - Flags: review?(makemyday)
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.
(Assignee)

Comment 15

a year ago
Created attachment 8822157 [details] [diff] [review]
bug758585-v4.patch

Patch version fixing tab and modifying comment.
Attachment #8822153 - Attachment is obsolete: true
Attachment #8822153 - Flags: review?(makemyday)
Attachment #8822157 - Flags: review?(makemyday)
Great. Thanks. The next patches will the spot on from the beginning ;-)
(Assignee)

Comment 17

a year ago
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 19

a year ago
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.
Attachment #8822157 - Flags: review?(makemyday) → review-
(Assignee)

Comment 20

a year ago
Thanks for this detailed review of proposed patch. 
I will work on a new patch version following your comments.
(Assignee)

Comment 21

a year ago
Created attachment 8822724 [details] [diff] [review]
bug758585-v5.patch

Please find a new parch proposal following the previous review.
I have tried to follow comments from that review.
Thanks,
Attachment #8822157 - Attachment is obsolete: true
Attachment #8822724 - Flags: review?(makemyday)

Comment 22

a year ago
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.
Attachment #8822724 - Flags: review?(makemyday) → review+
(Assignee)

Comment 23

a year ago
Created attachment 8822901 [details] [diff] [review]
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,
Attachment #8822724 - Attachment is obsolete: true
Attachment #8822901 - Flags: review?(makemyday)

Comment 24

a year ago
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.
Attachment #8822901 - Flags: review?(makemyday) → review+
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 = ...
Flags: needinfo?(makemyday)
(Assignee)

Comment 26

a year ago
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?
(Assignee)

Comment 28

a year ago
Well, i also think there is something wrong, let's see what makemyday thinks.
(Assignee)

Comment 29

a year ago
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,
(Assignee)

Updated

a year ago
Flags: needinfo?(vseerror)

Comment 30

a year ago
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?
Flags: needinfo?(vseerror)
Flags: needinfo?(philipp)
Flags: needinfo?(makemyday)
(Assignee)

Comment 31

a year ago
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

a year ago
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.

Comment 34

a year ago
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

a year ago
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

a year ago
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?
Flags: needinfo?(philipp)
(Assignee)

Updated

a year ago
Status: NEW → ASSIGNED
(Assignee)

Comment 37

a year ago
Created attachment 8826857 [details] [diff] [review]
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,
Attachment #8826857 - Flags: review?(makemyday)

Comment 38

a year ago
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.
Attachment #8826857 - Flags: review?(makemyday) → review+

Updated

a year ago
Attachment #8822901 - Attachment is obsolete: true
(Assignee)

Comment 39

a year ago
Created attachment 8827275 [details] [diff] [review]
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,
Attachment #8826857 - Attachment is obsolete: true
Attachment #8827275 - Flags: review?(makemyday)
(Assignee)

Comment 40

a year ago
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 42

a year ago
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
            );
        }
    },
Attachment #8827275 - Flags: review?(makemyday) → review+
(Assignee)

Comment 43

a year ago
Created attachment 8833780 [details] [diff] [review]
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,
Attachment #8827275 - Attachment is obsolete: true
Attachment #8833780 - Flags: review?(makemyday)

Comment 44

a year ago
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).
Attachment #8833780 - Flags: review?(makemyday) → review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed, leave-open
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?
Flags: needinfo?(makemyday)
Keywords: checkin-needed
(Assignee)

Comment 47

a year ago
Created attachment 8836406 [details] [diff] [review]
bug758585-v11.patch

Please find attached a new patch proposal, removing unused code and fixing trailing space.
Thanks,
Attachment #8833780 - Attachment is obsolete: true
Attachment #8836406 - Flags: review?(makemyday)

Comment 48

a year ago
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.
Attachment #8836406 - Flags: review?(makemyday) → review+
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)
Flags: needinfo?(makemyday)
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.
Flags: needinfo?(makemyday)
Created attachment 8839151 [details] [diff] [review]
bug758585-v12.patch (JK).

This is all that's required.
Attachment #8839151 - Flags: review?(makemyday)
Duplicate of this bug: 640909

Updated

9 months ago
Depends on: 640911, 640912

Comment 53

7 months ago
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.
Attachment #8839151 - Flags: review?(makemyday) → review+
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.
Flags: needinfo?(makemyday) → needinfo?(jeanluc.bonnafoux)
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

7 months ago
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
(Assignee)

Comment 57

7 months ago
Hello,

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

Thanks,
(Assignee)

Updated

7 months ago
Flags: needinfo?(jeanluc.bonnafoux)
2017-10-24 makes it Thunderbird 58 or Calendar 6.0.
Component: Backend → General
Keywords: leave-open
Product: MailNews Core → Calendar
Summary: Fix all creations of nsITimer that are locally-scoped and susceptible to GC before firing, in comm-central → Fix all creations of nsITimer that are locally-scoped and susceptible to GC before firing, in Calendar
Target Milestone: --- → 6.0
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.