1232285 - The Terminator's notion of ticks is incorrect

Status: RESOLVED FIXED

(Toolkit :: Async Tooling, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

In nsTerminator.cpp, we currently hardcode TICK_DURATION = 1000, assuming that one tick is one millisecond. That's probably not true on all platforms.

Rather, we should define `tick_duration = PR_MillisecondsToInterval(1000)` to get the correct value for the platform.

Comment 1

[Timur Valeev]

Attachment: bug-1232285-fix-0.1.patch

Hi! Is it good?

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8698459 [details] [diff] [review]
bug-1232285-fix-0.1.patch

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

Actually, that's not exactly what I want.
Define `TICKS_DURATION` is used twice in the code. I'd like to replace the global define with a local constant `ticksDuration` defined from `PR_MillisecondsToInterval(1000)`.

Comment 3

[Timur Valeev]

Attachment: bug-1232285-fix-0.2.patch

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8699010 [details] [diff] [review]
bug-1232285-fix-0.2.patch

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

Looks good to me. Just don't forget to change the commit message to add ",r=Yoric" at the end.
Let's run this through the tests: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac274be0103f

Note that I'll be away for ~2 weeks so you may need to find someone else to land this code.

::: toolkit/components/terminator/nsTerminator.cpp
@@ +126,5 @@
>    uint32_t crashAfterTicks = options->crashAfterTicks;
>    options = nullptr;
>  
>    const uint32_t timeToLive = crashAfterTicks;
> +  const uint32_t ticksDuration = PR_MillisecondsToInterval(1000);

This should work, but let's make it `PRIntervalTime` instead of `uint32_t`.

Comment 5

[Timur Valeev]

Attachment: bug-1232285-fix-0.3.patch

Comment 6

[Timur Valeev]

Hi! Do you know who can land this code?

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/15e06810ae30

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Timur Valeev from comment #6)
> Hi! Do you know who can land this code?

Landed, thanks!

(For future patches, you can set the checkin-needed keyword after posting a successful try run URL to the bug.)

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/15e06810ae30