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

Avoid proxy handler race condition crash #11176

Merged
merged 1 commit into from Nov 20, 2017

Conversation

Projects
None yet
3 participants
@mgc
Copy link
Contributor

mgc commented Nov 19, 2017

#11134 seems to be a very delicate race condition. No luck making an isolated demo, but here is a working theory:

  • URLRequest hits a proxy while running on an IO thread
  • LoginHandler is constructed, captures render_process_host_id_/render_frame_id_ at that moment, then posts a task to the UI thread
  • The original process or render frame dies somehow. Maybe the Window is closed, or navigation happens. This is the race
  • The RequestLogin task finally executes on the UI thread
  • App::OnLogin calls LoginHandler::GetWebContents() which returns null, either because render_process_host_id_/render_frame_id_ are no longer valid, or because RenderFrameHost can't be mapped to a WebContents
  • App::OnLogin tries to construct a wrapper around the null web_contents, and ends up crashing here

This seems like a plausible race, and there are already places in the code where we convert host and frame ids into a webcontents then guard against exactly this case. How do people feel about handling the null web_contents gracefully and just cancelling the auth request?

@mgc mgc self-assigned this Nov 19, 2017

@mgc mgc requested review from ckerr , zcbenz and deepak1556 Nov 19, 2017

@mgc mgc requested a review from electron/reviewers as a code owner Nov 19, 2017

@zcbenz

zcbenz approved these changes Nov 20, 2017

Copy link
Member

zcbenz left a comment

It looks good to me.

@deepak1556
Copy link
Member

deepak1556 left a comment

👍

@zcbenz zcbenz merged commit cb98ed8 into master Nov 20, 2017

8 checks passed

ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/branch This commit looks good
Details
continuous-integration/jenkins/pr-head This commit looks good
Details

@zcbenz zcbenz deleted the proxy-crash-race branch Nov 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment