fix: Avoid crashing in NativeViewHost::SetParentAccessible on Windows 10

[poiru]

This fixes #26905. The patch was obtained from @deepak1556, who in turn
got it from the Microsoft Teams folks.

I believe the crash started happening due to the changes in
https://chromium.googlesource.com/chromium/src.git/+/5c6c8e994bce2bfb867279ae5068e9f9134e70c3%5E!/#F15

This affects Electron 9 and later.

Notes: Fix occasional crash on Windows

[nornagon]

We require all patches to describe a plan for their own removal, or else a reason why they should be maintained forever, per https://github.com/electron/electron/blob/master/docs/development/patches.md#patch-justification.

Can this patch be upstreamed? If not, why not? Can we fix this crash without a patch?

[poiru]

@nornagon Updated!

[deepak1556]

This is based on the investigation done by Way Vadhanasin and Julie Koubova from MS teams. The original trace from memory dump was

views::NativeViewHost::SetParentAccessible(IAccessible * accessible) Line 57 C++
ui::AXPlatformNode::NotifyAddAXModeFlags(ui::AXMode mode_flags) Line 97 C++
content::BrowserAccessibilityStateImpl::AddAccessibilityModeFlags(ui::AXMode mode) Line 227 C++
ui::AXPlatformNodeWin::QueryService(const _GUID & guidService, const _GUID & riid, void * * object) Line 4529 C++
oleacc.dll!AccWrap_Base::QueryService() Unknown
nvdaHelperRemote.dll!00007ffd31ff8dff() Unknown
nvdaHelperRemote.dll!00007ffd31fec10e() Unknown
nvdaHelperRemote.dll!00007ffd31fe3afb() Unknown
user32.dll!__ClientCallWinEventProc() Unknown
ntdll.dll!KiUserCallbackDispatcherContinue() Unknown
win32u.dll!NtUserNotifyWinEvent() Unknown
user32.dll!NotifyWinEvent() Unknown
ui::AXSystemCaretWin::MoveCaretTo(const gfx::Rect & bounds_physical_pixels) Line 72 C++
views::HWNDMessageHandler::OnCaretBoundsChanged(const ui::TextInputClient * client) Line 1060 C++
ui::InputMethodBase::NotifyTextInputCaretBoundsChanged(const ui::TextInputClient * client) Line 158 C++
ui::InputMethodWinTSF::OnCaretBoundsChanged(const ui::TextInputClient * client) Line 111 C++
ui::InputMethodWinTSF::OnDidChangeFocusedClient(ui::TextInputClient * focused_before, ui::TextInputClient * focused) Line 168 C++
[Inline Frame] ui::InputMethodBase::SetFocusedTextInputClientInternal(ui::TextInputClient * client) Line 169 C++
ui::InputMethodBase::SetFocusedTextInputClient(ui::TextInputClient * client) Line 53 C++
content::RenderWidgetHostViewAura::OnWindowFocused(aura::Window * gained_focus, aura::Window * lost_focus) Line 1800 C++
wm::FocusController::SetFocusedWindow(aura::Window * window) Line 287 C++
wm::FocusController::FocusAndActivateWindow(wm::ActivationChangeObserver::ActivationReason reason, aura::Window * window) Line 244 C++
views::View::Focus() Line 2001 C++
views::FocusManager::SetFocusedViewWithReason(views::View * view, views::FocusManager::FocusChangeReason reason) Line 378 C++
views::FocusManager::RestoreFocusedView() Line 456 C++
views::DesktopNativeWidgetAura::OnWindowActivated(wm::ActivationChangeObserver::ActivationReason reason, aura::Window * gained_active, aura::Window * lost_active) Line 1182 C++
electron::ElectronDesktopNativeWidgetAura::OnWindowActivated(wm::ActivationChangeObserver::ActivationReason reason, aura::Window * gained_active, aura::Window * lost_active) Line 47 C++
wm::FocusController::SetActiveWindow(wm::ActivationChangeObserver::ActivationReason reason, aura::Window * requested_window, aura::Window * window) Line 366 C++
wm::FocusController::FocusAndActivateWindow(wm::ActivationChangeObserver::ActivationReason reason, aura::Window * window) Line 217 C++
electron::api::BrowserWindow::OnWindowFocus() Line 287 C++

Inspecting the dump had showed that NativeViewHost does not have a valid native_wrapper_ set. The instance of NativeViewHost comes from a holder_ member of WebView. And this causes the Ax notification to dereference a null pointer.

Tracing the construction of the above WebView instance

views::WebView::WebView
electron::InspectableWebContentsViewViews::InspectableWebContentsViewViews
electron::InspectableWebContentsImpl::InspectableWebContentsImpl
electron::CommonWebContentsDelegate::InitWithWebContents
electron::api::WebContents::InitWithSessionAndOptions
electron::api::WebContents::WebContents
electron::api::WebContents::Create
base::internal::FunctorTraits<v8::Local<v8::Promise> (*)(v8::Isolate *, const base::trace_event::TraceConfig &),void>::Invoke
base::internal::InvokeHelper<0,v8::Local<v8::Promise> >::MakeItSo
base::internal::Invoker<base::internal::BindState<v8::Local<v8::Promise> (*)(…
base::internal::Invoker<base::internal::BindState<v8::Local<v8::Promise> (*)(…
base::RepeatingCallback<gin::Handle<electron::api::WebContents> (v8::Isolate *, const gin_helper::Dictionary &)>::Run
gin_helper::Invoker<gin_helper::IndicesHolder<0,1>,v8::Isolate *,const gin_helper::Dictionary &>::DispatchToCallback
gin_helper::Dispatcher<gin::Handle<electron::api::WebContents> (v8::Isolate *, const gin_helper::Dictionary &)>::DispatchToCallback
v8::internal::FunctionCallbackArguments::Call

It comes from

electron/shell/browser/ui/views/inspectable_web_contents_view_views.cc

Lines 86 to 92 in b9c9e7f

	devtools_web_view_(new views::WebView(nullptr)),
	devtools_visible_(false),
	devtools_window_delegate_(nullptr),
	title_(base::ASCIIToUTF16("Developer Tools")) {
	if (!inspectable_web_contents_->IsGuest() &&
	inspectable_web_contents_->GetWebContents()->GetNativeView()) {
	auto* contents_web_view = new views::WebView(nullptr);

The reason why WebView doesn't get a native_wrapper_ is because the parent of these view's which is electron::InspectableWebContentsViewViews is not of type RootView where there is an enforcement of a valid Widget

[deepak1556]

I am not sure of the reason why we consider electron::InspectableWebContentsViewViews as Root, @zcbenz might have more context on this. But I thought #22739 might fix the RootView assumption and it did not.

So for now teams have incorporated this patch that fixes the crash path but definitely doesn't address the root issue, which is with how we maintain the view tree.

[nornagon]

Accepted for now, I hope that we can find a fix for this that does not require a patch.

[release-clerk]

Release Notes Persisted

Fixed an occasional crash on Windows related to NativeViewHost::SetParentAccessible.

[trop]

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

[trop]

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

[trop]

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

[trop]

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