970904 - sleep() shell function is broken

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

sleep(1) should sleep for 1 second but it just seems to hang.

Comment 1

[Jan de Mooij [:jandem]]

I've no idea when this broke but based on the code probably a long time ago....

The timeout() function calls SetTimeoutValue, which calls ScheduleWatchdog to start the watchdog thread.

The sleep() function relies on the watchdog waking it up, but it doesn't start the watchdog thread so this never happens. And I think there's another problem: WatchdogMain hangs indefinitely if gWatchdogHasTimeout is false, so it seems like Sleep_fn should either set gWatchdogHasTimeout or it should notify the gWatchdogWakeup condition var..

Comment 2

[Jan de Mooij [:jandem]]

I wonder why we don't use PR_Sleep instead of the watchdog thread. I'll see if that works.

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #2)
> I wonder why we don't use PR_Sleep instead of the watchdog thread. I'll see
> if that works.

I read some old bugs and I think the problem was that we want timeout() to interrupt a sleep() call. What we could do is:

if (gWatchdogHasTimeout) {
    .. complicated code we have now..
} else {
    PR_Sleep(ticks);
}

Or something.

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Make sleep() work without timeout() prior to it in js shell.

Just implemented comment #3.

Green on try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0212ae089de4

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8550590 [details] [diff] [review]
Make sleep() work without timeout() prior to it in js shell.

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

Looks good, but this will probably fail to compile if we use NSPR emulation (configure with --posix-nspr-emulation), because it doesn't have PR_Sleep. Can you confirm and fix that? :)

Comment 6

[Tooru Fujisawa [:arai]]

Attachment: Fix PR_WaitCondVar timeout for sleep() in js shell.

Thank you for letting me know about NSPR emulation.

I looked the implementation of PR_Sleep, and I noticed that jsshell's `Sleep_fn` works without watchdog, but it sleeps a thousand times longer because of wrong timeout value (in [us]) passed to `PR_WaitCondVar`s timeout parameter (which should be in [ms]).

Try is running: https://treeherder.mozilla.org/#/jobs?repo=try&revision=16790207ff54

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8553508 [details] [diff] [review]
Fix PR_WaitCondVar timeout for sleep() in js shell.

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

Ah, good catch. Thanks!

An unrelated thing I noticed is that sleep() returns itself. Can you add args.rval().setUndefined(); before the return statement at the end, while you're fixing this function?

Comment 8

[Jan de Mooij [:jandem]]

Funny thing about sleep returning itself is that you can do sleep(1)(1)(1) to sleep 3 seconds ;)

Comment 9

[Tooru Fujisawa [:arai]]

Thank you for reviewing :)
Fixed the return value.

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

Comment 10

[Phil Ringnalda (:philor)]

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