fix: stale renderer process on quit

[deermichel]

Description of Change

Closes #32884.

In the mentioned Chromium roll I observed the following:

• patches/chromium/content_browser_main_loop.patch which replaced QuitClosure() with QuitWhenIdleClosure() got removed
• this appears again in the modified shell/browser/electron_browser_main_parts.cc, but now with QuitClosure()

If I replace QuitClosure() with QuitWhenIdleClosure(), all processes terminate properly as it was the case before the Chromium roll. Since this is Electron code, it would explain why Chromium doesn't suffer from the same issue.

Because I lack context of what exactly this code does, please double-check that this change is fine :). But apparently, regressions were anticipated according to this commit message. cc @MarshallOfSound @ckerr

cc @miniak @deepak1556 @zarubond @dsanders11

Checklist

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

Release Notes

Notes: Fixed stale renderer process when application is quit while renderer is busy.

[deermichel]

@dsanders11 this would probably mean you can revert the upstream commit you made 😆 . But first, please confirm that this change works :)

[nornagon]

Seems okay, by analogy to https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/chrome_browser_main.cc;l=1797-1798;drc=59caa8489dd18412f963596dfc0292e6586f2611

[dsanders11]

@deermichel, I've tested this on the machine where I see reproductions, and there's good news and bad news.

Good news, your gist in the issue readily reproduces when using v19 nightlies, and I can confirm that the change in this PR stops it from reproducing with your gist.

Bad news, I still see the issue reproduce just by running and stopping Fiddle manually with the default fiddle. I can't seem to replicate it with app.exit in the fiddle itself, at least not so far, so it could be related to the SIGTERM. However, old versions of Electron don't reproduce that way, and it's gotten significantly worse since the latest Chromium roll, I can reproduce consistently with a few run/stops.

So I think this PR is valid and a step in the right direction, but I don't think it fully solves the issue, unfortunately.

[deermichel]

Ok, that's a bummer - but yeah then it's most likely related to the "arbitrary killing" of the operating system. Nonetheless, once this lands I at least want to check if that solves the user-facing issue we have in Skype (and I'm pretty optimistic it does).

[dsanders11]

I've played with it further, and seems the only spot where it still reproduces is with 'Stop' in Fiddle. Oddly it doesn't reproduce with an 'End Process' from Task Manager. 🤔 While that's annoying since you can trip it while running Fiddle, it's not a production concern.

LGTM

[deepak1556]

👍

[deepak1556]

Failing tests are unrelated, merging.

[release-clerk]

Release Notes Persisted

Fixed stale renderer process when application is quit while renderer is busy.

[trop]

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

[trop]

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

[trop]

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

[trop]

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

[trop]

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

[indutny-signal]

Just discovered this. Fantastic work everyone!