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

RESOLVED FIXED in Thunderbird 54.0

Status

MailNews Core
Testing Infrastructure
RESOLVED FIXED
7 years ago
6 months ago

People

(Reporter: standard8, Assigned: Jorg K (GMT+2))

Tracking

Trunk
Thunderbird 54.0

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

+++ 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: 640629
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

Comment 2

6 months ago
jbonnafo, is this the same concept as in bug 758585? Is there still anything to do here?
Flags: needinfo?(jeanluc.bonnafoux)
(Assignee)

Comment 3

6 months ago
Same bug as 758585, I'd say.
Status: NEW → RESOLVED
Last Resolved: 6 months ago
Flags: needinfo?(jeanluc.bonnafoux)
Resolution: --- → DUPLICATE
Duplicate of bug: 758585

Comment 4

6 months ago
But the other bug has no change to mailTestUtils.js . Is that file already fixed by other means?
(Assignee)

Comment 5

6 months 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

6 months ago
Created attachment 8842167 [details] [diff] [review]
640909-timer.patch

Here you go.
Assignee: nobody → jorgk
Status: REOPENED → ASSIGNED
Attachment #8842167 - Flags: review?(acelists)

Comment 7

6 months ago
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+

Comment 8

6 months ago
(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

6 months ago
Created attachment 8842176 [details] [diff] [review]
640909-timer.patch (v2).

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

6 months ago
Created attachment 8842178 [details] [diff] [review]
640909-timer.patch (v3)

Oops, had to lose the 'let'.
Attachment #8842176 - Attachment is obsolete: true
Attachment #8842178 - Flags: review+
(Assignee)

Comment 11

6 months ago
https://hg.mozilla.org/comm-central/rev/8aa38511b468ae26a1d3d7213a250fd77516bd9c
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago6 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 54.0
You need to log in before you can comment on or make changes to this bug.