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

Fix memory leak when using webFrame and spell checker #16770

Merged
merged 2 commits into from Feb 8, 2019

Conversation

Projects
None yet
4 participants
@zcbenz
Copy link
Member

zcbenz commented Feb 6, 2019

Description of Change

Close #15459.

When using spell checker in pages with sandbox, there are two sources of memory leak:

  1. Every time when page is reloaded, the code would create a new api::WebFrame, and the old one would be leaked since the renderer process is not restarted.
  2. The spell check client is only destroyed when web frame is destroyed, but reloading a frame does not destroy the corresponding blink::WebFrame.

This PR fixes the leak with 2 methods:

  1. Refactor the webFrame module so it does not create api::WebFrame instances, the native methods become simple getters without a global state. So no memory will be leaked on reload, and the code now is cleaner and uses less memory.
  2. The spell check client is now destroyed when the page context is destroyed, so it won't hold a reference to code in the old context, which prevents garbage collection of V8.

Note that this PR should be rebased instead of squashed as the commits have clear responsibilities.

Checklist

Release Notes

Notes: Fix memory leak when using webFrame and spell checker.

@zcbenz zcbenz requested a review from electron/reviewers as a code owner Feb 6, 2019

@zcbenz zcbenz force-pushed the web-frame-refactor branch 3 times, most recently from 6ada33d to 7703b05 Feb 6, 2019

zcbenz added some commits Feb 6, 2019

fix: do not create native api::WebFrame in webFrame
When reloading a page without restarting renderer process (for example
sandbox mode), the blink::WebFrame is not destroyed, but api::WebFrame
is always recreated for the new page context. This leaves a leak of
api::WebFrame.

@zcbenz zcbenz force-pushed the web-frame-refactor branch from 15cd616 to 437b06a Feb 6, 2019

@nitsakh

nitsakh approved these changes Feb 7, 2019

Copy link
Contributor

nitsakh left a comment

LGTM! 👍 Thanks!

@MarshallOfSound MarshallOfSound merged commit d16b581 into master Feb 8, 2019

9 checks passed

Absolute Zero
Semantic Pull Request ready to be merged or rebased
Details
appveyor: win-ia32-debug AppVeyor build succeeded
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-debug AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

release-clerk bot commented Feb 8, 2019

Release Notes Persisted

Fix memory leak when using webFrame and spell checker.

@MarshallOfSound MarshallOfSound deleted the web-frame-refactor branch Feb 8, 2019

@trop

This comment has been minimized.

Copy link
Contributor

trop bot commented Feb 8, 2019

I have automatically backported this PR to "5-0-x", please check out #16851

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment