Skip to content
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.

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

Fix orphaned browser processes due to uncaught exceptions in the tests #18401

Merged
merged 1 commit into from
Jul 6, 2024

Conversation

timvandermeij
Copy link
Contributor

@timvandermeij timvandermeij commented Jul 6, 2024

If uncaught exceptions occur in the tests (which happened in #17962 and can be triggered manually by throwing an error in integration-boot.js) the teardown logic of the tests doesn't get to run and thus spawned browser processes are not closed properly. Given that test.mjs is the only process that has a reference to them they will become orphaned and keep running if test.mjs exits without explicitly closing them.

This commit fixes the issue by always closing the browsers if uncaught exceptions occur, and we make sure to log them for debugging purposes.

Unblocks #17962.

This patch is ever so slightly easier to review with the ?w=1 flag: https://github.com/mozilla/pdf.js/pull/18401/files?w=1.

@timvandermeij
Copy link
Contributor Author

timvandermeij commented Jul 6, 2024

I have applied the following diff:

diff --git a/test/integration-boot.mjs b/test/integration-boot.mjs
index 4e04bb6f1..332c97a50 100644
--- a/test/integration-boot.mjs
+++ b/test/integration-boot.mjs
@@ -20,6 +20,8 @@ async function runTests(results) {
   jasmine.exitOnCompletion = false;
   jasmine.jasmine.DEFAULT_TIMEOUT_INTERVAL = 30000;
 
+  throw new Error("Yikes, something unexpected happened");
+
   jasmine.loadConfig({
     random: false,
     spec_dir: "integration"

Before this patch npx gulp integrationtest would crash, keep the Chrome process open and log this:

file:///home/timvandermeij/Documenten/Ontwikkeling/pdf.js/Code/test/integration-boot.mjs:23
  throw new Error("Yikes, something unexpected happened");
        ^

Error: Yikes, something unexpected happened
    at runTests (file:///home/timvandermeij/Documenten/Ontwikkeling/pdf.js/Code/test/integration-boot.mjs:23:9)
    at startIntegrationTest (file:///home/timvandermeij/Documenten/Ontwikkeling/pdf.js/Code/test/test.mjs:818:9)

Node.js v20.15.0
[12:46:24] The following tasks did not complete: integrationtest, runIntegrationTest
Did you forget to signal async completion?
file:///home/timvandermeij/Documenten/Ontwikkeling/pdf.js/Code/gulpfile.mjs:723
        throw new Error(`Running ${testsName} tests failed.`);
        ^

Error: Running integration tests failed.
    at ChildProcess.<anonymous> (file:///home/timvandermeij/Documenten/Ontwikkeling/pdf.js/Code/gulpfile.mjs:723:15)
    at ChildProcess.emit (node:events:519:28)
    at ChildProcess.emit (node:domain:551:15)
    at maybeClose (node:internal/child_process:1105:16)
    at ChildProcess._handle.onexit (node:internal/child_process:305:5)
    at Process.callbackTrampoline (node:internal/async_hooks:130:17)

Node.js v20.15.0

After this patch npx gulp integrationtest doesn't crash anymore, closes all browser processes and logs this:

Error: Yikes, something unexpected happened
    at runTests (file:///home/timvandermeij/Documenten/Ontwikkeling/pdf.js/Code/test/integration-boot.mjs:23:9)
    at startIntegrationTest (file:///home/timvandermeij/Documenten/Ontwikkeling/pdf.js/Code/test/test.mjs:818:9)
    at async main (file:///home/timvandermeij/Documenten/Ontwikkeling/pdf.js/Code/test/test.mjs:1093:7)

Run 0 tests
OHNOES!  No integration tests ran!
integration tests runtime was 3.3 seconds

Not only does this therefore fix the problem of hanging browser windows, but it also makes the logs more concise/actionable by removing noise from the uncaught exception propagating.

If uncaught exceptions occur in the tests (which happened in mozilla#17962 and
can be triggered manually by throwing an error in `integration-boot.js`)
the teardown logic of the tests doesn't get to run and thus spawned
browser processes are not closed properly. Given that `test.mjs` is the
only process that has a reference to them they will become orphaned and
keep running if `test.mjs` exits without explicitly closing them.

This commit fixes the issue by always closing the browsers if uncaught
exceptions occur, and we make sure to log them for debugging purposes.
@timvandermeij
Copy link
Contributor Author

/botio unittest

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_unittest from @timvandermeij received. Current queue size: 0

Live output at: http://54.193.163.58:8877/0936bcbf5dcdb06/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_unittest from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/30305e1c62e43f7/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/30305e1c62e43f7/output.txt

Total script time: 2.47 mins

  • Unit Tests: Passed

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/0936bcbf5dcdb06/output.txt

Total script time: 7.07 mins

  • Unit Tests: FAILED

@timvandermeij
Copy link
Contributor Author

/botio integrationtest

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/e39aa1e1bb10d5a/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 0

Live output at: http://54.193.163.58:8877/481c711563dfae6/output.txt

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

r=me, thank you!

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/e39aa1e1bb10d5a/output.txt

Total script time: 7.96 mins

  • Integration Tests: Passed

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/481c711563dfae6/output.txt

Total script time: 16.95 mins

  • Integration Tests: FAILED

@timvandermeij timvandermeij merged commit fe69243 into mozilla:master Jul 6, 2024
9 checks passed
@timvandermeij timvandermeij deleted the test-orphaned-browsers branch July 6, 2024 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants