1250109 - crash in mozilla::dom::battery::BatteryManager::UpdateFromBatteryInfo

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-5c20ba20-321a-42b7-bd1d-8a95a2160222.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::dom::battery::BatteryManager::UpdateFromBatteryInfo(mozilla::hal::BatteryInformation const&) 	dom/battery/BatteryManager.cpp
1 	xul.dll 	mozilla::dom::battery::BatteryManager::Notify(mozilla::hal::BatteryInformation const&) 	dom/battery/BatteryManager.cpp
2 	xul.dll 	mozilla::ObserverList<mozilla::hal::BatteryInformation>::Broadcast(mozilla::hal::BatteryInformation const&) 	xpcom/glue/Observer.h
3 	xul.dll 	mozilla::hal::NotifyBatteryChange(mozilla::hal::BatteryInformation const&) 	hal/Hal.cpp
4 	xul.dll 	mozilla::hal_impl::BatteryWindowProc 	hal/windows/WindowsBattery.cpp
5 	user32.dll 	InternalCallWinProc 	
6 	user32.dll 	UserCallWinProcCheckWow 	
7 	user32.dll 	DispatchClientMessage 	
8 	user32.dll 	__fnPOWERBROADCAST 	
9 	ntdll.dll 	KiUserCallbackDispatcher 	
10 	ntdll.dll 	KiUserApcDispatcher 	
11 	user32.dll 	PeekMessageW 	
12 	msctf.dll 	CThreadInputMgr::PeekMessageW(tagMSG*, HWND__*, unsigned int, unsigned int, unsigned int, int*) 	
13 	xul.dll 	nsAppShell::ProcessNextNativeEvent(bool) 	widget/windows/nsAppShell.cpp
14 	xul.dll 	nsBaseAppShell::DoProcessNextNativeEvent(bool) 	widget/nsBaseAppShell.cpp
15 	xul.dll 	nsBaseAppShell::OnProcessNextEvent(nsIThreadInternal*, bool) 	widget/nsBaseAppShell.cpp
16 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp
17 	xul.dll 	NS_ProcessNextEvent(nsIThread*, bool) 	xpcom/glue/nsThreadUtils.cpp
18 	xul.dll 	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp
19 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc
20 	xul.dll 	nsThreadManager::QueryInterface(nsID const&, void**) 	xpcom/threads/nsThreadManager.cpp
21 	xul.dll 	nsBaseAppShell::Run() 	widget/nsBaseAppShell.cpp
22 	xul.dll 	nsAppStartup::Run() 	toolkit/components/startup/nsAppStartup.cpp
23 	xul.dll 	XREMain::XRE_mainRun() 	toolkit/xre/nsAppRunner.cpp
24 	xul.dll 	XREMain::XRE_main(int, char** const, nsXREAppData const*) 	toolkit/xre/nsAppRunner.cpp
25 	xul.dll 	XRE_main 	toolkit/xre/nsAppRunner.cpp
26 		@0x51006040

this seems to be a cross-platform crash regressing since firefox 43 - it caught my attention because its crash volume has doubled in the last days. 
from the timing it looks like bug 1191918 might be the source of the original regression.

many user comments seem to link the crash to playing games on gsn.com

Comment 1

[Michelangelo De Simone [Offline] [:mds]]

Attachment: MozReview Request: Bug 1250109 - Changing DOMEventTargetHelper subclasses to not assume that GetOwner() is non-null, since it can be nulled out by navigation. r?bzbarsky

Review commit: https://reviewboard.mozilla.org/r/37669/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37669/

Comment 2

[Michelangelo De Simone [Offline] [:mds]]

Comment on attachment 8725876 [details]
MozReview Request: Bug 1250109 - Changing DOMEventTargetHelper subclasses to not assume that GetOwner() is non-null, since it can be nulled out by navigation. r?bzbarsky

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37669/diff/1-2/

Comment 3

[Boris Zbarsky [:bzbarsky]]

https://reviewboard.mozilla.org/r/37669/#review34691

::: dom/base/ScreenOrientation.cpp:351
(Diff revision 2)
> +  if (!GetOwner()) {

Should probably throw on aRv here.

::: dom/notification/Notification.cpp:1661
(Diff revision 2)
> +  if (!GetOwner()) {

Seems like this will do the wrong thing in the case when mWorkerPrivate is non-null.

Comment 4

[Michelangelo De Simone [Offline] [:mds]]

Comment on attachment 8725876 [details]
MozReview Request: Bug 1250109 - Changing DOMEventTargetHelper subclasses to not assume that GetOwner() is non-null, since it can be nulled out by navigation. r?bzbarsky

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37669/diff/2-3/

Comment 5

[Doug Turner (:dougt)]

Comment on attachment 8725876 [details]
MozReview Request: Bug 1250109 - Changing DOMEventTargetHelper subclasses to not assume that GetOwner() is non-null, since it can be nulled out by navigation. r?bzbarsky

https://reviewboard.mozilla.org/r/37669/#review34803

Please ask mccr8 for formal review.

::: dom/notification/Notification.cpp:1662
(Diff revision 3)
> -  nsIDocument* doc = mWorkerPrivate ? mWorkerPrivate->GetDocument()
> +      nsIDocument* doc = mWorkerPrivate ? mWorkerPrivate->GetDocument()

Tabs? (reviewboard may be lying.)

::: dom/notification/Notification.cpp:1770
(Diff revision 3)
> -      nsCOMPtr<nsPIDOMWindowInner> window = GetOwner();
> +      if (nsCOMPtr<nsPIDOMWindowInner> window = GetOwner()) {

we usually put the declaration outside the if().

nsCOMPtr<nsPIDOMWindowInner> window = GetOwner();
if (window) {
 ...
}

Comment 6

[Michelangelo De Simone [Offline] [:mds]]

Comment on attachment 8725876 [details]
MozReview Request: Bug 1250109 - Changing DOMEventTargetHelper subclasses to not assume that GetOwner() is non-null, since it can be nulled out by navigation. r?bzbarsky

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37669/diff/3-4/

Comment 7

[Michelangelo De Simone [Offline] [:mds]]

(In reply to Doug Turner (:dougt) from comment #5)

> ::: dom/notification/Notification.cpp:1662
> (Diff revision 3)
> > -  nsIDocument* doc = mWorkerPrivate ? mWorkerPrivate->GetDocument()
> > +      nsIDocument* doc = mWorkerPrivate ? mWorkerPrivate->GetDocument()
> Tabs? (reviewboard may be lying.)

Uh?! I've doublechecked and they're expanded; I'm not sure why they look like tabs on MozReview.

> ::: dom/notification/Notification.cpp:1770
> (Diff revision 3)
> > -      nsCOMPtr<nsPIDOMWindowInner> window = GetOwner();
> > +      if (nsCOMPtr<nsPIDOMWindowInner> window = GetOwner()) {
> 
> we usually put the declaration outside the if().

Done, although I've seen that the "internal declaration" is already present in dom/, see https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerContainer.cpp#85

(In reply to Boris Zbarsky [:bz] from comment #3)
> ::: dom/base/ScreenOrientation.cpp:351
> (Diff revision 2)
> > +  if (!GetOwner()) {
> Should probably throw on aRv here.

Done.

> ::: dom/notification/Notification.cpp:1661
> (Diff revision 2)
> > +  if (!GetOwner()) {
> Seems like this will do the wrong thing in the case when mWorkerPrivate is
> non-null.

I've reversed the condition and moved the relevant login in it.

Comment 8

[Andrew McCreight [:mccr8]]

As I said in IRC, I'm not a DOM peer, so somebody else will have to review it. Maybe bz as it appears he has looked at it already.

I looked over the patch a bit and I noticed you are using four-space indents, while the surrounding code looks like it is two-space indented, so you should fix that.

Comment 9

[Michelangelo De Simone [Offline] [:mds]]

Comment on attachment 8725876 [details]
MozReview Request: Bug 1250109 - Changing DOMEventTargetHelper subclasses to not assume that GetOwner() is non-null, since it can be nulled out by navigation. r?bzbarsky

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37669/diff/4-5/

Comment 10

[Andrew McCreight [:mccr8]]

It looks like that requested review from me and not bz. I'm not very familiar with ReviewBoard so I can't tell you why that is. :)

Comment 11

[Michelangelo De Simone [Offline] [:mds]]

Comment on attachment 8725876 [details]
MozReview Request: Bug 1250109 - Changing DOMEventTargetHelper subclasses to not assume that GetOwner() is non-null, since it can be nulled out by navigation. r?bzbarsky

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37669/diff/5-6/

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8725876 [details]
MozReview Request: Bug 1250109 - Changing DOMEventTargetHelper subclasses to not assume that GetOwner() is non-null, since it can be nulled out by navigation. r?bzbarsky

https://reviewboard.mozilla.org/r/37669/#review35175

This changeset could use a clearer commit message; probably something about changing DOMEventTargetHelper subclasses to not assume that GetOwner() is non-null, since it can be nulled out by navigation.

::: dom/base/ScreenOrientation.cpp:393
(Diff revision 6)
> -  if (!mFullScreenListener) {
> +  if (!mFullScreenListener || !GetOwner()) {

We may want to null out mFullScreenEventListener even if !GetOwner(), right?

::: dom/notification/DesktopNotification.cpp:81
(Diff revision 6)
> -    uint32_t appId = (window.get())->GetDoc()->NodePrincipal()->GetAppId();
> +    uint32_t appId = window ? (window.get())->GetDoc()->NodePrincipal()->GetAppId()

This doesn't need the .get().  I know it was preexisting, but it's still not needed.  ;)

::: dom/notification/Notification.cpp:1661
(Diff revision 6)
> +  if (GetOwner()) {

This still seems wrong to me.  This object has a GetOwner() or an mWorkerPrivate but never both; see the constructor.  So conditioning any code that uses mWorkerPrivate on GetOwner makes no sense.

Oh, and while you're here, can you AssertIsOnMainThread(); at the top of this function?

Comment 13

[Michelangelo De Simone [Offline] [:mds]]

Comment on attachment 8725876 [details]
MozReview Request: Bug 1250109 - Changing DOMEventTargetHelper subclasses to not assume that GetOwner() is non-null, since it can be nulled out by navigation. r?bzbarsky

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37669/diff/6-7/

Comment 14

[Michelangelo De Simone [Offline] [:mds]]

Comment on attachment 8725876 [details]
MozReview Request: Bug 1250109 - Changing DOMEventTargetHelper subclasses to not assume that GetOwner() is non-null, since it can be nulled out by navigation. r?bzbarsky

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37669/diff/7-8/

Comment 15

[Michelangelo De Simone [Offline] [:mds]]

(In reply to Boris Zbarsky [:bz] from comment #12)

> This changeset could use a clearer commit message; probably something about
> changing DOMEventTargetHelper subclasses to not assume that GetOwner() is
> non-null, since it can be nulled out by navigation.

Done.

> ::: dom/base/ScreenOrientation.cpp:393
> > +  if (!mFullScreenListener || !GetOwner()) {
> We may want to null out mFullScreenEventListener even if !GetOwner(), right?

Done.

> ::: dom/notification/DesktopNotification.cpp:81
> > +    uint32_t appId = window ? (window.get())->GetDoc()->NodePrincipal()->GetAppId()
> This doesn't need the .get().  I know it was preexisting, but it's still not
> needed.  ;)

Done.

> ::: dom/notification/Notification.cpp:1661
> > +  if (GetOwner()) {
> This still seems wrong to me.  This object has a GetOwner() or an
> mWorkerPrivate but never both; see the constructor.  So conditioning any
> code that uses mWorkerPrivate on GetOwner makes no sense.

Fixed.

> Oh, and while you're here, can you AssertIsOnMainThread(); at the top of
> this function?

Done.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8725876 [details]
MozReview Request: Bug 1250109 - Changing DOMEventTargetHelper subclasses to not assume that GetOwner() is non-null, since it can be nulled out by navigation. r?bzbarsky

https://reviewboard.mozilla.org/r/37669/#review35269

It really would be good if the commit message talked about _why_ we want to check for null GetOwner...

r=me with that

Comment 17

[Michelangelo De Simone [Offline] [:mds]]

Comment on attachment 8725876 [details]
MozReview Request: Bug 1250109 - Changing DOMEventTargetHelper subclasses to not assume that GetOwner() is non-null, since it can be nulled out by navigation. r?bzbarsky

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37669/diff/8-9/

Comment 18

[Michelangelo De Simone [Offline] [:mds]]

(In reply to Boris Zbarsky [:bz] from comment #16)

> It really would be good if the commit message talked about _why_ we want to
> check for null GetOwner...

Done. This should do it.

Comment 19

[Boris Zbarsky [:bzbarsky]]

No, the point was to say something like what I suggested in comment 12, which would explain why GetOwner can be null in sane situations.

Comment 20

[Michelangelo De Simone [Offline] [:mds]]

Comment on attachment 8725876 [details]
MozReview Request: Bug 1250109 - Changing DOMEventTargetHelper subclasses to not assume that GetOwner() is non-null, since it can be nulled out by navigation. r?bzbarsky

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37669/diff/9-10/

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dce8cc239a84

Comment 22

[Carsten Book [:Tomcat]]

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