-
Notifications
You must be signed in to change notification settings - Fork 116
Conversation
after further testing .. I am no Expert at node so I need pointers at how to fix this problem. |
Did this work previously? Thanks for the references for what you based this on 👍 |
I am using Electron 8 so "allowRendererProcessReuse" is turned off by default but in Electron 9 it will be enabled by default anyway when I turn it on with the current module, the whole app crash off course because the module is non-context-aware. but with my Pull Request the App Continue to Work Normally Unless you actually have to Reuse the Renderer Process. I guess the problem is that I have to detect when the Context has changed and unregister the old callbacks. |
The old callbacks will not work after the renderer has been refreshed because the previous browser context no longer exists. The changes so far in this PR allow the module to be used in recent Electron but it'll probably be leaking memory everywhere. We'll almost certainly need to implement the cleanup hook to ensure that resources are freed. Here is an example of the required hook. My c++ skills are not up to scratch so I'm not 100% sure what needs to be changed. It looks like at a minimum Some modules have decided that this is a good time to migrate to N-API which doesn't have these issues. |
@timfish Thanks for the extra details on what we need to do! ❤️ Is this PR good as a small iteration? Does it break any use cases that worked before? If it fixes some use case and not our existing ones, I'm ok to merge and create an issue to track the debt (known issue). Here is the issue we have for switching to N-API but I haven't looked into this at all: #42 |
as far as I know this PR is not breaking existing compatibility but it will need more work and testing ( I will try to work on testing in my free time but it's a slow pace) as for N-API .. I honestly have no Idea How to migrate. it will need some research from me. |
Hello,
this is a solve for the issue #102
I tested this on Electron 8.3.4 with
app.allowRendererProcessReuse = true;
and
app.allowRendererProcessReuse = false;
in both tests the module behaves as expected.
I don't know if there is further modification to be done (Cleanup or some other Tweaks).
this PR is Inspired by this PR
and this Commit
I hope it helps :D