1996718 - Ensure BrowserChild owned by ContentProcessController hasn't yet been destroyed before using it.

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P3)

Description

[Hiroyuki Ikezoe (:hiro)]

We have a null check, for example here, but the BrowserChild instance may have been torn down.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: (secure)

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/87050d5baa59
https://hg.mozilla.org/integration/autoland/rev/969f60d3dd6b
Check whether BrowserChild has not yet been destroyed in each function of ContentProcessController. r=botond

Comment 3

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/969f60d3dd6b

Comment 4

[BugBot [:suhaib / :marco]]

The patch landed in nightly and beta is affected.
:hiro, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox146 to wontfix.

For more information, please visit BugBot documentation.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

We'd keep watching whether this change affected on bug 1995079 or not.

Comment 6

[Donal Meehan [:dmeehan]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)

We'd keep watching whether this change affected on bug 1995079 or not.

:hiro, we are in the final week of beta for Fx146. Any updates on whether you will add an uplift request?
I see you set esr140 to wontfix? (This does graft cleanly to esr140)

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Gah, I am sorry, I thought I set wontfix to 146.

Comment 8

[Donal Meehan [:dmeehan]]

:dveditz, before I remove the tracking+ flags, just checking if you have any concerns with the esr140 and 146 wontfix?
(I see you set it as sec-moderate)

Comment 9

[Daniel Veditz [:dveditz]]

The ESR-140 wontfix was a mistake; we should take it when we take this on main. The 147 cycle is fine if we're worried about regressions and want to make sure it's fixing the stalled sec-high signatures like bug 1995079 and bug 2000508. The signature from bug 2000508 also shows up as a UAF in ESR-115 so I'd like to wait a bit before declaring "wontfix" on that branch.

UAF singleton bp-91c576d3-fb45-4e3c-8db8-170d50251031 might be related to this, too. It's the only UAF crash I see that has ContentProcessController explicitly on the stack.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Feel free to add ESR140 and ESR115 uplift nominations whenever you're ready to do so.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

I am not going to request uplift since this change appears to be not mitigating bug 1995079.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Not tracking this for ESR115 uplift in that case.

Do we want to get this on ESR140 just for the sake of consistency & hardening? From comment 9, it sounds like a yes to me.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Yeah, it was a mistake. but I am not going to uplift to ESR either.

The patch here doesn't fix bug 1995079 at all.

(In reply to Daniel Veditz [:dveditz] from comment #9)

UAF singleton bp-91c576d3-fb45-4e3c-8db8-170d50251031 might be related to this, too. It's the only UAF crash I see that has ContentProcessController explicitly on the stack.

This one is indeed related, but its underlying cause is bug 1995079 itself, it's purely in DOM. ContentProcessController is one of the sites which ends up triggering a function causing bug 1995079.