fix(server): close Firefox completely between specs on Windows #7106

suryart · 2020-04-22T09:11:24Z

Firefox is spawned differently than chrome. For Firefox, the .kill() does not actually close the instance running with the current profile from the arguments. It spawns a parent and a child process and runs the spec in the child process instance, while .kill() kills the child process, it leaves the parent process running, leading the next run to conflict with another spawned process(which fails for obvious reason) for the next spec execution.

Fix Cypress could not connect to Firefox. Error: cannot open socket displayed at beginning of running spec #6392

User facing changelog

Fixed an issue where running multiple specs in cypress run on Windows could fail.

Additional details

Bug fix: Video recording fails as Firefox instance not closing at the end of the spec on Windows.
Bug fix: Failing to start new spec with Firefox as previous Firefox instance not closing at the end of the spec on Windows.

How has the user experience changed?

N/A

PR Tasks

Have tests been added/updated?
Has the original issue been tagged with a release in ZenHub?
[ na ] Has a PR for user-facing changes been opened in cypress-documentation?
[ na ] Have API changes been updated in the type definitions?
[ na ] Have new configuration options been added to the cypress.schema.json?

cypress-bot · 2020-04-22T09:11:26Z

Thanks for the contribution! Below are some guidelines Cypress uses when doing PR reviews.

Please write [WIP] in the title of your Pull Request if your PR is not ready for review - someone will review your PR as soon as the [WIP] is removed.
Please familiarize yourself with the PR Review Checklist and feel free to make updates on your PR based on these guidelines.

PR Review Checklist

If any of the following requirements can't be met, leave a comment in the review selecting 'Request changes', otherwise 'Approve'.

User Experience

The feature/bugfix is self-documenting from within the product.
The change provides the end user with a way to fix their problem (no dead ends).

Functionality

The code works and performs its intended function with the correct logic.
Performance has been factored in (for example, the code cleans up after itself to not cause memory leaks).
The code guards against edge cases and invalid input and has tests to cover it.

Maintainability

The code is readable (too many nested 'if's are a bad sign).
Names used for variables, methods, etc, clearly describe their function.
The code is easy to understood and there are relevant comments explaining.
New algorithms are documented in the code with link(s) to external docs (flowcharts, w3c, chrome, firefox).
There are comments containing link(s) to the addressed issue (in tests and code).

Quality

The change does not reimplement code.
There's not a module from the ecosystem that should be used instead.
There is no redundant or duplicate code.
There are no irrelevant comments left in the code.
Tests are testing the code’s intended functionality in the best way possible.

Internal

The original issue has been tagged with a release in ZenHub.

CLAassistant · 2020-04-22T09:11:30Z

All committers have signed the CLA.

jennifer-shehane · 2020-04-22T14:42:34Z

Hi, we're changing our contributing workflow to use Draft PRs instead of [WIP] in the title. I've converted this PR to a Draft PR and removed [WIP] from the title.

Please mark the PR as Ready for Review when you're ready for a Cypress team member to review the PR.

flotwig · 2020-05-12T16:36:36Z

@suryart does this only apply on win32, confirmed? it seems that users are having similar issues on macOS and linux, although not everyone 🤔

flotwig · 2020-05-13T17:16:15Z

ah, i get it, this only happens on windows because we have to use cmd.exe to spawn firefox, and firefox immediately detaches itself from cmd.exe which leads to proc.kill failing...

suryart · 2020-05-22T06:42:09Z

@suryart does this only apply on win32, confirmed? it seems that users are having similar issues on macOS and linux, although not everyone 🤔

Sorry for the delayed response. I see in the other comment that you've figured it's happening only on Windows(win32). The PR mentioned seems like the right approach to me. So, please feel free to close this PR if needed.

flotwig

Hey @suryart, thanks for the PR. I tried doing a different fix in #7336 but it didn't work, so we should continue with your approach.

This PR looks good so far. Please add some unit tests in firefox_spec.ts to ensure there is not a regression, address the comments, and it should be good to merge.

packages/server/lib/browsers/firefox.ts

suryart · 2020-06-01T12:32:31Z

Hey @suryart, thanks for the PR. I tried doing a different fix in #7336 but it didn't work, so we should continue with your approach.

This PR looks good so far. Please add some unit tests in firefox_spec.ts to ensure there is not a regression, address the comments, and it should be good to merge.

Sure. Thank you for the reviews. I will update code according to the changes requested in a few weeks.

flotwig · 2020-06-05T15:30:27Z

@suryart I'll try to finish this up today, so we can get the fix out with the release next week

…irefox

cypress · 2020-06-05T17:47:23Z

Test summary

7876 • 1 • 130 • 0

Run details


Project	cypress
Status	Failed
Commit	`c00f6cd`
Started	Jul 10, 2020 2:49 PM
Ended	Jul 10, 2020 2:55 PM
Duration	06:37 💡
OS	Linux Debian - 10.1
Browser	Multiple

View run in Cypress Dashboard ➡️

Failures

	cypress/integration/commands/xhr_spec.js ⏯		1
1	... > sets err on log when caused by the XHR response

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

flotwig · 2020-06-05T20:08:29Z

packages/server/lib/browsers/firefox.ts

+
+  let parentPidFile = browserInstance.spawnargs[browserInstance.spawnargs.length - 1].split('\\')
+  let parentPidWithFilename = parentPidFile[parentPidFile.length - 1].split('-')
+  let parentPid = parentPidWithFilename[parentPidWithFilename.length - 1]


the pid here is actually the pid of the Cypress Electron process, which we don't want to kill

flotwig · 2020-06-05T21:10:42Z

having some issues getting this to work, something doesn't like being killed:

bhaskerchari · 2020-06-29T02:29:46Z

@flotwig,

Any luck with the changes done and the approach you have taken? I have also tried various ways of killing firefox instance but getting the same error as you mentioned in the above comment.

flotwig · 2020-06-29T13:17:17Z

@bhaskerchari not yet, for now you could probably disable video recording (video: false) to avoid this error.

bhaskerchari · 2020-06-29T15:22:57Z

@bhaskerchari not yet, for now you could probably disable video recording (video: false) to avoid this error.

@flotwig , Sorry just curious how does it work by disabling video recording (video: false) to avoid this error? What it has to do with Video recording? However I tried this option you suggested but still didn't work.

flotwig · 2020-07-09T16:45:52Z

@bhaskerchari I was able to work around the ECONNRESET error by handling foxdriver errors here: 111e09b

bhaskerchari · 2020-07-10T13:42:42Z

@bhaskerchari I was able to work around the ECONNRESET error by handling foxdriver errors here: 111e09b

@flotwig ,

Thank you for the update but any idea on when the fix may be released into the latest cypress version?

flotwig · 2020-07-10T14:47:11Z

Thank you for the update but any idea on when the fix may be released into the latest cypress version?

Hopefully within the next release.

packages/server/test/e2e/1_firefox_spec.ts

fixed

flotwig · 2020-07-15T14:23:26Z

@bahmutov now that cypress-io/cypress-example-kitchensink#436 is opened, can this be approved?

bahmutov

I think this can work, let's merge it into develop to test against Kitchensink via

- npm run test-if -- --repo cypress-example-kitchensink --command test:ci:record:windows:firefox

jennifer-shehane marked this pull request as draft April 22, 2020 14:42

jennifer-shehane changed the title ~~[WIP] Issue 6392 Firefox instance close fix on windows~~ Issue 6392 Firefox instance close fix on windows Apr 22, 2020

flotwig mentioned this pull request May 13, 2020

Fix failure to exit/relaunch Firefox on Windows #7336

Closed

5 tasks

flotwig previously requested changes May 27, 2020

View reviewed changes

suryart and others added 2 commits June 5, 2020 12:57

override .kill() function for Windows platform to make it work with F…

e9cc69d

…irefox

try using Marionette:Quit on windows

593bc91

flotwig force-pushed the issue-6392-firefox-close-fix-on-windows branch 2 times, most recently from 32b6b9c to 69195c2 Compare June 5, 2020 17:07

Use tree-kill to kill firefox

7567d97

flotwig force-pushed the issue-6392-firefox-close-fix-on-windows branch from 69195c2 to 7567d97 Compare June 5, 2020 17:23

flotwig reviewed Jun 5, 2020

View reviewed changes

flotwig added 3 commits July 9, 2020 11:14

Merge branch 'develop' into issue-6392-firefox-close-fix-on-windows

6e1b8bd

ignore foxdriver errors

111e09b

cleanup

768958d

flotwig changed the title ~~Issue 6392 Firefox instance close fix on windows~~ fix(server): close Firefox completely between specs on Windows Jul 9, 2020

flotwig added 2 commits July 10, 2020 10:15

Merge remote-tracking branch 'origin/develop' into issue-6392

b4ab623

add tests

c00f6cd

flotwig marked this pull request as ready for review July 10, 2020 15:25

flotwig requested review from kuceb, a team and flotwig and removed request for a team July 10, 2020 15:26

bahmutov reviewed Jul 10, 2020

View reviewed changes

packages/server/test/e2e/1_firefox_spec.ts Show resolved Hide resolved

flotwig requested review from bahmutov and removed request for flotwig July 10, 2020 18:03

jennifer-shehane assigned suryart Jul 13, 2020

Merge branch 'develop' into issue-6392-firefox-close-fix-on-windows

2045544

Merge branch 'develop' into issue-6392-firefox-close-fix-on-windows

72b8108

bahmutov approved these changes Jul 15, 2020

View reviewed changes

bahmutov merged commit f0a6564 into cypress-io:develop Jul 15, 2020

cypress-bot bot mentioned this pull request Jul 15, 2020

Cypress could not connect to Firefox. Error: cannot open socket displayed at beginning of running spec #6392

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(server): close Firefox completely between specs on Windows #7106

fix(server): close Firefox completely between specs on Windows #7106

suryart commented Apr 22, 2020 •

edited by flotwig

cypress-bot bot commented Apr 22, 2020

CLAassistant commented Apr 22, 2020 •

edited

jennifer-shehane commented Apr 22, 2020

flotwig commented May 12, 2020

flotwig commented May 13, 2020

suryart commented May 22, 2020

flotwig left a comment

suryart commented Jun 1, 2020

flotwig commented Jun 5, 2020

cypress bot commented Jun 5, 2020 •

edited

flotwig Jun 5, 2020

flotwig commented Jun 5, 2020

bhaskerchari commented Jun 29, 2020

flotwig commented Jun 29, 2020

bhaskerchari commented Jun 29, 2020

flotwig commented Jul 9, 2020

bhaskerchari commented Jul 10, 2020

flotwig commented Jul 10, 2020

flotwig commented Jul 15, 2020

bahmutov left a comment

fix(server): close Firefox completely between specs on Windows #7106

fix(server): close Firefox completely between specs on Windows #7106

Conversation

suryart commented Apr 22, 2020 • edited by flotwig

User facing changelog

Additional details

How has the user experience changed?

PR Tasks

cypress-bot bot commented Apr 22, 2020

PR Review Checklist

User Experience

Functionality

Maintainability

Quality

Internal

CLAassistant commented Apr 22, 2020 • edited

jennifer-shehane commented Apr 22, 2020

flotwig commented May 12, 2020

flotwig commented May 13, 2020

suryart commented May 22, 2020

flotwig left a comment

Choose a reason for hiding this comment

suryart commented Jun 1, 2020

flotwig commented Jun 5, 2020

cypress bot commented Jun 5, 2020 • edited

Test summary

Run details

Failures

flotwig Jun 5, 2020

Choose a reason for hiding this comment

flotwig commented Jun 5, 2020

bhaskerchari commented Jun 29, 2020

flotwig commented Jun 29, 2020

bhaskerchari commented Jun 29, 2020

flotwig commented Jul 9, 2020

bhaskerchari commented Jul 10, 2020

flotwig commented Jul 10, 2020

flotwig commented Jul 15, 2020

bahmutov left a comment

Choose a reason for hiding this comment

suryart commented Apr 22, 2020 •

edited by flotwig

CLAassistant commented Apr 22, 2020 •

edited

cypress bot commented Jun 5, 2020 •

edited