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: use a WeakPtr so we do not UAF the store in FunctionLifetimeMonitor #22056

Merged
merged 1 commit into from Feb 9, 2020
Merged

fix: use a WeakPtr so we do not UAF the store in FunctionLifetimeMonitor #22056

merged 1 commit into from Feb 9, 2020

Conversation

MarshallOfSound
Copy link
Member

We already did this for the ObjectLifeMonitor, should also do it for the FunctionLifetimeMonitor

Notes: Fixed issue where renderers could crash during GC when using the contextBridge module

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Feb 5, 2020
loc
loc approved these changes Feb 5, 2020
View reviewed changes
Copy link
Contributor

@loc loc left a comment

Choose a reason for hiding this comment

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

lgtm

nornagon
nornagon reviewed Feb 5, 2020
View reviewed changes
shell/renderer/api/electron_api_context_bridge.cc

private:
context_bridge::RenderFramePersistenceStore* store_;
base::WeakPtr<context_bridge::RenderFramePersistenceStore> store_;
Copy link
Member

Choose a reason for hiding this comment

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

Would it be reasonable to eagerly delete the FLMs when the store is deleted? rather than leaving them hanging around until they happen to wake up and notice they're useless now since their store is gone

Copy link
Member Author

Choose a reason for hiding this comment

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

@nornagon They will all be deleted at the same time (give or take some milliseconds). The store is only ripped out when the context is destroyed. The function monitors will be ripped apart during the final GC run

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Feb 6, 2020
@MarshallOfSound MarshallOfSound merged commit dafbf04 into master Feb 9, 2020
@release-clerk
Copy link

release-clerk bot commented Feb 9, 2020

Release Notes Persisted

Fixed issue where renderers could crash during GC when using the contextBridge module

@MarshallOfSound MarshallOfSound deleted the weak-ptr-function-monitor branch February 9, 2020 04:49
This was referenced Feb 9, 2020
Merged
Merged
Merged
@trop
Copy link
Contributor

trop bot commented Feb 9, 2020

I have automatically backported this PR to "7-1-x", please check out #22112

@trop trop bot removed the target/7-1-x label Feb 9, 2020
@trop
Copy link
Contributor

trop bot commented Feb 9, 2020

I have automatically backported this PR to "9-x-y", please check out #22113

@trop
Copy link
Contributor

trop bot commented Feb 9, 2020

I have automatically backported this PR to "8-x-y", please check out #22114

@sofianguy sofianguy added this to Fixed in 8.0.1 in 8.2.x Feb 15, 2020
@sofianguy sofianguy added this to Fixed in 7.1.12 in 7.2.x Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zcbenz zcbenz zcbenz approved these changes

@loc loc loc approved these changes

@nornagon nornagon Awaiting requested review from nornagon

Assignees
No one assigned
Labels
None yet
Projects
No open projects
7.2.x
Fixed in 7.1.12
8.2.x
Fixed in 8.0.1
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@MarshallOfSound @nornagon @zcbenz @loc