8000 fix: libuv hang when nodeIntegrationInSubframes enabled by codebytere · Pull Request #27582 · electron/electron · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: libuv hang when nodeIntegrationInSubframes enabled #27582

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
merged 4 commits into from
Feb 17, 2021
Merged

fix: libuv hang when nodeIntegrationInSubframes enabled #27582

merged 4 commits into from
Feb 17, 2021

Conversation

codebytere
Copy link
Member
@codebytere codebytere commented Feb 1, 2021 •
edited
Loading

Description of Change

Closes #27457.

Refs 13f319e

If we're loading Node.js into a subframe and we've already initialized the Node.js integration, that means we're in DidCreateScriptContext because a new subframe was created, not because we've reloaded the page. If that's the case, that means the last call to node_bindings_->PrepareMessageLoop() may still be active and calling it twice in a row can hang the page. In that case, don't re-prepare the message loop.

cc @deepak1556 @zcbenz @MarshallOfSound @ckerr

Checklist

Release Notes

Notes: Fixed an issue where libuv might hang with multiple subframes when nodeIntegrationInSubframes is enabled.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label Feb 1, 2021
@codebytere codebytere added semver/patch backwards-compatible bug fixes target/10-x-y labels Feb 1, 2021
@codebytere codebytere force-pushed the fix-subframes-reload branch from 8e1a52e to d407725 Compare February 2, 2021 00:48
ckerr
ckerr approved these changes Feb 2, 2021
View reviewed changes
Copy link
Member
@ckerr ckerr left a comment

Choose a reason for hiding this comment

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

This works & seems reasonable.

It's a little cumbersome though. If someone else has a better idea I'd be happy for an alternate 🙂

deepak1556
deepak1556 reviewed Feb 2, 2021
View reviewed changes
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened recently label Feb 2, 2021
zcbenz
zcbenz reviewed Feb 3, 2021
View reviewed changes
Copy link
Contributor
@zcbenz zcbenz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

If that's the case, that means the last call to node_bindings_->PrepareMessageLoop() may still be active and calling it twice in a row can hang the page.

If this is the root cause, may be we can fix it by checking whether backend fd has changed?

Also when calling PrepareMessageLoop, I think we should probably do a release first to avoid possible resources leaks.

void PrepareMessageLoop() {
  int backend_fd = uv_backend_fd(uv_loop_);
  if (backend_fd == old_backend_fd)
    return;

  if (IsInitialized(embed_sem_))
    FreeTheHandles();

  ...
}

@Patronics
Copy link
Patronics commented Feb 6, 2021 •
edited
Loading

Hey, is there any news about this issue? Is this pull request functional to fix the problem? If so, is there any chance I could get a compiled version of it (or instructions to compile it), so I could test and verify it resolves the issue (and continue working on the project affected by it)?

@codebytere codebytere force-pushed the fix-subframes-reload branch from d407725 to 4e3ebb7 Compare February 8, 2021 23:01
deepak1556
deepak1556 approved these changes Feb 9, 2021
View reviewed changes
Copy link
Member
@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

LGTM

ckerr
ckerr reviewed Feb 9, 2021
View reviewed changes
@ckerr
Copy link
Member
ckerr commented Feb 9, 2021

Not the end of the world, but aren't we leaking the semaphore on reload now?

zcbenz
zcbenz approved these changes Feb 11, 2021
View reviewed changes
Copy link
Contributor
@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

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

I'm good with current change, but we need to fix leak of uv handles on reloads with another PR.

@codebytere
Copy link
Member Author
codebytere commented Feb 11, 2021 •
edited
Loading

Looks like this is legitimately dying on Windows -

✗ Electron tests failed with code 0xc0000409.
ERROR Error: Command failed: C:\Program Files\nodejs\node.exe ./script/spec-runner.js
    at checkExecSyncError (child_process.js:630:11)
    at Object.execFileSync (child_process.js:648:15)
    at runSpecRunner (C:\Users\codebytere\.electron_build_tools\src\e-test.js:33:16)
    at Object.<anonymous> (C:\Users\codebytere\.electron_build_tools\src\e-test.js:63:3)
    at Module._compile (internal/modules/cjs/loader.js:1137:30)
    at Object.Module._extensions..js (internal/modul
8000
es/cjs/loader.js:1157:10)
    at Module.load (internal/modules/cjs/loader.js:985:32)
    at Function.Module._load (internal/modules/cjs/loader.js:878:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12)
    at internal/main/run_main_module.js:17:47

Investigating now 🤔

@codebytere
Copy link
Member Author
codebytere commented Feb 11, 2021 •
edited
Loading

It looks like this bug is only present on Linux and macOS - and uv_backend_fd doesn't work on iocp. So we'll gate it to those OSes

@codebytere codebytere force-pushed the fix-subframes-reload branch 2 times, most recently from 5356c37 to 5e68d46 Compare February 12, 2021 17:11
@zcbenz
Copy link
Contributor
zcbenz commented Feb 15, 2021

It looks like this bug is only present on Linux and macOS - and uv_backend_fd doesn't work on iocp. So we'll gate it to those OSes

We need to prevent duplicate initializations even if it does not seem to cause problems for now.

On Windows you can access IOCP handle via uv_loop_->iocp, the uv_backend_fd always returns -1 there.

@codebytere codebytere force-pushed the fix-subframes-reload branch 2 times, most recently from cb2d939 to 8723fe6 Compare February 16, 2021 15:57
@trop
Copy link
Contributor
trop bot commented Feb 17, 2021

I was unable to backport this PR to "12-x-y" cleanly;
you will need to perform this backport manually.

@codebytere codebytere mentioned this pull request Feb 23, 2021
Merged
@trop
Copy link
Contributor
trop bot commented Feb 23, 2021

@codebytere has manually backported this PR to "12-x-y", please check out #27879

@codebytere codebytere mentioned this pull request Feb 23, 2021
Merged
@trop
Copy link
Contributor
trop bot commented Feb 23, 2021

@codebytere has manually backported this PR to "11-x-y", please check out #27880

@codebytere codebytere mentioned this pull request Feb 23, 2021
Merged
@trop
Copy link
Contributor
trop bot commented Feb 23, 2021

@codebytere has manually backported this PR to "10-x-y", please check out #27881

@Patronics Patronics mentioned this pull request Mar 4, 2021
Closed
3 tasks
@codebytere codebytere mentioned this pull request Mar 13, 2021
Closed
3 tasks
@aabuhijleh aabuhijleh mentioned this pull request Apr 6, 2021
Closed
3 tasks
@sergiou87 sergiou87 mentioned this pull request May 20, 2021
Closed
3 tasks
@zcbenz zcbenz mentioned this pull request Feb 26, 2022
Closed
@zcbenz zcbenz mentioned this pull request Mar 29, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ckerr ckerr ckerr approved these changes

@zcbenz zcbenz zcbenz approved these changes

@deepak1556 deepak1556 deepak1556 approved these changes

@MarshallOfSound MarshallOfSound Awaiting requested review from MarshallOfSound

Assignees
No one assigned
Labels
semver/patch backwards-compatible bug fixes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Renderer entirely hangs with "uncaught illegal access" when loading page with 2 iframes, nodeIntegrationInSubFrames, and node-ipc
5 participants
@codebytere @Patronics @ckerr @zcbenz @deepak1556
0