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

fix: correctly destroy spellcheck client #16448

Merged
merged 2 commits into from Jan 24, 2019
Merged

fix: correctly destroy spellcheck client #16448

merged 2 commits into from Jan 24, 2019

Conversation

nitsakh
Copy link
Contributor

@nitsakh nitsakh commented Jan 18, 2019 •
edited

Description of Change

This change addresses #15459.
The spellcheck client wasn't destroyed when the renderer goes away.
This change adds an observer which takes care of the destruction.

Checklist

Release Notes

Notes: Fixed webFrame.setSpellCheckProvider memory leak

@nitsakh nitsakh requested review from zcbenz, deepak1556 and a team January 18, 2019 19:52
@nitsakh
Copy link
Contributor Author

nitsakh commented Jan 18, 2019

We should backport this to 3-0-x also, right?

deepak1556
deepak1556 requested changes Jan 19, 2019
View reviewed changes
Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with minor changes, thanks!

atom/renderer/api/atom_api_web_frame.cc Outdated Show resolved Hide resolved
atom/renderer/api/atom_api_web_frame.cc Outdated Show resolved Hide resolved
atom/renderer/api/atom_api_web_frame.cc Outdated Show resolved Hide resolved
@codebytere
Copy link
Member

We should backport this to 3-0-x also, right?

@nitsakh since 3-1-x went stable 3-0-x is only receiving security fixes now i believe

@nitsakh nitsakh force-pushed the fix-spell-leak branch 2 times, most recently from f411cd2 to b3d1a24 Compare January 24, 2019 05:16
This was referenced Jan 24, 2019
Merged
Merged
Merged
deepak1556
deepak1556 approved these changes Jan 24, 2019
View reviewed changes
Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@codebytere codebytere changed the title fix: Destroy spellcheck client fix: correctly destroy spellcheck client Jan 24, 2019
@codebytere codebytere merged commit e6952e2 into master Jan 24, 2019
@release-clerk
Copy link

release-clerk bot commented Jan 24, 2019

Release Notes Persisted

Fixed webFrame.setSpellCheckProvider memory leak

@codebytere codebytere deleted the fix-spell-leak branch January 24, 2019 16:19
@trop
Copy link
Contributor

trop bot commented Jan 24, 2019

I was unable to backport this PR to "4-0-x" cleanly;
you will need to perform this backport manually.

@trop
Copy link
Contributor

trop bot commented Jan 24, 2019

I was unable to backport this PR to "3-1-x" cleanly;
you will need to perform this backport manually.

@sofianguy sofianguy added this to Fixed in 3.1.3 in 3.1.x Jan 31, 2019
@sofianguy sofianguy added this to 5.0.0-beta.2 in 5.0.x Feb 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@deepak1556 deepak1556 deepak1556 approved these changes

@codebytere codebytere codebytere approved these changes

@MarshallOfSound MarshallOfSound MarshallOfSound approved these changes

@zcbenz zcbenz Awaiting requested review from zcbenz

Assignees
No one assigned
Labels
None yet
Projects
No open projects
3.1.x
Fixed in 3.1.3
5.0.x
Fixed in 5.0.0-beta.2
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@nitsakh @codebytere @deepak1556 @MarshallOfSound