Skip to content
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

Iframe webview: search can crash the process #126729

Closed
bpasero opened this issue Jun 19, 2021 · 7 comments
Closed

Iframe webview: search can crash the process #126729

bpasero opened this issue Jun 19, 2021 · 7 comments
Assignees
Labels
electron Issues and items related to Electron freeze-slow-crash-leak VS Code crashing, performance, freeze and memory leak issues insiders-released Patch has been released in VS Code Insiders sandbox Running VSCode in a node-free environment verified Verification succeeded webview Webview issues
Milestone

Comments

@bpasero
Copy link
Member

bpasero commented Jun 19, 2021

Extracted from #125856 (for #96307)

I can reproduce 2 scenarios where a crash occurs:

Crash when document changes

  • open preview
  • open file as well
  • start search in preview
  • change file by e.g. adding a word that is searched
  • 💥 crash

0045564b-de8d-4298-972c-37c9c6e6650a.dmp.zip

Crash when reopening preview

  • open a preview
  • open find and start typing
  • quickly close the preview
  • open preview again
  • 💥 crash

03b9b157-63f0-4749-b3e4-699582f41fad.dmp.zip

I am not sure if both crashes are the same.

For the second crash, I wonder if we are missing to call stopFind once the editor closes (we do not even call this when webiew is enabled I think).

@bpasero bpasero added freeze-slow-crash-leak VS Code crashing, performance, freeze and memory leak issues webview Webview issues sandbox Running VSCode in a node-free environment labels Jun 19, 2021
@bpasero bpasero added the electron Issues and items related to Electron label Jun 19, 2021
@rzhao271
Copy link
Contributor

rzhao271 commented Jun 22, 2021

I currently suspect both crashes are the same.
I produced a minimal repro where starting a find session, and then swapping the iframe out, results in a crash due to a null reference (discussed in more detail offline).

@deepak1556
Copy link
Collaborator

Both crashes are same with EXC_BAD_ACCESS that happens when a frame was swapped due to shutdown. Definitely matches the investigation from @rzhao271, will push a fix shortly to the internal builds.

Crash reason:  EXC_BAD_ACCESS / KERN_INVALID_ADDRESS
Crash address: 0x0
Process uptime: 28 seconds

Thread 0 (crashed)
 0  Electron Framework!content::FindRequestManager::FrameObserver::RenderFrameHostChanged(content::RenderFrameHost*, content::RenderFrameHost*) [find_request_manager.cc : 205 + 0x0]
 1  Electron Framework!content::WebContentsImpl::NotifyFrameSwapped(content::RenderFrameHost*, content::RenderFrameHost*, bool) [web_contents_impl.cc : 6314 + 0x14]
 2  Electron Framework!content::WebContentsImpl::NotifyFrameSwapped(content::RenderFrameHost*, content::RenderFrameHost*, bool) [web_contents_impl.cc : 6314 + 0x14]
 3  Electron Framework!content::WebContentsImpl::NotifySwappedFromRenderManager(content::RenderFrameHost*, content::RenderFrameHost*, bool) [web_contents_impl.cc : 7775 + 0x10]
 4  Electron Framework!content::RenderFrameHostManager::InitChild(content::SiteInstance*, int, base::UnguessableToken const&) [render_frame_host_manager.cc : 237 + 0x10]
 5  Electron Framework!content::RenderFrameHostImpl::AddChild(std::__1::unique_ptr<content::FrameTreeNode, std::__1::default_delete<content::FrameTreeNode> >, int, int, base::UnguessableToken const&) [render_frame_host_impl.cc : 2918 + 0xc]
 6  Electron Framework!content::FrameTree::AddFrame(content::RenderFrameHostImpl*, int, int, mojo::PendingReceiver<blink::mojom::BrowserInterfaceBroker>, mojo::PendingAssociatedReceiver<blink::mojom::PolicyContainerHost>, blink::mojom::TreeScopeType, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, bool, base::UnguessableToken const&, base::UnguessableToken const&, blink::FramePolicy const&, blink::mojom::FrameOwnerProperties const&, bool, blink::mojom::FrameOwnerElementType) [frame_tree.cc : 225 + 0x14]
 7  Electron Framework!content::RenderFrameHostImpl::OnCreateChildFrame(int, mojo::PendingReceiver<blink::mojom::BrowserInterfaceBroker>, mojo::PendingAssociatedReceiver<blink::mojom::PolicyContainerHost>, blink::mojom::TreeScopeType, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, bool, base::UnguessableToken const&, base::UnguessableToken const&, blink::FramePolicy const&, blink::mojom::FrameOwnerProperties const&, blink::mojom::FrameOwnerElementType) [render_frame_host_impl.cc : 2647 + 0x3c]
 8  Electron Framework!content::RenderFrameHostImpl::CreateChildFrame(int, mojo::PendingReceiver<blink::mojom::BrowserInterfaceBroker>, mojo::PendingAssociatedReceiver<blink::mojom::PolicyContainerHost>, blink::mojom::TreeScopeType, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&, bool, blink::FramePolicy const&, mojo::StructPtr<blink::mojom::FrameOwnerProperties>, blink::mojom::FrameOwnerElementType) [render_frame_host_impl.cc : 2680 + 0x38]

@deepak1556
Copy link
Collaborator

Upstream fix https://domoreexp.visualstudio.com/Teamspace/_git/electron-build/pullrequest/370827

@rzhao271
Copy link
Contributor

I still get a crash when trying scenario 1:
crash.zip

I haven't tried scenario 2, nor have I symbolicated the file yet.

@deepak1556
Copy link
Collaborator

@rzhao271 which build of electron are you using with OSS ? The crash report indicates you are not using the internal version published recently for 12.0.11

@deepak1556
Copy link
Collaborator

Can confirm you are using older version of 12.0.11, very likely from cache. Can you delete %LOCALAPPDATA%/electron/Cache and try again, thanks!

Debt: add a --force option in gulp-atom-electron to bypass cached downloads.

@rzhao271
Copy link
Contributor

I confirmed it works now. Thanks!

@rzhao271 rzhao271 added the verified Verification succeeded label Jun 23, 2021
@rzhao271 rzhao271 added this to the June 2021 milestone Jun 25, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Aug 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
electron Issues and items related to Electron freeze-slow-crash-leak VS Code crashing, performance, freeze and memory leak issues insiders-released Patch has been released in VS Code Insiders sandbox Running VSCode in a node-free environment verified Verification succeeded webview Webview issues
Projects
None yet
Development

No branches or pull requests

4 participants
@bpasero @deepak1556 @rzhao271 and others