800064 - The watchdog time computation in JS Shell fails to properly account for units

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Niko Matsakis [:nmatsakis]]

Attachment: Patch to convert units.

This test looks infinitely but should not:

timeout(1);
var start = new Date();

while (true) {
    var end = new Date();
    var duration = (end.getTime() - start.getTime()) / 1000;
    if (duration > 1) {
        print("tick");
        start = new Date();
    }
}

The problem is that the watchdog code in the shell fails to convert into the correct NSPR interval units.

Comment 1

[Terrence Cole [:terrence]]

Comment on attachment 670055 [details] [diff] [review]
Patch to convert units.

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

Bugzilla won't auto-detect patches so you have to check the |patch| checkbox when attaching a patch for the spliter review button to show up.

r=me with the nit below and a jit-test, as we discussed on IRC.

::: js/src/shell/js.cpp
@@ +2782,4 @@
>          } else {
> +             int64_t sleepDuration = gWatchdogHasTimeout
> +               ? (gWatchdogTimeout - now) / PRMJ_USEC_PER_SEC * PR_TicksPerSecond()
> +               : PR_INTERVAL_NO_TIMEOUT;

This is getting to0 long to look nice as a ternary.  Please adjust this to:
uint64_t sleepDuration = PR_INTERVAL_NO_TIMEOUT;
if (gWatchdogHasTimeout)
    sleepDuration = (gWatchdogTimeout - now) / PRMJ_USEC_PER_SEC * PR_TicksPerSecond();

Comment 2

[Niko Matsakis [:nmatsakis]]

Attachment: Fix bug, add a test.

Comment 3

[Terrence Cole [:terrence]]

Comment on attachment 670108 [details] [diff] [review]
Fix bug, add a test.

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

::: js/src/shell/js.cpp
@@ +2782,4 @@
>          } else {
> +             int64_t sleepDuration = gWatchdogHasTimeout
> +               ? (gWatchdogTimeout - now) / PRMJ_USEC_PER_SEC * PR_TicksPerSecond()
> +               : PR_INTERVAL_NO_TIMEOUT;

Did you forget to hg qref?

Comment 4

[Niko Matsakis [:nmatsakis]]

Attachment: reformat.

Sorry, missed your first comment about reformatting.

Comment 5

[Terrence Cole [:terrence]]

Comment on attachment 670114 [details] [diff] [review]
reformat.

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

Looks great!

Comment 6

[Terrence Cole [:terrence]]

We should get this in: I can push it for you if you'd like.

Comment 7

[Niko Matsakis [:nmatsakis]]

Attachment: refreshed version.

Comment 8

[Niko Matsakis [:nmatsakis]]

Terrence, just realized I forgot to add "commit-needed" to this bug (missed your comment).  I'll do that now.

Comment 9

[Niko Matsakis [:nmatsakis]]

Attachment: re-refreshed patch.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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