Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Webview does not dispose properly #50556

Closed
bpasero opened this issue May 28, 2018 · 8 comments
Closed

Webview does not dispose properly #50556

bpasero opened this issue May 28, 2018 · 8 comments
Assignees
@mjbvz
Labels
bug Issue identified by VS Code Team member as probable bug electron Issues and items related to Electron insiders-released Patch has been released in VS Code Insiders perf upstream Issue identified as 'upstream' component related (exists outside of VS Code) upstream-issue-linked This is an upstream issue that has been reported upstream verified Verification succeeded webview Webview issues
Milestone
May 2021

Comments

@bpasero
Copy link
Member

bpasero commented May 28, 2018

I am testing from the ben/editor branch that contains the changes for Grid. If you split an editor to the side and then close that editor, we now actually dispose the editor and no longer keeping it around as we used to before. This means that we now rely on the editor freeing up its resources properly.

For any webview editor (e.g. markdown preview or extensions editor) I see lots of controls still hanging around after:

  • open a single of these editors
  • split it once
  • close the second instance
  • create a heap snapshot
  • repeat split/close a couple of times
  • create a heap snapshot again
  • compare it to the first

image
@bpasero bpasero added bug Issue identified by VS Code Team member as probable bug webview Webview issues workbench-editor-grid Grid layout issues in the editor area labels May 28, 2018
@mjbvz mjbvz added this to the June 2018 milestone May 31, 2018
mjbvz added a commit that referenced this issue Jun 14, 2018
@mjbvz
Working on fixing some webview leaks
e3ea9b1 
#50556
@mjbvz
Copy link
Contributor

mjbvz commented Jun 14, 2018

@bpasero I've fixed some of the leaks on our side but the leaks still exist. Can you take a this possible electron issue to see if you have any thoughts on the problem: electron/electron#13243

@bpasero
Copy link
Member Author

bpasero commented Jun 14, 2018

@mjbvz did you test with our previous Electron 1.7.12 to see if this is a regression from the 2.0 update?

@mjbvz
Copy link
Contributor

mjbvz commented Jun 14, 2018

Not a regression from 1.7. I've minimized the impact of this problem. Certainly possible I'm doing something wrong in the code here

@mjbvz mjbvz modified the milestones: June 2018, Backlog Jun 21, 2018
@mjbvz mjbvz added upstream Issue identified as 'upstream' component related (exists outside of VS Code) electron Issues and items related to Electron perf labels Jun 21, 2018
@ivankravets

This comment has been minimized.

Sign in to view
@mjbvz

This comment has been minimized.

Sign in to view
@bpasero bpasero removed the workbench-editor-grid Grid layout issues in the editor area label Sep 12, 2018
@mjbvz mjbvz modified the milestones: Backlog, October 2019 Oct 8, 2019
@mjbvz
Copy link
Contributor

mjbvz commented Oct 16, 2019

I investigated this again and made a few more fixes but am still blocked by the upstream electron issue.

The best long term fix here is probably to move off of webviews and to normal iframes instead

@mjbvz mjbvz modified the milestones: October 2019, Backlog Oct 16, 2019
@ivankravets
Copy link

How to use native iframes? Do you have example?

mjbvz added a commit that referenced this issue Oct 16, 2019
@mjbvz
Fixing some possible leaks in electron webviews
8c4e220 
For #50556
@bpasero bpasero added the upstream-issue-linked This is an upstream issue that has been reported upstream label Oct 18, 2019
@mjbvz mjbvz mentioned this issue Oct 24, 2019
Closed
2 tasks
@mjbvz mjbvz modified the milestones: Backlog, May 2021 May 6, 2021
@mjbvz mjbvz closed this as completed in a3cddc8 May 6, 2021
@mjbvz
Copy link
Contributor

mjbvz commented May 6, 2021 •
edited

Tested using iframe based webviews using the following steps:

  1. Set "webview.experimental.useIframes": true to force iframes to be used
  2. Open a markdown file. Open the preview once and then close it
  3. Snapshot
  4. Now, multiple times: open the preview and then close it
  5. With the preview closed, take another snapshot (give GC a few seconds to run).

You can now compare between the snapshots. Search for webview and confirm that no webview elements are still around. Make sure you are looking at the objects allocated between snapshot X and Y since there will still be some static webview objects around (such as the services):

Screen Shot 2021-05-05 at 5 41 19 PM

@bpasero bpasero added the verified Verification succeeded label Jun 2, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Jun 20, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@mjbvz mjbvz

Labels
bug Issue identified by VS Code Team member as probable bug electron Issues and items related to Electron insiders-released Patch has been released in VS Code Insiders perf upstream Issue identified as 'upstream' component related (exists outside of VS Code) upstream-issue-linked This is an upstream issue that has been reported upstream verified Verification succeeded webview Webview issues
Projects
None yet
Milestone
May 2021
Development

No branches or pull requests
4 participants
@bpasero @ivankravets @mjbvz and others