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

[Bug]: removeBrowserView() crashes if view.webContents.close() is called beforehand. #39377

Closed
3 tasks done
Bill77 opened this issue Aug 4, 2023 · 1 comment · Fixed by #39387
Closed
3 tasks done
Assignees
Labels
25-x-y 26-x-y bug 🪲 has-repro-gist Issue can be reproduced with code at https://gist.github.com/ status/confirmed A maintainer reproduced the bug or agreed with the feature

Comments

@Bill77
Copy link

Bill77 commented Aug 4, 2023

Preflight Checklist

Electron Version

25.3.0

What operating system are you using?

macOS

Operating System Version

macOs Ventura 13.4.1 (c)

What arch are you using?

x64

Last Known Working Electron version

25.2.0

Expected Behavior

We are using browserViews in a browserWindow, when closing our view, we need the window.close event to be sent, so I'm using the view.webContents.close() to close the window (it's hosting a citrix receiver html5 page) before I call mainWindow.removeBrowserView().

Expected behavior is the browserView closes, and the getBrowserViews list is updated by removeBrowserView() call.

Actual Behavior

The removeBrowserView call crashes the electron app. In the fiddle it crashes with Electron exited with signal SIGSEGV.

Testcase Gist URL

https://gist.github.com/Bill77/1db0aefcd338b612f66e09a9f7372d82

Additional Information

Attached is a Electron Fiddle with the add and close buttons. To reproduce, add the view and then close it, which works in 25.2.0 but does not in 25.3.0+. Looking at the change log, it seem it's related to this PR.

I did try to just use the webContents.close() and skip calling removeBrowserView() but the view list does not get updated, and we get a list of views that have null webContents. Which I suspect is causing the issue in removeBrowerView() to crash.

We either need a solution where within removeBrowserView it calls the webContents.close or the logic in removeBrowserView needs null checks on the webContent references. Sorry, that's my guess based on not looking at the Electron code directly! 🤷‍♂️

@Bill77 Bill77 added the bug 🪲 label Aug 4, 2023
@codebytere codebytere added status/confirmed A maintainer reproduced the bug or agreed with the feature has-repro-gist Issue can be reproduced with code at https://gist.github.com/ 25-x-y 26-x-y labels Aug 6, 2023
@codebytere codebytere self-assigned this Aug 6, 2023
codebytere added a commit that referenced this issue Aug 7, 2023
@rjvelasquezm

This comment was marked as off-topic.

codebytere added a commit that referenced this issue Aug 8, 2023
fix: removeBrowserView draggable region removal

Closes #39377.
trop bot added a commit that referenced this issue Aug 8, 2023
Closes #39377.

Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
trop bot added a commit that referenced this issue Aug 8, 2023
Closes #39377.

Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
trop bot added a commit that referenced this issue Aug 8, 2023
Closes #39377.

Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
codebytere added a commit that referenced this issue Aug 8, 2023
fix: removeBrowserView draggable region removal

Closes #39377.

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
codebytere added a commit that referenced this issue Aug 8, 2023
fix: removeBrowserView draggable region removal

Closes #39377.

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
jkleinsc pushed a commit that referenced this issue Aug 9, 2023
fix: removeBrowserView draggable region removal

Closes #39377.

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
win32ss pushed a commit to win32ss/supermium-electron that referenced this issue Sep 24, 2023
fix: removeBrowserView draggable region removal

Closes electron#39377.
MrHuangJser pushed a commit to MrHuangJser/electron that referenced this issue Dec 11, 2023
fix: removeBrowserView draggable region removal

Closes electron#39377.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
25-x-y 26-x-y bug 🪲 has-repro-gist Issue can be reproduced with code at https://gist.github.com/ status/confirmed A maintainer reproduced the bug or agreed with the feature
Projects
No open projects
Status: Unsorted Items
Status: 🛠 Fixed for Next Release
Development

Successfully merging a pull request may close this issue.

4 participants
@codebytere @Bill77 @rjvelasquezm and others