The default bug view has changed. See this FAQ.

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
6 years ago
24 days ago

People

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

Tracking

Trunk
Thunderbird 54.0

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

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

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

24 days 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

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

Comment 4

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

Comment 5

24 days 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

24 days 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

24 days 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

24 days 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

24 days 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

24 days 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

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