Skip to content

[wasm][test-browser] Retry launching chrome if it fails on startup #343

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
akoeplinger merged 2 commits into from
Oct 30, 2020
Merged

[wasm][test-browser] Retry launching chrome if it fails on startup #343

akoeplinger merged 2 commits into from
Oct 30, 2020

Conversation

@radical
Copy link
Member

@radical radical commented Oct 29, 2020 •
edited
Loading

When starting chrome via selenium+chromedriver, it seems to crash
inexplicably (dotnet/runtime#43955).
As a workaround, let's retry that a few times.

Also, adds --no-sandbox from #340 .

@radical
[wasm][test-browser] Retry launching chrome if it fails on startup
62bdf15 
When starting chrome via selenium+chromedriver, it seems to crash
inexplicably (dotnet/runtime#43955).
As a workaround, let's retry that a few times.
@radical
Copy link
Member Author

radical commented Oct 29, 2020

/cc @lewing @akoeplinger

@akoeplinger
Copy link
Member

akoeplinger commented Oct 29, 2020 •
edited
Loading

I don't think we should be adding --no-sandbox as the docs say:

A common cause for Chrome to crash during startup is running Chrome as root user (administrator) on Linux.
While it is possible to work around this issue by passing '--no-sandbox' flag when creating your WebDriver session,
such a configuration is unsupported and highly discouraged.
Please configure your environment to run Chrome as a regular user instead.

We're definitely not running Chrome as root so I'm a bit hesitant to add this highly discouraged option. The retry should hopefully fix it too.

akoeplinger
akoeplinger reviewed Oct 29, 2020
View reviewed changes
src/Microsoft.DotNet.XHarness.CLI/Commands/WASM/Browser/WasmTestBrowserCommand.cs
{
return (driverService, new ChromeDriver(driverService, options, _arguments.Timeout));
}
catch (WebDriverException wde) when (wde.Message.Contains("exited abnormally") && retry_num < max_retries - 1)
Copy link
Member

@akoeplinger akoeplinger Oct 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we make this check for failed to start: exited abnormally so we don't accidentally retry other cases of abnormal exits, e.g. if the browser crashes due to some WASM bug while running tests.

Copy link
Member Author

@radical radical Oct 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is catching exceptions only while starting the browser, before any page gets opened.

@akoeplinger
Copy link
Member

akoeplinger commented Oct 29, 2020

We could also try passing the options puppeteer passes by default: https://github.com/puppeteer/puppeteer/blob/main/src/node/Launcher.ts#L159-L181

@radical
[wasm][test-browser] Enable verbose driver logging
580578c 
.. and remove `--no-sandbox`, as that seems to be discouraged in the
docs.
@radical
Copy link
Member Author

radical commented Oct 29, 2020

I have removed --no-sandbox, and added some extra arguments for chrome based on @akoeplinger's comment. I removed the ones that seem to be passed already (by chromedriver/selenium?).

@akoeplinger akoeplinger merged commit 6cbe79c into dotnet:master Oct 30, 2020
@akoeplinger akoeplinger mentioned this pull request Oct 30, 2020
Closed
@akoeplinger akoeplinger mentioned this pull request Nov 9, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@akoeplinger akoeplinger akoeplinger approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@radical @akoeplinger