Closed
Bug 758585
Opened 13 years ago
Closed 7 years ago
Fix all creations of nsITimer that are locally-scoped and susceptible to GC before firing, in Calendar
Categories
(Calendar :: General, defect)
Calendar
General
Tracking
(Not tracked)
RESOLVED
FIXED
6.0
People
(Reporter: sgautherie, Assigned: jeanluc.bonnafoux)
References
(Depends on 2 open bugs, )
Details
Attachments
(1 file, 10 obsolete files)
4.35 KB,
patch
|
MakeMyDay
:
review+
|
Details | Diff | Splinter Review |
It looks like at least some SeaMonkey tests would be affected...
(I didn't check further.)
Updated•8 years ago
|
Comment 1•8 years ago
|
||
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,
Updated•8 years ago
|
Assignee: nobody → jeanluc.bonnafoux
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•8 years ago
|
||
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)
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•8 years ago
|
||
Yes GC is Garbage Collection.
And yes that is the path to follow.
Flags: needinfo?(mkmelin+mozilla)
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•8 years ago
|
||
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,
Attachment #8822141 -
Flags: review?(kaie)
Comment 10•8 years ago
|
||
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)
Updated•8 years ago
|
Flags: needinfo?(acelists)
Assignee | ||
Comment 11•8 years ago
|
||
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)
Comment 12•8 years ago
|
||
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•8 years ago
|
||
Third patch proposal, fixing indentation.
Attachment #8822148 -
Attachment is obsolete: true
Attachment #8822148 -
Flags: review?(jorgk)
Attachment #8822153 -
Flags: review?(makemyday)
Comment 14•8 years ago
|
||
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•8 years ago
|
||
Patch version fixing tab and modifying comment.
Attachment #8822153 -
Attachment is obsolete: true
Attachment #8822153 -
Flags: review?(makemyday)
Attachment #8822157 -
Flags: review?(makemyday)
Comment 16•8 years ago
|
||
Great. Thanks. The next patches will the spot on from the beginning ;-)
Assignee | ||
Comment 17•8 years 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,
Comment 18•8 years ago
|
||
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•8 years 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•8 years ago
|
||
Thanks for this detailed review of proposed patch.
I will work on a new patch version following your comments.
Assignee | ||
Comment 21•8 years ago
|
||
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•8 years 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•8 years ago
|
||
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•8 years 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 25•8 years 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]:
-----------------------------------------------------------------
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 = ...
Updated•8 years ago
|
Flags: needinfo?(makemyday)
Assignee | ||
Comment 26•8 years 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,
Comment 27•8 years ago
|
||
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•8 years ago
|
||
Well, i also think there is something wrong, let's see what makemyday thinks.
Assignee | ||
Comment 29•8 years 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,
Comment 30•8 years 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•8 years 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•8 years ago
|
||
It's enough to get Lightning if you build with
ac_add_options --enable-calendar
in your mozconfig.
Comment 33•8 years ago
|
||
(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•8 years 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•8 years 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•8 years 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 | ||
Comment 37•8 years ago
|
||
Hello,
Please find a new patch proposal following comments.
Thanks,
Attachment #8826857 -
Flags: review?(makemyday)
Comment 38•8 years 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•8 years ago
|
Attachment #8822901 -
Attachment is obsolete: true
Assignee | ||
Comment 39•8 years ago
|
||
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•8 years 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,
Comment 41•8 years ago
|
||
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•8 years 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•8 years ago
|
||
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•8 years 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+
Keywords: checkin-needed,
leave-open
Comment 45•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
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•8 years 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+
Comment 49•8 years ago
|
||
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)
Updated•8 years ago
|
Flags: needinfo?(makemyday)
Comment 50•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8836406 -
Flags: feedback-
Comment 51•8 years ago
|
||
This is all that's required.
Attachment #8839151 -
Flags: review?(makemyday)
Updated•7 years ago
|
Comment 53•7 years 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+
Updated•7 years ago
|
Attachment #8836406 -
Attachment is obsolete: true
Comment 54•7 years ago
|
||
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)
Comment 55•7 years ago
|
||
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 years 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 years ago
|
||
Hello,
Thanks a lot for your patch, please feel free to move forward on this topic.
Thanks,
Comment 58•7 years ago
|
||
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
Updated•7 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•