AlarmHalService leaks on shutdown due to cycle with AlarmService

RESOLVED FIXED

Status

()

Core
Hardware Abstraction Layer (HAL)
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: nsm, Assigned: nsm)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed-in-birch])

Attachments

(1 attachment, 1 obsolete attachment)

Splitting out from Bug 867868.

This leak interacts with PushService (which uses alarms) to prevent use of push on desktop.
Created attachment 748662 [details] [diff] [review]
Fix AlarmService memory leaks

Carrying forward r=gene from Bug 867868.
Attachment #748662 - Flags: review+
Bug 863732 applied on top of a tree that has Bug 863599 (which caused orange on all debug unit tests).

https://tbpl.mozilla.org/?tree=Try&rev=dbe2240ce433

And with this patch applied on top.

https://tbpl.mozilla.org/?tree=Try&rev=ecd350527fc6
Created attachment 748742 [details] [diff] [review]
AlarmHalService cycle collection.

Justin,

I tried converting AlarmHalService to be cycle collected, but it doesn't seem to be working. AHS still leaks on shutdown.
Could you give it a look, I may be missing something. Not urgent.
Attachment #748742 - Flags: feedback?(justin.lebar+bug)
Landed "Fix AlarmService memory leaks"

https://hg.mozilla.org/projects/birch/rev/b58472c87d72

Marking as leave open for possible cycle collector based solution.
Whiteboard: [fixed-in-birch][leave open]
Comment on attachment 748742 [details] [diff] [review]
AlarmHalService cycle collection.

I'm not sure what the precise incantation is these days, but I /think/ you still have to list all of your nsCOMPtr's in the TRAVERSE block.  The CC needs this in order to know which pointers can be involved in a cycle.

I also don't think you need an empty TRACE block, although I'm not as sure.  smaug or mccr8 can tell you.
Attachment #748742 - Flags: feedback?(justin.lebar+bug)
Comment on attachment 748742 [details] [diff] [review]
AlarmHalService cycle collection.

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

> but I /think/ you still have to list all of your nsCOMPtr's in the TRAVERSE block.
The NS_IMPL_CYCLE_COLLECTION_2 declares both the TRAVERSE and UNLINK methods, so that should be okay.

::: dom/alarm/AlarmHalService.h
@@ +28,5 @@
>                          public SystemTimezoneChangeObserver
>  {
>  public:
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(AlarmHalService)

You can drop the _SCRIPT_HOLDER here, because this class isn't wrapper cached and doesn't hold any other JS pointer.  That in turn will let you remove the empty TRACE thing.
So if it leaks with this patch as written, that's not a result of the CC?
Yeah, not as far as I can see.  Something on the JS side must be root or something?

You could do a shutdown CC dump to figure out what is leaking.  Set XPCOM_CC_LOG_SHUTDOWN=1 MOZ_CC_LOG_DIRECTORY=whateverdirectory then run and shut down.
> Something on the JS side must be root or something?

Yes, it's being rooted from JS, but shouldn't all those roots go away when we shut down?

> The NS_IMPL_CYCLE_COLLECTION_2 declares both the TRAVERSE and UNLINK methods, so that should be okay.

Oh, I see; I missed that.
(In reply to Andrew McCreight [:mccr8] from comment #8)
> Yeah, not as far as I can see.  Something on the JS side must be root or
> something?
> 
> You could do a shutdown CC dump to figure out what is leaking.  Set
> XPCOM_CC_LOG_SHUTDOWN=1 MOZ_CC_LOG_DIRECTORY=whateverdirectory then run and
> shut down.

I tried this and heapgraph/cc/find_roots.py doesn't turn up anything for the AlarmService/AlarmHalService. But the shutdown leak log clearly shows AlarmHalService has leaked, and one end of the cycle is quite likely held by AlarmService since the other patch that I landed fixes it. In fact the only instances of the string AlarmHalService in the shutdown dumps are held by Components.interfaces (nsIAlarmHalService) which is GCed.
Hmm.  You could try XPCOM_CC_ALL_TRACES_AT_SHUTDOWN instead of XPCOM_CC_LOG_SHUTDOWN which may show more.  I'm not really sure what shutdown ends up doing on JS components.
After talking with mccr8 and looking at the shutdown logs, the lack of cycle collection is not the problem. The AlarmHalService is kept alive by JS objects holding on to the AlarmService, and breaking those links in JS is the way to fix that. Cycle collection will help if AlarmHalService is going to be used by multiple people.
Yeah, there's a cycle between JS and C++, but it is held alive JS-side by AlarmService being in the global backstage pass thing.  The cycle collector logs revealed all!
Is it a bug that this JS global backstage pass thing isn't cleared on shutdown?  If we could do that, I suspect a lot of annoying shutdown observers could be nixed.
Marking this as fixed for now. Filed bug 874990 to track the general case.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-birch][leave open] → [fixed-in-birch]
Are we going to uplift this to b2g18?
Gene, not strictly required since this doesn't affect b2g.
Attachment #748742 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.