[CLOSED] Terminate tern worker on quit

[core-ai-bot]

Issue by RaymondLim
Tuesday Nov 25, 2014 at 23:30 GMT
Originally opened as adobe/brackets#10020

---

This fixes issue #7683.

---

RaymondLim included the following code: https://github.com/adobe/brackets/pull/10020/commits

[core-ai-bot]

Comment by RaymondLim
Monday Dec 01, 2014 at 22:09 GMT

---

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

[core-ai-bot]

Comment by RaymondLim
Monday Dec 08, 2014 at 00:30 GMT

---

@peterflynn,@nethip Can you review and merge?

[core-ai-bot]

Comment by nethip
Monday Dec 08, 2014 at 09:05 GMT

---

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

[core-ai-bot]

Comment by peterflynn
Monday Dec 08, 2014 at 20:50 GMT

---

@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.

[core-ai-bot]

Comment by peterflynn
Monday Dec 08, 2014 at 21:06 GMT

---

@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).

[core-ai-bot]

Comment by nethip
Tuesday Dec 09, 2014 at 05:49 GMT

---

@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.

[core-ai-bot]

Comment by nethip
Tuesday Dec 09, 2014 at 05:52 GMT

---

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

[core-ai-bot]

Comment by nethip
Tuesday Dec 09, 2014 at 15:31 GMT

---

@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.