fix: mitigate white screen flash after occlusion by disabling compositor recycling

[loc]

Description of Change

Fix #19858, caused by compositor recycling in response to OS occlusion events.

Compositor recycling is useful for Chrome because there can be many tabs and spinning up a compositor for each one would be costly. In practice, Chrome uses the parent compositor code path of browser_compositor_view_mac.mm; the NSView of each tab is detached when it's hidden and attached when it's shown. For Electron, there is no parent compositor, so we're forced into the "own compositor" code path, which seems to be non-optimal and pretty ruthless in terms of the release of resources. Electron has no real concept of multiple tabs per window, so it should be okay to disable this ruthless recycling altogether in Electron.

There's some risk:
a) it might degrade CPU performance
b) it might have a memory cost
c) I largely don't know what I'm doing

To satiate a), I loaded giphy.com before and after the patch with the window both visible and occluded. After the patch, the renderer and GPU processes still drop to zero, as we would hope.

No patch, visible:

No patch, occluded:

Patch, visible:

Patch, occluded:

Through inspection, I think I can allay concern b): the recycleable_compositor keeps around at least one compositor of the pool that it manages (per window). So, with this change, I don't think we're actually affecting anything. It's possible that detaching from the compositor frees up some surface buffers or something, but I imagine that's negligible.

For c) reviewers will have to weigh in with more context on potential risks to other features that I'm unaware of.

As a bonus, with this change, we're able to remove a workaround introduced in #18661. The occlusion event from macOS was errantly firing when fullscreening, causing the compositor to be recycled. Now that we don't do that, we no longer need the debounce.

Checklist

• PR description included and stakeholders cc'd
• npm test passes
• tests are changed or added
• relevant documentation is changed or added
• PR title follows semantic commit guidelines
• PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Fixed white flash after restoring an app from the background.

[codebytere]

patch lgtm given extra context from @MarshallOfSound - you'll need to resolve patch conflict though.

[deepak1556]

Electron has no real concept of multiple tabs per window, so it should be okay to disable this ruthless recycling altogether in Electron.

While this is true in general, @loc have you tested this change's impact for macOS native tabs code path ?

/cc requesting review from @brenca @zcbenz for compositor code path.

[MarshallOfSound]

have you tested this change's impact for macOS native tabs code path ?

We hadn't but now I have and the same got the results as our other tests. Renderers backgrounded in native tabs don't white flash but they do drop to 0% CPU usage when occluded 👍

[zcbenz]

This looks awesome!

[brenca]

LGTM, this shouldn't have any effect on OSR AFAIK, we are already treating those windows as visible all the time if I remember correctly.

[MarshallOfSound]

This appears to have destabilized a visibilityState test. Will investigate today 😄

[MarshallOfSound]

This appears to have destabilized a visibilityState test. Will investigate today 😄

So my debounce patch was hiding the fact that this test has been broken for a while. I've disabled it in this PR to unblock it and will investigate after it has landed.

[release-clerk]

Release Notes Persisted

Fixed white flash after restoring an app from the background.

[trop]

I was unable to backport this PR to "6-0-x" cleanly;
you will need to perform this backport manually.

[trop]

I was unable to backport this PR to "7-0-x" cleanly;
you will need to perform this backport manually.

[trop]

A maintainer has manually backported this PR to "6-0-x", please check out #19900

[trop]

A maintainer has manually backported this PR to "7-0-x", please check out #19901

[poiru]

@MarshallOfSound @zcbenz Unfortunately this patch caused a regression with BrowserViews. In our app, switching tabs switches the visible BrowserView. Due to a change in this PR, the BrowserViews are momentarily blank when you switch to them. Prior to this change and e.g. in Chrome (the browser), switching to a tab makes it immediately visible without any sort of flicker.

Reverting a bit of this PR restores the old and correct behavior:

 void BrowserCompositorMac::SetRenderWidgetHostIsHidden(bool hidden) {
-  render_widget_host_is_hidden_ = false;
+  render_widget_host_is_hidden_ = hidden;
   UpdateState();
 }

I'm not sure what to do here - should we revert this PR or should we try to find another solution? This is a critical issue for us, we cannot upgrade to 6.x and beyond due to this.

[MarshallOfSound]

should we revert this PR or should we try to find another solution?

IMO no, this fixes a pretty nasty bug which affects the majority of users. I'm unsure why it would negatively impact BrowserView users but that shouldn't mean we revert this.

I'd be open to a forward-fix PR here but I don't have time to dig into this right now.

[poiru]

@MarshallOfSound I managed to find a fix that resolves our issue without regressing the common case. See #20829.