Closed Bug 1001075 Opened 6 years ago Closed 5 years ago

crash in mozilla::hal_impl::DisableBatteryNotifications()

Categories

(Core :: Widget: Cocoa, defect, critical)

All
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla32
Tracking Status
e10s + ---
firefox32 --- verified

People

(Reporter: blassey, Assigned: mconley)

References

Details

(Keywords: crash, reproducible)

Crash Data

Attachments

(2 files, 1 obsolete file)

This bug was filed from the Socorro interface and is 
report bp-5d0d4648-9c5c-45b7-a36f-f81a22140423.
=============================================================

This has been the signature of my last 5 crashes. Note that I'm running with e10s turned on
From the stack trace it looks like these crashes happen on shutdown.  Do they?
Assignee: nobody → mconley
Hey blassey, do you have STR for this crash? As smichaud pointed out, the traces seem to suggest that you're shutting down when the crashes occur... are there things you need to do before shutting down for the crash to rear its head?
Flags: needinfo?(blassey.bugs)
Unfortunitely, I don't have better STR. My last crash was defintely during shutdown though.
Flags: needinfo?(blassey.bugs)
I have reproduced this. I have some STR, trying to find a minimal set...
STR:

1) In about:config, set browser.tabs.remote.autostart to true, and restart Firefox.
2) In the a tab, go to a website running Flash, like YouTube, or Homestarrunner.com.
3) In a second tab, visit any site that makes use of the DOM Battery API, like http://robnyman.github.io/battery/.
4) Quit the browser

This gives me a reliable crash, which traces that match blassey's.

Example:

https://crash-stats.mozilla.com/report/index/6311c9f9-0695-4409-bab8-4e4bd2140507
Mike, your STR works for me even on a machine that has no battery (a MacPro)!

The Flash site I visited was http://mirrors.creativecommons.org/getcreative/.

bp-0c57c812-c9d4-4121-bdb6-5dd5e2140507
Keywords: reproducible
Mike, could you attach your gdb stack trace?
Nevermind.  Here's one.
I'd guess that mRunLoopSource has become invalid -- that it's been released too many times.
Also note the crash address (in both Mike's and my crash stacks):  0x7fff9778e921

In light of bug 1002564, it's interesting that this is a real 64-bit value (neither truncated to 32-bit nor truncated and sign extended).  Both builds contain my bandaid patch for that bug (as does the 2014-05-02 m-c nightly and later ones), and plugins are definitely in play here.  So my patch seems to have done some good, though it doesn't fix all cases of truncation of the crash address to 32-bits.
It looks like with these STR, we're hitting MacPowerInformationService::~MacPowerInformationService() before StopListening is called. Hm....
So here's what's going on - with the STR, we're successfully hearing the xpcom-shutdown notification, and tearing down the MacPowerInformationService by calling MacPowerInformationService::StopListening and then MacPowerInformationService::~MacPowerInformationService.

THEN, after that, we construct the service again at MacPowerInformationService::MacPowerInformationService, and then call StopListening again, and that's where we crash.

Here's the backtrace for this second call to MacPowerInformationService::MacPowerInformationService:

#0  mozilla::hal_impl::MacPowerInformationService::MacPowerInformationService () at /Users/mikeconley/Projects/mozilla-central/hal/cocoa/CocoaBattery.cpp:155
#1  operator new () at /Users/mikeconley/Projects/mozilla-central/obj-x86_64-apple-darwin12.5.0/dist/include/mozilla/mozalloc.h:165
#2  0x00000001017d448d in mozilla::hal_impl::MacPowerInformationService::GetInstance () at CocoaBattery.cpp:130
#3  0x00000001017d456b in mozilla::hal_impl::DisableBatteryNotifications () at CocoaBattery.cpp:86
#4  0x00000001017d26f9 in mozilla::hal::ObserversManager<mozilla::hal::BatteryInformation>::RemoveObserver (this=0x10513dd18, aObserver=<value temporarily unavailable, due to optimizations>) at /Users/mikeconley/Projects/mozilla-central/hal/Hal.cpp:209
#5  0x00000001017d8383 in mozilla::hal_sandbox::HalParent::ActorDestroy (this=0x11e3c7c50, aWhy=209584128) at SandboxHal.cpp:462
#6  0x00000001016b0b6f in mozilla::dom::PContentParent::DestroySubtree (this=0x118243000, why=mozilla::ipc::IProtocolManager<mozilla::ipc::IProtocol>::NormalShutdown) at PContentParent.cpp:4919
#7  0x00000001016b0073 in mozilla::dom::PContentParent::OnChannelClose (this=0x118243000) at PContentParent.cpp:4728
#8  0x0000000101628ac7 in mozilla::ipc::MessageChannel::NotifyChannelClosed () at /Users/mikeconley/Projects/mozilla-central/obj-x86_64-apple-darwin12.5.0/dist/include/mozilla/ipc/MessageChannel.h:1695
#9  0x0000000101628ac7 in mozilla::ipc::MessageChannel::Close (this=0x118243060) at MessageChannel.cpp:130
#10 0x0000000102046152 in mozilla::dom::ContentParent::ShutDownProcess (this=0x118243000, aCloseWithError=<value temporarily unavailable, due to optimizations>) at ContentParent.cpp:1058
#11 0x0000000102048779 in mozilla::dom::ContentParent::Observe (this=0x118243000, aSubject=0x1005f81c8, aTopic=0x1044e2070 "xpcom-shutdown", aData=0x0) at ContentParent.cpp:2058
#12 0x00000001013bb6e4 in nsTArray_Impl<nsISupports*, nsTArrayInfallibleAllocator>::ElementAt () at /Users/mikeconley/Projects/mozilla-central/obj-x86_64-apple-darwin12.5.0/dist/include/nsTArray.h:96
#13 0x00000001013bb6e4 in nsTArray_Impl<nsISupports*, nsTArrayInfallibleAllocator>::operator[] () at nsObserverList.cpp:921
#14 0x00000001013bb6e4 in nsObserverList::NotifyObservers () at /Users/mikeconley/Projects/mozilla-central/xpcom/ds/nsObserverList.h:120
#15 0x00000001013bb6e4 in nsObserverService::NotifyObservers (this=<value temporarily unavailable, due to optimizations>, aSubject=0x1005f81c8, aTopic=0x1044e2070 "xpcom-shutdown", someData=0x0) at nsObserverList.cpp:130
#16 0x00000001013922fd in nsCOMPtr<nsIServiceManager>::~nsCOMPtr () at nsXPComInit.cpp:787
#17 0x00000001013922fd in nsCOMPtr<nsIServiceManager>::~nsCOMPtr () at /Users/mikeconley/Projects/mozilla-central/xpcom/glue/nsCOMPtr.h:790
#18 0x00000001013922fd in mozilla::ShutdownXPCOM (servMgr=0x1005f81c8) at nsXPComInit.cpp:130
#19 0x0000000102d40124 in ScopedXPCOMStartup::~ScopedXPCOMStartup (this=0x10050c4d0) at /Users/mikeconley/Projects/mozilla-central/toolkit/xre/nsAppRunner.cpp:1202
#20 0x0000000102d4538c in ScopedXPCOMStartup::~ScopedXPCOMStartup () at /Users/mikeconley/Projects/mozilla-central/toolkit/xre/nsAppRunner.cpp:1183
#21 0x0000000102d4538c in XREMain::XRE_main (this=0x7fff5fbff2c0, argc=<value temporarily unavailable, due to optimizations>, argv=<value temporarily unavailable, due to optimizations>, aAppData=<value temporarily unavailable, due to optimizations>) at mozalloc.h:130
#22 0x0000000102d45677 in XRE_main (argc=6259144, argv=0xbb800000, aAppData=0x15, aFlags=<value temporarily unavailable, due to optimizations>) at /Users/mikeconley/Projects/mozilla-central/toolkit/xre/nsAppRunner.cpp:4304
#23 0x000000010000160f in main (argc=<value temporarily unavailable, due to optimizations>, argv=<value temporarily unavailable, due to optimizations>) at /Users/mikeconley/Projects/mozilla-central/browser/app/nsBrowserApp.cpp:282

It looks like this is because the ContentParent is trying to disable the battery notifications _after_ xpcom-shutdown.
Comment on attachment 8421217 [details] [diff] [review]
Account for possible re-entry in MacPowerInformationService::StopListening to avoid crash. r=?

It looks like we can have re-entry to MacPowerInformationService::StopListening when we have ContentParents.

This patch ensures that StopListening is called if the MacPowerInformationService is destructed, and bails out early from StopListening if mRunLoopSource is nullptr.

Not sure that's the best way forward - it'd be nice to avoid the re-entry entirely, but not sure how possible this is.

What do you think BenWa?
Attachment #8421217 - Flags: feedback?(bgirard)
Comment on attachment 8421217 [details] [diff] [review]
Account for possible re-entry in MacPowerInformationService::StopListening to avoid crash. r=?

Review of attachment 8421217 [details] [diff] [review]:
-----------------------------------------------------------------

::: hal/cocoa/CocoaBattery.cpp
@@ +197,5 @@
>  MacPowerInformationService::StopListening()
>  {
>    MOZ_ASSERT(mRunLoopSource, "IOPS Notification Loop Source not set up. "
>                               "(StopListening without BeginListening)");
> +  if (!mRunLoopSource) {

This breaks the assertion above. Assertion should never be triggered.
(In reply to Benoit Girard (:BenWa) from comment #16)
> Comment on attachment 8421217 [details] [diff] [review]
> Account for possible re-entry in MacPowerInformationService::StopListening
> to avoid crash. r=?
> 
> Review of attachment 8421217 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: hal/cocoa/CocoaBattery.cpp
> @@ +197,5 @@
> >  MacPowerInformationService::StopListening()
> >  {
> >    MOZ_ASSERT(mRunLoopSource, "IOPS Notification Loop Source not set up. "
> >                               "(StopListening without BeginListening)");
> > +  if (!mRunLoopSource) {
> 
> This breaks the assertion above. Assertion should never be triggered.

I see - so are you suggesting we find some way of eliminating the re-entry entirely in order to unbreak this assertion? Or are you suggesting I remove the assertion?
Flags: needinfo?(bgirard)
I don't know the code to say either. Also have you tried contacting Reuben about this problem?
Flags: needinfo?(bgirard)
That assertion should actually be triggered by this bug, I assume the only reason it wasn't is that no one reproduced this in a debug build…

Better than changing StopListening would be to avoid creating the new service and adding a new xpcom-shutdown observer in this scenario. Setting a flag in the xpcom-shutdown observer and checking it in {Enable,Disable}BatteryNotifications should be enough.
Attachment #8421217 - Attachment is obsolete: true
Attachment #8421217 - Flags: feedback?(bgirard)
Comment on attachment 8421429 [details] [diff] [review]
Account for possible re-entry in MacPowerInformationService::StopListening to avoid crash. r=?

Something like this?
Attachment #8421429 - Flags: feedback?(reuben.bmo)
Comment on attachment 8421429 [details] [diff] [review]
Account for possible re-entry in MacPowerInformationService::StopListening to avoid crash. r=?

Review of attachment 8421429 [details] [diff] [review]:
-----------------------------------------------------------------

Yep.
Attachment #8421429 - Flags: feedback?(reuben.bmo) → feedback+
Comment on attachment 8421429 [details] [diff] [review]
Account for possible re-entry in MacPowerInformationService::StopListening to avoid crash. r=?

Awesome, thanks reuben!
Attachment #8421429 - Flags: review?(bgirard)
Attachment #8421429 - Flags: review?(bgirard) → review+
https://hg.mozilla.org/mozilla-central/rev/0f38f6b70209
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
I was able to reproduce this issue with a Nightly build from March 28, on a Mac OS X 10.9.4, with steps from comment 5. The crash no longer occurs in Firefox 32 Beta 6 (BuildID: 20140811180644).

Looking at [1] there are also no crashes in the past 4 weeks for versions 32, 33 or 34.

[1] https://crash-stats.mozilla.com/report/list?product=Firefox&range_unit=days&range_value=28&signature=CoreFoundation%400x11c9b#tab-reports
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.