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

fix: crash when clicking links with target=_blank from webview #29874

Merged
merged 1 commit into from Jun 30, 2021

Conversation

deepak1556
Copy link
Member

Description of Change

Fixes #28348

When running the test case in debug build, it will be seen that RWHV::SetActive call fails because it tries to access an interface blink::FrameWidget that is not bound.

The interface usually gets bound when RenderView gets created during navigation

RenderWidgetHostImpl::BindAndGenerateCreateFrameWidgetParams
RenderViewHostImpl::CreateRenderView
WebContentsImpl::CreateRenderViewForRenderManager
RenderFrameHostManager::InitRenderView

When starting with window.opener suppressed, WebViewGuestDelegate::CreateNewGuestWindow created NSView for page which is going to be navigated immediately. Since the view was setup, it was receiving the events from the UI loop that led to the above crash.

This change removes the eager widget creation call for this case and lets chromium handle it as expected.

Checklist

Release Notes

Notes: fix crash when clicking links with target=_blank from webview

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jun 24, 2021
@deepak1556 deepak1556 added target/12-x-y semver/patch backwards-compatible bug fixes labels Jun 24, 2021
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jun 25, 2021
new_guest_view->CreateViewForWidget(
guest_contents_impl->GetRenderViewHost()->GetWidget());
if (!create_params.initially_hidden)
widget_view->Show();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this call to Show is new, is it needed? Is this code inspired by some analogous code in Chromium?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is analogous to the non-guest code path from content::WebContentsImpl::CreateNewWindow, this one specifically to https://source.chromium.org/chromium/chromium/src/+/main:content/browser/web_contents/web_contents_impl.cc;l=3794-3798

@deepak1556 deepak1556 merged commit 522b19e into main Jun 30, 2021
@deepak1556 deepak1556 deleted the robo/fix_native_window_webview branch June 30, 2021 01:10
@release-clerk
Copy link

release-clerk bot commented Jun 30, 2021

Release Notes Persisted

fix crash when clicking links with target=_blank from webview

@trop
Copy link
Contributor

trop bot commented Jun 30, 2021

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

@trop trop bot removed the target/14-x-y label Jun 30, 2021
@trop
Copy link
Contributor

trop bot commented Jun 30, 2021

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

@trop
Copy link
Contributor

trop bot commented Jun 30, 2021

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

@linonetwo
Copy link

Click on link inside BrowserView still crush #29263 (comment)

Do you think is related?

@linonetwo
Copy link

I confirm this is fixed in v14beta, but not v13 latest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: App Crashes when a new foreground tab is opened from a webview with nativeWindowOpen and allowPopups
4 participants