924681 - crash in nsPresContext::GetPrimaryFrameFor(nsIContent*)

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

This bug was filed from the Socorro interface and is 
report bp-3b4505d2-dd02-4227-bced-ff0872131002.
=============================================================
Crashing Thread
Frame 	Module 	Signature 	Source
0 	libxul.so 	nsPresContext::GetPrimaryFrameFor(nsIContent*) 	layout/base/nsPresContext.h
1 	libxul.so 	nsEventStateManager::FireContextClick() 	content/events/src/nsEventStateManager.cpp
2 	libxul.so 	nsEventStateManager::sClickHoldCallback(nsITimer*, void*) 	content/events/src/nsEventStateManager.cpp
3 	libxul.so 	nsTimerImpl::Fire() 	xpcom/threads/nsTimerImpl.cpp
4 	libxul.so 	nsTimerEvent::Run() 	xpcom/threads/nsTimerImpl.cpp
5 	libxul.so 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp
6 	libxul.so 	NS_ProcessNextEvent(nsIThread*, bool) 	xpcom/glue/nsThreadUtils.cpp
7 	libxul.so 	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp
8 	libxul.so 	mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp
9 	libxul.so 	MessageLoop::RunInternal() 	ipc/chromium/src/base/message_loop.cc
10 	libxul.so 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc
11 	libxul.so 	nsBaseAppShell::Run() 	widget/xpwidgets/nsBaseAppShell.cpp
12 	libxul.so 	XRE_RunAppShell 	toolkit/xre/nsEmbedFunctions.cpp
13 	libxul.so 	mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp
14 	libxul.so 	MessageLoop::RunInternal() 	ipc/chromium/src/base/message_loop.cc
15 	libxul.so 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc
16 	libxul.so 	XRE_InitChildProcess 	toolkit/xre/nsEmbedFunctions.cpp
17 	plugin-container 	main 	ipc/app/MozillaRuntimeMain.cpp
18 	libc.so 	__libc_init 	bionic/libc/bionic/libc_init_dynamic.c
19 		@0xb0001dc5

Looks like one could potentially play around with the pulse.me and crash

Comment 1

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

More crashes: https://crash-stats.mozilla.com/report/list?product=B2G&signature=nsPresContext%3A%3AGetPrimaryFrameFor%28nsIContent*%29

Comment 2

[Andrew Halberstadt [:ahal]]

I think I'm hitting this in b2g desktop mochitests. This is intermittent, and doesn't seem to be related to a specific test (if I disable one test, others just crash in its place).

Log file with crash stack:
https://tbpl.mozilla.org/php/getParsedLog.php?id=29893368&tree=Cedar&full=1

Though there are many other jobs with different tests that hit the crash: https://tbpl.mozilla.org/?tree=Cedar&showall=1&jobname=b2g (see the Bg M(1)'s)

Andrew, do you think you could get someone to look into this? I don't think I'll be able to get around this by disabling tests, so this blocks b2g desktop mochitests.

Comment 3

[Andrew Halberstadt [:ahal]]

Gregor, fyi this is blocking me from rolling b2g desktop mochitests out on tbpl.

Comment 4

[Andrew Overholt [:overholt]]

Olli, this seems like your area of expertise.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Attachment: null check

Looks like a null pointer crash (offset from null).
FireContextClick() is, IIRC, used currently only in b2g.

We could also just cancel the timer in few more places, but I think this patch is just fine.

Comment 6

[Andrew Halberstadt [:ahal]]

(In reply to Olli Pettay [:smaug] from comment #5)
> Created attachment 825468 [details] [diff] [review]
> null check
> 
> Looks like a null pointer crash (offset from null).
> FireContextClick() is, IIRC, used currently only in b2g.
> 
> We could also just cancel the timer in few more places, but I think this
> patch is just fine.

So this looks like it would fix the crash, which is great, but I think we'll still have the root problem that the presentation is intermittently null on b2g desktop for some reason (similar to bug 927586). I anticipate new failures after this.

I guess we'll see how this goes and I'll file a new bug if this doesn't fix the root problem. Thanks for the quick patch though!

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

The patch should be valid. We don't cancel the possible timeout when mPresContext becomes null, and
that is the root problem.

b2g-desktop may use iframes in some unusual way and expect that there is presentation always.
But that doesn't sound like this bug.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 825468 [details] [diff] [review]
null check

If you don't mind, please add {} before landing.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

I knew you were going to ask that :)

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: with {}

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/357348508e06

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/357348508e06

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/128b434db0a2