Skip to content
This repository was archived by the owner on Sep 6, 2021. It is now read-only.

[Review Only] appReadyDeferred#8062

Closed
redmunds wants to merge 1 commit intomasterfrom
randy/appShutdown-7683
Closed

[Review Only] appReadyDeferred#8062
redmunds wants to merge 1 commit intomasterfrom
randy/appShutdown-7683

Conversation

@redmunds
Copy link
Copy Markdown
Contributor

@redmunds redmunds commented Jun 7, 2014

This is an attempt to fix this remaining crash of #7683 :

  • Crash on close only seems to happen if there is a file that gets opened at startup.
  • Only happens if file is JavaScript or is an HTML file that references a JS file
  • If I hit Ctrl-Q before file is shown in editor (i.e. can still see "code the web"), then there's no crash
  • If I wait a few seconds after file is shown in editor before Ctrl-Q, then there's no crash
  • Best way to reproduce crash is to hit Ctrl-Q as soon as you see any file contents

So, the remaining crash seems to point to JS Code Hints.

@dangoor - This is my first attempt to implement the solution that we talked about. This does not fix crash, but I wanted to post code for review to see if I'm on the right track.

@redmunds
Copy link
Copy Markdown
Contributor Author

redmunds commented Jun 9, 2014

Note: Jeff is upgrading CEF from 1750 to 1916, so we should verify problem still exists before putting much effort into this PR.

@dangoor
Copy link
Copy Markdown
Contributor

dangoor commented Jun 10, 2014

@redmunds Do you want to try Jeff's 1916 branch to see if you can still reproduce the problem?

@peterflynn
Copy link
Copy Markdown
Member

@redmunds I wonder if this is leading us down a path of one-off fixing specific repro cases of a more general problem. It seems like the issue is that the shell crashes any time we're trying to quit while a callback into V8 is still pending -- most typically a file IO callback. There's lots of file IO and startup and shutdown, so it's easiest to hit then... but it seems like this could be hit basically on any operation -- e.g. I hit Save All and then quickly quit, and some of those Save operations are still running.

So I wonder if a more general native fix would be better, rather than sprinkling extra tracking code into every async IO operation we can find (and trying to remember to do so for all future cases too).

@dangoor
Copy link
Copy Markdown
Contributor

dangoor commented Jun 10, 2014

@peterflynn That's a valid point. It does seem reasonable that the shell should know best when it's safe to terminate.

@redmunds
Copy link
Copy Markdown
Contributor Author

Unfortunately, I'm still seeing the "remaining crash on quit" problem in CEF 1916.

I agree this is not the ultimate fix for the problem. Any ideas are welcome.

@redmunds redmunds changed the title Initial attempt at implementing appReadyDeferred [Review Only] appReadyDeferred Jun 13, 2014
@redmunds
Copy link
Copy Markdown
Contributor Author

Marking "PR Triage Complete" and assigning to @dangoor.

@dangoor
Copy link
Copy Markdown
Contributor

dangoor commented Jun 17, 2014

I'm thinking I should hold off on looking deeper at this since there is investigation into the root crash going on.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@redmunds I don't think you're getting the correct Promise object. The one that we really want to wait for is the Promise object used in Tern worker that is still reading some files from the current project. See my comment

@redmunds
Copy link
Copy Markdown
Contributor Author

Closing.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants