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.

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: dangling raw_ptr in OSRWHV destructor #41088

Merged
merged 1 commit into from
Jan 25, 2024
Merged

Conversation

ckerr
Copy link
Member

@ckerr ckerr commented Jan 24, 2024

Description of Change

delegated_frame_host_ holds a pointer to delegated_frame_host_client_. Since delegated_frame_host_client_ was being destroyed first, that pointer was dangling in the OSRWHV destructor.

Also, make these two unique_ptr fields const since they point to the same objects for the lifespan of the OSRWHV.

Checklist

Release Notes

Notes: none

`delegated_frame_host_` holds a pointer to `delegated_frame_host_client_`.
Since `delegated_frame_host_client_` was being destroyed first, that
pointer was dangling in the OSRWHV destructor.

Also, make these two unique_ptr fields `const` since they point to the
same objects for the lifespan of the OSRWHV.
@ckerr ckerr added semver/patch backwards-compatible bug fixes target/28-x-y PR should also be added to the "28-x-y" branch. target/29-x-y PR should also be added to the "29-x-y" branch. labels Jan 24, 2024
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jan 24, 2024
Comment on lines 250 to 251
compositor_.reset();
root_layer_.reset();
Copy link
Member

Choose a reason for hiding this comment

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

Do these two calls need to be explicit in the destructor ?

Copy link
Member Author

@ckerr ckerr Jan 24, 2024

Choose a reason for hiding this comment

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

@deepak1556 good catch, I was going to remove them for that reason: you're right, they're entirely redundant.

I decided to leave them alone here because it's a little off-topic for this bugfix PR, and I have another cleanup PR in mind where it's a better fit.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jan 25, 2024
@zcbenz zcbenz merged commit 921da72 into main Jan 25, 2024
26 checks passed
@zcbenz zcbenz deleted the fix/osr-rhv-dtor-bug branch January 25, 2024 01:15
Copy link

release-clerk bot commented Jan 25, 2024

No Release Notes

@trop
Copy link
Contributor

trop bot commented Jan 25, 2024

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

@trop trop bot added in-flight/28-x-y and removed target/28-x-y PR should also be added to the "28-x-y" branch. labels Jan 25, 2024
@trop
Copy link
Contributor

trop bot commented Jan 25, 2024

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

@trop trop bot added in-flight/29-x-y merged/28-x-y PR was merged to the "28-x-y" branch. merged/29-x-y PR was merged to the "29-x-y" branch. and removed target/29-x-y PR should also be added to the "29-x-y" branch. in-flight/28-x-y in-flight/29-x-y labels Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged/28-x-y PR was merged to the "28-x-y" branch. merged/29-x-y PR was merged to the "29-x-y" branch. semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants