fix: webview crash on iframe removal

[alexstrat]

Fixes #18788 #17890

Description of Change

Backports this Chromium fix as a patch in 5-0-0 branch.

As a consequence, remove the previous and temporary patch introduced in electron/libchromiumcontent#676

cc @codebytere @zcbenz @deepak1556

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: Fix webview crash on iframe removal

[alexstrat]

I cherry-picked the Chromium commit as it. Not sure it is the expected patch formatting for electron in the end.

[codebytere]

@alexstrat were you able to determine what specifically about this patch breaks the functionality? Ideally i'd love to pare down the patch as much as possible to just the components that are necessary to revert 🤔

[alexstrat]

@codebytere my interpretation is that:

Previously, Chromium was unable to detach a subframe of a (inner) webcontents inside a (outter) webcontens (for instance, in our case, a webview in a BrowserWindow). That's basically the statement I read in this comment and that led @zcbenz to this patch (we just don't detach).

The Chromium patch seems to fix exactly this case:

Prior to this change, any detachment of a proxy would cause the inner WebContents to get detached from the outer WebContents.

That's why I would argue to keep the whole patch as it.

That being said, my current knowledge of Chromium architecture did not allow me to:

• understand/find the root cause of the issue, so I'm providing what I'm only considering as an interpretation for now
• understand correctly how the patch in Chromium actually fixes the issue

Therefore, I think the interpretation and opinion of @zcbenz is necessary here.

[deepak1556]

Should also try to revert the manual management #14487 and see if the issue persists on reload. The new CL seems to address the concerns raised by Cheng's PR, but would be great to get in some tests.

@alexstrat can you add in tests for the iframe issues that are fixed by the new patch, seems like there are repros that could be turned into a test. Thanks!

[alexstrat]

Should also try to revert the manual management #14487 and see if the issue persists on reload. The new CL seems to address the concerns raised by Cheng's PR, but would be great to get in some tests.

I should rather do that on master, right?

@alexstrat can you add in tests for the iframe issues that are fixed by the new patch, seems like there are repros that could be turned into a test. Thanks!

Yes, I’ll try that. Though the difficulty is that the webview does not really crash, it just stops to render. So, I still don’t know how to catch that in a test. I’d appreciate ideas!

[deepak1556]

Though the difficulty is that the webview does not really crash, it just stops to render. So, I still don’t know how to catch that in a test. I’d appreciate ideas!

Does that trigger webContents unresponsive event ? we could listen for that.

[zcbenz]

Can you double check this wouldn't cause regression on #14211?

[alexstrat]

@deepak1556 Regarding testing, neither the 'unresponsive' nor 'crashed' events are fired when the webview freezes.

I tried to hook into the 'paint' event as well but it does not seem I'm able to catch the paint events for the webview and the paint event for the outer webcontents (BrowserWindow) keeps flowing after iframe removal. 😞

Have you other suggestion?

[alexstrat]

@zcbenz I tried to reproduce #14211 with the instructions provided in description (I waited for ~20 reloads) and could not reproduce it => this patch does NOT cause a regression on #14211

Edit: after reverting #14487 on top of this patch, I did not reproduce #14211. So I guess this patch makes #14487 not useful anymore.

I can suggest a PR that reverts #14487 but I think we should 1/ first backport this CL like suggested here so that 5-0-0 matches 6-0-0 and master, and 2/ revert #14487 on master and backport the revert in 6-0-0 and 5-0-0.

[zcbenz]

I can suggest a PR that reverts #14211 but I think we should 1/ first backport this CL like suggested here so that 5-0-0 matches 6-0-0 and master, and 2/ revert #14211 on master and backport the revert in 6-0-0 and 5-0-0.

The plan sounds good to me.

[deepak1556]

Can you revert the deletion of disable_detach_webview_frame.patch otherwise it doesn't match 6-0-x and master. And as you suggested, a separate PR targeting master can then remove this patch along with the manual management, and probably add some tests.

[alexstrat]

Can you revert the deletion of disable_detach_webview_frame.patch otherwise, it doesn't match 6-0-x and master.

@deepak1556 good catch, yes!

✅done:

• I made sure the CL is the first patch executed
• I took disable_detach_webview_frame.patch as it is just after bumping chromium to 76.0.3783.1 so that it matches as much as possible 6-0-x and master (even if it does not seem to have evolved since then)

[release-clerk]

Release Notes Persisted

Fix webview crash on iframe removal

[subbiah95]

@deepak1556 , I have a similar crash going on in V4.0.0, I am using webview, the crash happened twice, 3 and 5 seconds after start
the crash points to..
/content/browser/frame_host/render_frame_host_manager.cc
GetProxyToOuterDelegate()->SetChildRWHView(child_rwhv, nullptr);

Is my crash same as this one ?
I am not attaching the dump file as I have changed few codes in a few modules(nothing to do with crash)

[deepak1556]

@subbiah95 Can't say for sure without the dump, but I would suggest to try newer versions of electron where the crash might be fixed, V4 is quite old and unsupported release line.