New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: webview crash on iframe removal #18976
fix: webview crash on iframe removal #18976
Conversation
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 | ||
From: =?UTF-8?q?Alexandre=20Lach=C3=A8ze?= <alexandre.lacheze@gmail.com> | ||
Date: Tue, 25 Jun 2019 17:36:08 +0200 | ||
Subject: Portals: Fix crash when detaching OOPIF from portal. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cherry-picked the Chromium commit as it. Not sure it is the expected patch formatting for electron in the end.
@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 🤔 |
@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:
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:
Therefore, I think the interpretation and opinion of @zcbenz is necessary here. |
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! |
I should rather do that on master, right?
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! |
Does that trigger |
Can you double check this wouldn't cause regression on #14211? |
@deepak1556 Regarding testing, neither the I tried to hook into the Have you other suggestion? |
@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 |
Can you revert the deletion of |
@deepak1556 good catch, yes! ✅done:
|
Release Notes Persisted
|
@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 Is my crash same as this one ? |
@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. |
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#676cc @codebytere @zcbenz @deepak1556
Checklist
npm test
passesRelease Notes
Notes: Fix webview crash on iframe removal