871428 - AlarmHalService leaks on shutdown due to cycle with AlarmService

Status: RESOLVED FIXED

(Core :: Hardware Abstraction Layer (HAL), defect)

Description

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Splitting out from Bug 867868.

This leak interacts with PushService (which uses alarms) to prevent use of push on desktop.

Comment 1

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Fix AlarmService memory leaks

Carrying forward r=gene from Bug 867868.

Comment 2

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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

Comment 3

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: 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.

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Landed "Fix AlarmService memory leaks"

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

Marking as leave open for possible cycle collector based solution.

Comment 5

[Justin Lebar (not reading bugmail)]

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.

Comment 6

[Andrew McCreight [:mccr8]]

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.

Comment 7

[Justin Lebar (not reading bugmail)]

So if it leaks with this patch as written, that's not a result of the CC?

Comment 8

[Andrew McCreight [:mccr8]]

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.

Comment 9

[Justin Lebar (not reading bugmail)]

> 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.

Comment 10

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(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.

Comment 11

[Andrew McCreight [:mccr8]]

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.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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.

Comment 14

[Andrew McCreight [:mccr8]]

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!

Comment 15

[Justin Lebar (not reading bugmail)]

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.

Comment 16

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Marking this as fixed for now. Filed bug 874990 to track the general case.

Comment 17

[Gene Lian (I already quit Mozilla)]

Are we going to uplift this to b2g18?

Comment 18

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Gene, not strictly required since this doesn't affect b2g.