1453596 - Hang in "startServers" when the file "server_alive.txt" cannot be created in server.js

Status: RESOLVED FIXED

(Testing :: Mochitest, defect, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I can see a hang in `startServers()` specifically in `MochitestServer.ensureReady()` when the "server_alive.txt" file cannot be created by Mochitest server component:

https://dxr.mozilla.org/mozilla-central/rev/cfe6399e142c71966ef58a16cfd52c0b46dc6b1e/testing/mochitest/runtests.py#492-506

https://dxr.mozilla.org/mozilla-central/rev/cfe6399e142c71966ef58a16cfd52c0b46dc6b1e/testing/mochitest/server.js#177-184

This happens when a profile path has been specified with eg. a Unicode character in the Mochitest harness, and `serverAlive.exists()` returns false. Given that there is no else block with a proper failure message, the harness just times out.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Actually we cannot get rid of the hang in such a situation but we can at least provide a Javascript error which then can be picked up by Treeherder for the error line.

The underlying problem here is in xpcshell which is covered via bug 1453647.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1453596 - [mochitest] Throw exception if path for server alive file doesn't exist.

Review commit: https://reviewboard.mozilla.org/r/236042/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/236042/

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1453596 - [mochitest] Throw errors instead of strings to get line numbers.

Without using "new Error()" there will always be line 0 referenced
in the exception message.

Review commit: https://reviewboard.mozilla.org/r/236044/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/236044/

Comment 4

[Geoff Brown [:gbrown] (pto Apr 11-18)]

Comment on attachment 8967346 [details]
Bug 1453596 - [mochitest] Throw errors instead of strings to get line numbers.

https://reviewboard.mozilla.org/r/236044/#review241836

Comment 5

[Geoff Brown [:gbrown] (pto Apr 11-18)]

Comment on attachment 8967345 [details]
Bug 1453596 - [mochitest] Throw exception if path for server alive file doesn't exist.

https://reviewboard.mozilla.org/r/236042/#review241842

::: testing/mochitest/server.js:176
(Diff revision 1)
>    } else {
>      serverAlive.initWithPath(_PROFILE_PATH);
>    }
>  
>    // If we're running outside of the test harness, there might
>    // not be a test profile directory present

This comment seems a little misleading - maybe worth updating, or just deleting the comment?

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8967345 [details]
Bug 1453596 - [mochitest] Throw exception if path for server alive file doesn't exist.

https://reviewboard.mozilla.org/r/236042/#review241842

> This comment seems a little misleading - maybe worth updating, or just deleting the comment?

Misleading in which sense? Mochitest for example sets the _PROFILE_PATH, which might then not be present.

Comment 7

[Geoff Brown [:gbrown] (pto Apr 11-18)]

I think the comment makes it sound like server.js does not always expect the directory to be present -- as though it is okay for the directory not to exist. But actually, if the directory does not exist, we will throw an error (now). Maybe that's just my interpretation.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I see. I will update the commit.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8967345 [details]
Bug 1453596 - [mochitest] Throw exception if path for server alive file doesn't exist.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/236042/diff/1-2/

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8967346 [details]
Bug 1453596 - [mochitest] Throw errors instead of strings to get line numbers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/236044/diff/1-2/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/14d208836dd3
[mochitest] Throw exception if path for server alive file doesn't exist. r=gbrown
https://hg.mozilla.org/integration/autoland/rev/e67b96dc309c
[mochitest] Throw errors instead of strings to get line numbers. r=gbrown

Comment 12

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/14d208836dd3
https://hg.mozilla.org/mozilla-central/rev/e67b96dc309c

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Stability improvements for mochitest which would be good to see uplifted to mozilla-beta.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6bfef70fa570
https://hg.mozilla.org/releases/mozilla-beta/rev/1e47c5ece1f2