Closed Bug 640909 Opened 9 years ago Closed 3 years ago

Fix local scoping of nsITimer in mailTestUtils.js that is susceptible to GC before firing

Categories

(MailNews Core :: Testing Infrastructure, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 54.0

People

(Reporter: standard8, Assigned: jorgk)

References

()

Details

Attachments

(1 file, 2 obsolete files)

+++ 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.
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)
Same bug as 758585, I'd say.
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: needinfo?(jeanluc.bonnafoux)
Resolution: --- → DUPLICATE
Duplicate of bug: 758585
But the other bug has no change to mailTestUtils.js . Is that file already fixed by other means?
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 → ---
Attached patch 640909-timer.patch (obsolete) — Splinter Review
Here you go.
Assignee: nobody → jorgk
Status: REOPENED → ASSIGNED
Attachment #8842167 - Flags: review?(acelists)
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 ;)
Attached patch 640909-timer.patch (v2). (obsolete) — Splinter Review
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+
Oops, had to lose the 'let'.
Attachment #8842176 - Attachment is obsolete: true
Attachment #8842178 - Flags: review+
https://hg.mozilla.org/comm-central/rev/8aa38511b468ae26a1d3d7213a250fd77516bd9c
Status: ASSIGNED → RESOLVED
Closed: 3 years ago3 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.