Terminate tern worker on quit

[RaymondLim]

This fixes issue #7683.

[busykai]

in this function, perhaps, the param should be called terminate or something like this. also, add @param to the doc.

[RaymondLim]

Good point! I forgot to add @param since I was in a rush to set up the pull request for others to discuss. So don't land it yet even after I make the changes. Thanks for reviewing.

[RaymondLim]

Closing since the actual crash is caused by callback_maps_ in the shell code.

[RaymondLim]

@peterflynn, @nethip Can you review and merge?

[nethip]

@peterflynn Let us know if this PR looks good to you.

[peterflynn]

@nethip My main concern is that we already know of at least one extension that uses worker threads and causes a crash, so fixing only our own usage of worker threads isn't enough to fully fix this.

A secondary concern: because Raymond is unable to repro in cefclient yet, that means this may be caused by a bug in our brackets-shell code. If we don't fully understand the root cause of the bug, we don't know its scope: the same unfixed underlying bug may cause crashes in other scenarios too.

[peterflynn]

@RaymondLim Continuing our discussion before about building a testcase to see if this is a cefclient issue... it sounds like we have two cases right now:

• Full Brackets, running in full brackets-shell (but with a simple/empty worker thread, not Tern) -- crashes
• Simple worker thread in isolation, running in cefclient -- no crash

Since there are two variables that differ here, we don't know whether no crash in cefclient means the bug isn't present there, or if it just means that the much simpler testcase we're using in cefclient isn't enough to cause a crash.

First, I think we should try to just run the same very simple testcase code in brackets-shell -- is it enough to cause a crash there? If so, then we know it's a valid repro case, and the lack of a crash in cefclient tells us something. But if it's not enough to crash brackets-shell, the next step is to build a testcase that does repro the crash.

We can try to tackle that in two different ways:

• Add to the simple testcase -- e.g. add timing delays or other extras -- until it causes a crash in brackets-shell. Then see if that testcase crashes in cefclient too.
• Subtract from the "full" Brackets src repro case. Start from pflynn/in-browser-file-system -- is that enough to cause a crash even in brackets-shell? If so, you can just run that in cefclient too. If not, try to figure out what the in-browser branch is missing.
  • Is the in-browser sample project too simple to cause a crash even if you copy those files to disk and open them in desktop Brackets? If so, what's the simplest project that does cause a crash? Then try putting that same project content into the in-browser DemoFileSystem -- first make sure it crashes in brackets-shell, then try running it in cefclient.
  • If the project content is the same and it crashes desktop Brackets but not brackets-shell running the in-browser code, what's the remaining diff between full Brackets and the in-browser branch? You can start applying pieces of that diff onto the full Brackets src (disabling chunks of Brackets desktop code) until the crash goes away -- then we know which piece of JS code was missing from the in-browser branch and is needed to repro. (And then we might be able to either get that code working in-browser, or figure out a good enough way to emulate the same effect -- and that in turn lets us run the crash repro case in cefclient again).

[nethip]

@peterflynn I agree with you on getting this fixed from brackets-shell app side. I already started looking into it and I am trying to nail down to the actual bug. I am attacking this by replacing Brackets source files with that of CefClient and see if the crash goes away. After disabling app extensions and substituting client_handler, cef_main_window, cefclient_win and client_handler_win, with that of cefclient, the crash went away. So now I will have to nail down to the correct file to figure out which module is causing the crash. Looks like there is some new code that got added to 2171's cefclient's source.

[nethip]

@peterflynn @RaymondLim And about the repro, I am able to consistently crash appshell on my laptop with my demo_script worker example.

[nethip]

@peterflynn @RaymondLim I think I found the problem and the solution to that as well. I carefully looked at cefclient code and noticed that the destruction sequence is different to that of Bracket's.

In cefclient, destruction does not happen at WM_DESTROY, rather in ClientHandler::OnBeforeClose(). Inside OnBeforeClose(), they call CefQuitMessageLoop(). Then I checked in Brackets about the same destruction sequence. Looks like we are handling WM_DESTROY and when this message is received we do a PostQuitMessage(). Upon removing this handling and calling CefQuitMessageLoop() in OnBeforeClose(), the crash is gone! I did some unit testing and this does not crash even with my demo_script worker example anymore. I have put up the same for review. Please review this

adobe/brackets-shell#490

Let me know your comments.