1190735 - Make nsITimer.TYPE_REPEATING_PRECISE a synonym for nsITimer.TYPE_REPEATING_PRECISE_CAN_SKIP

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nicholas Nethercote [inactive]]

The docs for TYPE_REPEATING_PRECISE say "Use this timer type with extreme caution. Chances are, this is not what you want."

Bug 650379 introduced TYPE_REPEATING_PRECISE_CAN_SKIP as a safer alternative. That bug also converted all uses of TYPE_REPEATING_PRECISE in the tree to TYPE_REPEATING_PRECISE_CAN_SKIP.

Since then, one use of TYPE_REPEATING_PRECISE has been added to the tree relating to network stats (https://dxr.mozilla.org/mozilla-central/source/dom/network/NetworkStatsService.jsm#130).

The value is exposed via IDL so add-ons might use it. Making it equivalent to TYPE_REPEATING_PRECISE_CAN_SKIP might be a reasonable thing to do.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Remove nsITimer.TYPE_REPEATING_PRECISE

The patch is straightforward. The hard part is deciding if this is the right
thing to do. froydnj, please feel free to muster additional troops for the
review if appropriate; there's no rush on this.

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8642887 [details] [diff] [review]
Remove nsITimer.TYPE_REPEATING_PRECISE

Review of attachment 8642887 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/threads/nsITimer.idl
@@ +92,5 @@
>     * guarantees that it will not queue up new events to fire the callback
>     * until the previous callback event finishes firing.  If the callback
>     * takes a long time, then the next callback will be scheduled immediately
> +   * afterward, but only once.  This is the only non-slack timer.
> +   * to use.

Nit: this "to use" should be wrapped with the line above, which also means removing the period after "timer".

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e14cb4eb7c0b

Comment 4

[Nicholas Nethercote [inactive]]

I'll update the MDN docs once this has stuck.

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e14cb4eb7c0b

Comment 6

[Nicholas Nethercote [inactive]]

I updated the description of TYPE_REPEATING_PRECISE in
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsITimer.

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8642887 [details] [diff] [review]
Remove nsITimer.TYPE_REPEATING_PRECISE

Approval Request Comment
[Feature/regressing bug #]: N/A (to support landing bug 1197152)

[User impact if declined]: Can't uplift bug 1197152 to beta

[Describe test coverage new/current, TreeHerder]: Lots of stuff uses timers...

[Risks and why]: In nightly and aurora since 8/7.  Low risk to uplift

[String/UUID change made/needed]: none

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8642887 [details] [diff] [review]
Remove nsITimer.TYPE_REPEATING_PRECISE

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: See request in bug 1197152

User impact if declined: Can't land bug 1197152.  

Fix Landed on Version: 42

Risk to taking this patch (and alternatives if risky): Pretty low risk, and been in for a long while.

String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8642887 [details] [diff] [review]
Remove nsITimer.TYPE_REPEATING_PRECISE

We need this bug to be uplifted to fix bug 1197152. Beta41+

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Beta version of the patch (bug 1059572 moved PostTImerEvent from TimerImpl.cpp to TimerThread.cpp)

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/ddf1390f6cca

Comment 12

[Andrei Vaida [:avaida]]

Nicholas, is manual QA required here? If yes, please let us know what we can do to help verify this fix.

Comment 13

[Nicholas Nethercote [inactive]]

> Nicholas, is manual QA required here? If yes, please let us know what we can
> do to help verify this fix.

There's no useful QA that I can think of.

Comment 14

[Andrei Vaida [:avaida]]

(In reply to Nicholas Nethercote [:njn] from comment #13)
> > Nicholas, is manual QA required here? If yes, please let us know what we can
> > do to help verify this fix.
> 
> There's no useful QA that I can think of.

Thank you for the prompt reply! Setting qe-verify- based on Comment 13.

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8642887 [details] [diff] [review]
Remove nsITimer.TYPE_REPEATING_PRECISE

After discussion with the rest of the team, we're not taking this for ESR but can take it on a thunderbird relbranch.

Comment 16

[Kent James (:rkent)]

https://hg.mozilla.org/releases/mozilla-esr38/rev/9e87ca58e3f5 for THUNDERBIRD_38_VERBRANCH

(I did not roll the idl since this was a documentation change only).