Closed
Bug 640909
Opened 14 years ago
Closed 8 years ago
Fix local scoping of nsITimer in mailTestUtils.js that is susceptible to GC before firing
Categories
(MailNews Core :: Testing Infrastructure, defect)
MailNews Core
Testing Infrastructure
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 54.0
People
(Reporter: standard8, Assigned: jorgk-bmo)
References
()
Details
Attachments
(1 file, 2 obsolete files)
|
(deleted),
patch
|
jorgk-bmo
:
review+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #640629 +++
When timers are created, there is no extra reference held until they fire. That means that if the variable storing the timer object goes out of scope, it's susceptible to a GC which will destroy the timer without it ever firing.
|
Reporter | |
Comment 1•14 years ago
|
||
Actually, just going to file separate bugs. There's only one instance in mailnews/ and the mail/ and suite/ should be filed separate.
Component: Backend → Testing Infrastructure
No longer depends on: nsITimer-fail
QA Contact: backend → testing-infrastructure
Summary: Fix all creations of nsITimer in comm-central that are locally-scoped and susceptible to GC before firing → Fix local scoping of nsITimer in mailTestUtils.js that is susceptible to GC before firing
jbonnafo, is this the same concept as in bug 758585? Is there still anything to do here?
Flags: needinfo?(jeanluc.bonnafoux)
|
Assignee | |
Comment 3•8 years ago
|
||
Same bug as 758585, I'd say.
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: needinfo?(jeanluc.bonnafoux)
Resolution: --- → DUPLICATE
But the other bug has no change to mailTestUtils.js . Is that file already fixed by other means?
|
|
Assignee | |
Comment 5•8 years ago
|
||
Well, the idea was that the other bug should cover all of C-C. But it's going a bit slow. Look, I'll fix this one here for you, OK?
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
|
|
Assignee | |
Comment 6•8 years ago
|
||
Here you go.
Comment on attachment 8842167 [details] [diff] [review]
640909-timer.patch
Review of attachment 8842167 [details] [diff] [review]:
-----------------------------------------------------------------
::: mailnews/test/resources/mailTestUtils.js
@@ +413,5 @@
> * @param aFunc The function to invoke when the timer fires.
> * @param aFuncThis Optional 'this' pointer to use.
> * @param aFuncArgs Optional list of arguments to pass to the function.
> */
> + timer: null,
Looks good to me. Just please call it _timer as it is not supposed to be accessed from outside callers.
@@ +421,4 @@
> let wrappedFunc = function() {
> try {
> aFunc.apply(aFuncThis, aFuncArgs);
> }
Do we need to clear the timer in the wrappedFunc? this._timer = null;
Attachment #8842167 -
Flags: review?(acelists) → review+
(In reply to Jorg K (GMT+1) from comment #5)
> But it's going a bit slow. Look, I'll fix this one here for you, OK?
Yes thanks ;)
|
|
Assignee | |
Comment 9•8 years ago
|
||
Fixed variable name.
(In reply to :aceman from comment #7)
> Do we need to clear the timer in the wrappedFunc? this._timer = null;
I don't think so since it's TYPE_ONE_SHOT.
Attachment #8842167 -
Attachment is obsolete: true
Attachment #8842176 -
Flags: review+
|
|
Assignee | |
Comment 10•8 years ago
|
||
Oops, had to lose the 'let'.
Attachment #8842176 -
Attachment is obsolete: true
Attachment #8842178 -
Flags: review+
|
|
Assignee | |
Comment 11•8 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 54.0
You need to log in
before you can comment on or make changes to this bug.
Description
•