Closed Bug 1250109 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: DOM: Device Interfaces, defect, critical)

45 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox44 --- affected
firefox45 --- affected
firefox46 --- ?
firefox47 --- ?
firefox48 --- fixed

People

(Reporter: philipp, Assigned: mds)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

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
Assignee: nobody → michelangelo
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/
Attachment #8725876 - Attachment description: MozReview Request: Bug 1250109 - NULL-check in BatteryManager::UpdateFromBatteryInfo. r?dougt → MozReview Request: Bug 1250109 - Guarding few GetOwner() calls.
Attachment #8725876 - Flags: review?(dougt) → feedback?
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.
OS: Windows NT → All
Hardware: x86 → All
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/
Attachment #8725876 - Attachment description: MozReview Request: Bug 1250109 - Guarding few GetOwner() calls. → MozReview Request: Bug 1250109 - Guarding few GetOwner() calls. r?
Attachment #8725876 - Flags: feedback? → review?(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) {
 ...
}
Attachment #8725876 - Flags: review?(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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37669/diff/3-4/
Attachment #8725876 - Attachment description: MozReview Request: Bug 1250109 - Guarding few GetOwner() calls. r? → MozReview Request: Bug 1250109 - Guarding few GetOwner() calls. r?mccr8
Attachment #8725876 - Flags: review?(continuation)
(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.
Component: General → DOM: Device Interfaces
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.
Attachment #8725876 - Flags: review?(continuation) → review?(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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37669/diff/4-5/
Attachment #8725876 - Attachment description: MozReview Request: Bug 1250109 - Guarding few GetOwner() calls. r?mccr8 → MozReview Request: Bug 1250109 - Guarding few GetOwner() calls. r?bzbarsky
Attachment #8725876 - Flags: review?(bzbarsky) → review?(continuation)
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 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/
Attachment #8725876 - Flags: review?(continuation) → review?(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?
Attachment #8725876 - Flags: review?(bzbarsky)
Attachment #8725876 - Attachment description: MozReview Request: Bug 1250109 - Guarding few GetOwner() calls. r?bzbarsky → MozReview Request: Bug 1250109 - Checking for non-NULL GetOwner() calls in DOMEventTargetHelper subclasses. r?bzbarsky
Attachment #8725876 - Flags: review?(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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/37669/diff/6-7/
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/
(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 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
Attachment #8725876 - Flags: review?(bzbarsky) → review+
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/
(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.
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 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/
Attachment #8725876 - Attachment description: MozReview Request: Bug 1250109 - Checking for non-NULL GetOwner() calls in DOMEventTargetHelper subclasses. r?bzbarsky → 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://hg.mozilla.org/mozilla-central/rev/dce8cc239a84
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.