1373604 - Crash in nsContentUtils::DispatchFocusChromeEvent

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P1)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-e5bae163-2159-4b05-8300-60d2e0170616.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	nsContentUtils::DispatchFocusChromeEvent(nsPIDOMWindowOuter*) 	dom/base/nsContentUtils.cpp:4415
1 	xul.dll 	nsDocShell::InternalLoad(nsIURI*, nsIURI*, bool, nsIURI*, unsigned int, nsIPrincipal*, nsIPrincipal*, unsigned int, nsAString const&, char const*, nsAString const&, nsIInputStream*, nsIInputStream*, unsigned int, nsISHEntry*, bool, nsAString const&, nsIDocShell*, nsIURI*, bool, nsIDocShell**, nsIRequest**) 	docshell/base/nsDocShell.cpp:10166
2 	xul.dll 	nsDocShell::OnLinkClickSync(nsIContent*, nsIURI*, char16_t const*, nsAString const&, nsIInputStream*, nsIInputStream*, bool, nsIDocShell**, nsIRequest**, nsIPrincipal*) 	docshell/base/nsDocShell.cpp:14212
3 	xul.dll 	OnLinkClickEvent::Run() 	docshell/base/nsDocShell.cpp:13966
4 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:1406
5 	xul.dll 	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:96
6 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc:231
7 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc:211
8 	xul.dll 	nsBaseAppShell::Run() 	widget/nsBaseAppShell.cpp:156
9 	xul.dll 	nsAppShell::Run() 	widget/windows/nsAppShell.cpp:271
10 	xul.dll 	nsAppStartup::Run() 	toolkit/components/startup/nsAppStartup.cpp:283
11 	xul.dll 	XREMain::XRE_mainRun() 	toolkit/xre/nsAppRunner.cpp:4569
12 	xul.dll 	XREMain::XRE_main(int, char** const, mozilla::BootstrapConfig const&) 	toolkit/xre/nsAppRunner.cpp:4749
13 	xul.dll 	XRE_main(int, char** const, mozilla::BootstrapConfig const&) 	toolkit/xre/nsAppRunner.cpp:4844
14 	xul.dll 	mozilla::BootstrapImpl::XRE_main(int, char** const, mozilla::BootstrapConfig const&) 	toolkit/xre/Bootstrap.cpp:45
15 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:115
16 	firefox.exe 	__scrt_common_main_seh 	f:/dd/vctools/crt/vcstartup/src/startup/exe_common.inl:253
17 	kernel32.dll 	BaseThreadInitThunk 	
18 	ntdll.dll 	__RtlUserThreadStart 	
19 	ntdll.dll 	_RtlUserThreadStart

this is a crash signature that's apparently regressing in 55.0b judging by early crash data from the staged rollout. it's happening in a codepath last touched by bug 1303838, so i'm tentatively setting it as blocking that bug.

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Flagging on Samael for inputs!

Comment 2

[Hsin-Yi Tsai (she/her) [:hsinyi]]

This is top6 crash on 55.0b browser type in the past 7 days.

Comment 3

[Samael Wang [:freesamael] (away for now)]

The reports all look like nullptr+offset case, so likely aWindow is nullptr at [1]. Maybe targetDocShell is being destroyed at [2] so the window becomes null? Need to take a look.

[1] https://hg.mozilla.org/releases/mozilla-beta/annotate/6872377277a6/dom/base/nsContentUtils.cpp#l4415
[2] https://hg.mozilla.org/releases/mozilla-beta/annotate/6872377277a6/docshell/base/nsDocShell.cpp#l10166

Comment 4

[Samael Wang [:freesamael] (away for now)]

Attachment: Only try to switch focus to targetDocShell if the load succeeds

MozReview-Commit-ID: H1TsRcbagg4

Comment 5

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8879837 [details] [diff] [review]
Only try to switch focus to targetDocShell if the load succeeds

Hi Olli, 

I think the reason is that I didn't check if targetDocShell->InternalLoad() succeeds, so that if somehow the load fails, targetDocShell->GetWindow() may become nullptr.

Would you take a look?

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1118323902a
Only try to switch focus to targetDocShell if the load succeeds. r=smaug

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f1118323902a

Comment 8

[Marco Castelluccio [:marco]]

[Tracking Requested - why for this release]: Top crasher on Beta (currently #6).

Comment 9

[Julien Cristau [:jcristau]]

tracking as new topcrash

Comment 10

[Liz Henry (:lizzard)]

Can you request uplift to beta, please?

Comment 11

[Samael Wang [:freesamael] (away for now)]

Attachment: Only try to switch focus to targetDocShell if the load succeeds

rebase for beta

Comment 12

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8881445 [details] [diff] [review]
Only try to switch focus to targetDocShell if the load succeeds

Approval Request Comment
[Feature/Bug causing the regression]: bug 1303838
[User impact if declined]: application crash
[Is this code covered by automated tests?]: not for the crash case
[Has the fix been verified in Nightly?]: yes, for few days
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: it essentially only adds a simple condition test
[String changes made/needed]: none

Comment 13

[Liz Henry (:lizzard)]

Comment on attachment 8881445 [details] [diff] [review]
Only try to switch focus to targetDocShell if the load succeeds

Fix for topcrash in beta, let's take this for beta 6.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/67c173e36b9e