1654048 - ImplCycleCollectionUnlink(std::shared_ptr<webgl::NotLostData>&) doesn't release the object

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect, P1)

Description

[Kelsey Gilbert [:jgilbert]]

This could cause not all memory to be released when the context is CC'd.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1654048 - Release NotLost in CC unlink for `std::shared_ptr<webgl::NotLostData>&`.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f2cb7c9bcce
Release NotLost in CC unlink for `std::shared_ptr<webgl::NotLostData>&`. r=lsalzman

Comment 3

[Kelsey Gilbert [:jgilbert]]

This seems like something we should uplift to Beta.
mccr8, what do you think? Would the pre-patch version leak like I fear?

Comment 4

[Andrew McCreight [:mccr8]]

(In reply to Jeff Gilbert [:jgilbert] from comment #3)

This seems like something we should uplift to Beta.
mccr8, what do you think? Would the pre-patch version leak like I fear?

Sure, it would be good to uplift. A missing unlink won't necessarily result in a leak. If you have a cycle, you just have to break one link in the chain for it to all fall apart, so it depends on precisely what sort of cycles this reference is involved in.

Comment 5

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/2f2cb7c9bcce

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Too late to uplift this to Fx79 at this point given that we're already in RC week, but I'd still consider taking this on ESR78 next cycle.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Hi Jeff, does this need an ESR78 approval request?

Comment 8

[Kelsey Gilbert [:jgilbert]]

I don't think it's important enough, and it's supposed to not be necessary, I now think. (CC code is inscrutable though)
If we do take it, there's another fix needed, or this can cause UAFs by itself.