Closed Bug 1373604 Opened 2 years ago Closed 2 years ago

Crash in nsContentUtils::DispatchFocusChromeEvent

Categories

(Core :: Document Navigation, defect, P1, critical)

55 Branch
All
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 + fixed
firefox56 --- fixed

People

(Reporter: philipp, Assigned: freesamael)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(2 files)

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.
Flagging on Samael for inputs!
Flags: needinfo?(sawang)
This is top6 crash on 55.0b browser type in the past 7 days.
Priority: -- → P1
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
Assignee: nobody → sawang
Flags: needinfo?(sawang)
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?
Attachment #8879837 - Flags: review?(bugs)
Attachment #8879837 - Flags: review?(bugs) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1118323902a
Only try to switch focus to targetDocShell if the load succeeds. r=smaug
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/f1118323902a
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
[Tracking Requested - why for this release]: Top crasher on Beta (currently #6).
tracking as new topcrash
Can you request uplift to beta, please?
Flags: needinfo?(sawang)
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
Flags: needinfo?(sawang)
Attachment #8881445 - Flags: approval-mozilla-beta?
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.
Attachment #8881445 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.