Closed Bug 1654048 Opened 2 months ago Closed 2 months ago

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

Categories

(Core :: Canvas: WebGL, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla80
Tracking Status
firefox-esr68 --- unaffected
firefox-esr78 --- wontfix
firefox78 --- wontfix
firefox79 --- wontfix
firefox80 --- fixed

People

(Reporter: jgilbert, Assigned: jgilbert)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file)

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

Summary: ImplCycleCollectionUnlink(std::shared_ptr<webgl::NotLostData>&) doesn't clear the pointer → ImplCycleCollectionUnlink(std::shared_ptr<webgl::NotLostData>&) doesn't release the object
Pushed by jgilbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2f2cb7c9bcce
Release NotLost in CC unlink for `std::shared_ptr<webgl::NotLostData>&`. r=lsalzman

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

Flags: needinfo?(continuation)

(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.

Flags: needinfo?(continuation)
Status: NEW → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla80

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.

Hi Jeff, does this need an ESR78 approval request?

Flags: needinfo?(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.

Flags: needinfo?(jgilbert)
You need to log in before you can comment on or make changes to this bug.