-
Notifications
You must be signed in to change notification settings - Fork 15.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: ginify BrowserView #23578
refactor: ginify BrowserView #23578
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to force WebContents of BrowserView to close after each test, currently the tests are failing with:
not ok 250 webContents module getAllWebContents() API returns an array of web contents
AssertionError: expected [ Array(21) ] to have a length of 3 but got 21
at Context.<anonymous> (electron/spec-main/api-web-contents-spec.ts:37:27)
Also I think the removed APIs should be mentioned in breaking changes.
@zcbenz removed APIs are marked experimental (mostly, though I think the one omission is a mistake and should have been marked experimental). Do we still list breaking changes when APIs are experimental? |
I could not find any documents on what to do with removing experimental APIs, I'm good without mentioning it then. |
WebContents will stay alive as long as there's a page loaded in it. As you've discovered, the BrowserView can be GC'd while the WebContents stays alive, if there's a page loaded. To make sure the WebContents is closed, you must call This is a common pattern in JS, consider e.g. creating sockets in Node.js: for (let i = 0; i < 40; i++) {
let socket = net.connect({port: 1234}, (s) => { /* ... */ })
} Those sockets will remain alive unless explicitly closed. |
@nornagon You're right, but the difference is that sockets have a destroy function whereas BrowserView and webContents do not. I don't mind explicit resource management, we were destroying BrowserView using BrowserView.destroy before. The problem is:
So to remove the process leak, one needs to call an undocumented function which is unknowable by users of Electron and without it, there's no proper lifecycle management. This is an issue in our case because we dynamically create BrowserViews and use them as tabs in our application. |
@yohan1234 ah, yes, you're right. I had it in my head that I agree that this is a missing piece of our API. I've opened a new issue to track it: #26929 |
@nornagon Excellent! |
@pushkin- Is it bound to a BrowserWindow when you destroy it? to remove it prior to destroy: BrowserWindow.setBrowserView(null) |
@yohan1234 removing it from the window fixed the crash. Thanks! |
So destroying the webcontents fixes the process leak, but the entries are still not removed from my |
@pushkin- Not sure what you mean. All references need to be removed for garbage collection. Do you mean some kind of internal map in BrowserWindow or something in your application? |
@yohan1234 The WeakMap structure that I created from above (what nornagon posted) still has entries for destroyed webcontents:
|
@pushkin- Sorry, where is the code? |
@yohan1234 I'm referring to this. My specific code is like:
And when I close the window, I do:
And the entries are never removed from the WeakMap. |
@pushkin- .destroy() only destroys the internal V8 stuff in webcontents, it doesn't nuke the chain of things. In your code it seems that it might be that webContents (and BrowserWindow/BrowserView by proxy) leak forever |
Any news on a way to stop a BrowserView properly and unallocate memory as we were doing with |
@MatthieuLeMee we seem to have the same use case. @nornagon opened up a proposal but I don't know who the maintainer is: Perhaps the practical approach is to unmerge this PR? |
|
@nornagon Unless it's a bug in my code, I pasted an example above where I destroyed the webcontents, but the entry is not cleared from the I could open a separate issue for this if you want (if we don't already have one) |
@pushkin- yeah, let's go ahead and open a new issue, I think. If the BVs aren't being GC'd even after the webContents is destroyed then that'd be a bug. I don't think WeakMap would factor into it. |
Hi guys, I am currently upgrading from v7 (long way ahead!), and I don't get how I should replace those methods. Here's my use case: I want some renderer code to open a dialog. To do so, I've created an IPC handler, and I expose a method like this in preload script: First, I am not sure if I am allowed to call Then, the handler is like this: const showOpenDialogSyncHandler: IpcMainHandlerDef = [
showOpenDialogSyncChannel,
async (evt: any, ...args: any) => {
const parent = evt.sender
? BrowserWindow.fromWebContents(evt.sender)
: null;
evt.returnValue = dialog.showOpenDialogSync(parent, ...args);
return evt;
},
]; So to get the parent, I need to get the BrowserWindow based on the event sender WebContents. How should I achieve that without those methods? By the way they are still documented and nothing tells that they are deprecated or removed in current doc: https://www.electronjs.org/docs/latest/api/browser-window#browserwindowfromwebcontentswebcontents I have some hard time upgrading the code from "remote" to the new APIs Edit: ok I found more info in the hidden comments, I missed them. It seems that I need to maintain my own mapping. Edit 2: man it's BrowserView that was affect by this, not BrowserWindow sorry for the inconvenience ^^ |
Description of Change
This refactors
BrowserView
to usegin::Wrappable
instead ofgin_helper::TrackableObject
. It also makes some changes to the API, removing theid
and related methods, and making the object self-owned instead of forcing manual memory management on the user.I'm not yet convinced that the new code doesn't leak
BrowserView
s forever in certain (or all) situations, but putting it up for review because at least the tests pass!Checklist
npm test
passesRelease Notes
Notes: Removed experimental APIs:
BrowserView.{fromId, fromWebContents, getAllViews}
and theid
property ofBrowserView
.