Ignore callbacks if the browser instance has been deleted #282
Conversation
|
@JeffryBooher It looks like this fixes adobe/brackets#4365. Can you try reproducing adobe/brackets#4331 with this branch? |
ghost
assigned 
|
@gruehle I built your version of the shell and it didn't crash in the build folder but when I copied the build to my install folder it crashed so I don't believe this fixes the crash. |
|
Yeah, I was running from the build folder. That would explain why I couldn't repro the crash. Oh well.... Were you able to repro the mac crash? |
|
@gruehle so an Update on this: after trying your branch and still getting the crash, I went back to Sprint 27 and tried a build without a FileIndexManager (all of the API functions just returned resolved promises with empty arrays and all internal functions removed). It stopped crashing so I launched dev tools to see if there were any exceptions or console logs. There was one function that I had screwed up and was throwing an exception so I fixed it and relaunched Brackets (without dev tools) it started crashing again. So after thinking about what was different for a few minutes, I realized that dev tools may have prevented the crash so I tried it again with dev tools running and, instead of crashing, I got the white screen of death. So I knew your shell changes fixed the white screen of death so I tried running dev tools with your branch and no more crash. So I went back to see what we put in the console, because that had put other CEF based tools over the limit in the past and caused problems (but I'm pretty sure that issue was fixed). We don't put much but, for fun, I went to the cef ref app to see if any output in the console log would cause a crash on reload and Google doesn't put anything in the log but the standard login page at my.yahoo.com does. It didn't crash the ref app when reloading, though. Any ideas? |
| // Async callbacks could be initiated after a browser instance has been deleted, | ||
| // which can lead to bad things. If the browser instance has been deleted, don't | ||
| // invoke this callback. | ||
| if (context->GetBrowser()) { |
There was a problem hiding this comment.
what's the difference between what is returned here and what is passed into this function? I almost think we should be checking what's passed in since we use it in the else case for the outer if to get a pointer to the brackets js api.
Otherwise I think this change is fine -- just trying to explore solutions for other CEF crashers...
There was a problem hiding this comment.
In the mac reload crash scenario, the passed in browser parameter and browser.get() were both non-null. The only test that prevented the crash was context->GetBrowser().
|
I'm going to merge this in. It looks safe and I think we need it to help solve the other CEF crasher. |
Ignore callbacks if the browser instance has been deleted
Ignore callbacks if the browser instance has been deleted
Fix (workaround?) for adobe/brackets#4522
If the browser instance has been deleted between the time an async call was made and it's callback function invoked, ignore the callback. This is safe to do since the browser instance itself has been deleted, so there is no way any client code is still running.