409424 - Growl integration leaks

Status: RESOLVED FIXED

(Firefox :: Shell Integration, defect, P2)

Description

[Peter Van der Beken [:peterv]]

GrowlApplicationBridge never releases the delegate (mozGrowlDelegate) and mozGrowlDelegate never releases the observers that it holds in mDict. In the case of bug 388573 one of the observers ends up holding windows alive, leading to a large shutdown leak.

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: Partial patch

This fixes part of the leaks, but I am worried that there are more objects being leaked from this code. For example, I don't see where appName and appIconData in GrowlApplicationBridge.m are released?

Comment 2

[Shawn Wilsher :sdwilsh]

I wonder if upgrading to the latest growl files would help (1.1, compared to 0.7.6 which we have now).  We should also probably push any leak fixes upstream.

Comment 3

[Josh Aas]

We should probably upgrade to the latest growl code now, before we get even closer to final 1.9. I assume you would do that, Shawn? Can you file a bug on upgrading to 1.1?

Comment 4

[Boris Zbarsky [:bzbarsky]]

How long-lived are these observers that keep windows alive?  That sounds pretty bad if the observers stay alive for the app lifetime....

Comment 5

[Peter Van der Beken [:peterv]]

App lifetime, though in this specific case they are only holding the hidden window alive. It could be possible to have an extension that has observers that hold real windows alive, so yes, we do need to fix this.

Comment 6

[Shawn Wilsher :sdwilsh]

Well, unless they have disabled the notification (they being the user), we'll free it with the two callbacks.

Comment 7

[Shawn Wilsher :sdwilsh]

I just spent a little more than two hours with Growl trunk and our build system and still don't have it building.  I'm getting worried at how much bigger this binary is going to be...

What was our reason for not just using the Growl framework?  That would be *a lot* simpler to work with than trying to pick and choose all the right files that we need to build.

Comment 8

[Peter Van der Beken [:peterv]]

I doubt this is fixed in Growl trunk, last time I looked I didn't see any code to release the delegate static variable in GrowlApplicationBridge. Can we just try to fix this bug instead, moving the upgrade to a different bug?

Comment 9

[Shawn Wilsher :sdwilsh]

Yeah - I spent a couple hours on the upgrade and got *nowhere*.  If we used the framework, this would be pretty trivial, but we don't, so it's crazily hard :(

Comment 10

[Colin Barrett [:cbarrett]]

I brought this up with one of the Growl folks. Again, us not using the framework makes it nearly impossible for them to help us.

Remind me why we did that?

Comment 11

[Shawn Wilsher :sdwilsh]

It was decided that we didn't want to ship a 3rd party framework in toolkit by bsmedberg.

I think josh was against it as well.

Comment 12

[Mike Schroepfer]

(In reply to comment #11)
> It was decided that we didn't want to ship a 3rd party framework in toolkit by
> bsmedberg.
> 
> I think josh was against it as well.
> 

Why?

Comment 13

[Shawn Wilsher :sdwilsh]

I can't really argue that because I never agreed with their stance - you'd have to ask them.

Comment 14

[Mike Schroepfer]

(In reply to comment #13)
> I can't really argue that because I never agreed with their stance - you'd have
> to ask them.
> 

Josh/Benjamin if this would be trival to fix by using their lib why don't we?

Comment 15

[Peter Van der Beken [:peterv]]

I don't think using their lib will change anything, we need changes in the growl code *and* in our code. Having our own copy of the growl code might make it even easier to fix, we don't have to wait for a new version of growl. But somebody will have to fix this.

Comment 16

[Shawn Wilsher :sdwilsh]

I'm fairly sure they'd be receptive to getting leaks fixed.

Comment 17

[Peter Van der Beken [:peterv]]

Could we store the nsIObserver in the clickContext, and release it when the clickContext dies? The clickContext should die quickly if Growl notifications have been turned off and otherwise it will stay alive until growlNotificationTimedOut or growlNotificationWasClicked are called.

Comment 18

[Shawn Wilsher :sdwilsh]

oh hey, that's a *really* good idea!  My only question is how can we have it call NS_RELEASE on the pointer to the nsIObserver?  Objective-C isn't exactly my strong point, so pardon me if that's a stupid question.

Comment 19

[Peter Van der Beken [:peterv]]

I've been pondering that, we'll probably need a small Objective-C object that stores the observer in a member variable (nsIObserver* mObserver) and that has an initWithObserver method. It should override dealloc and do the release from dealloc. Then instead of storing a number in the clickcontext dictionary we store an instance of that object. I think that should work, but I haven't done a lot of Objective-C work myself :-). Something like this:

@interface ObserverHolder : NSObject
{
@private
  nsIObserver* mObserver;
}
- (id) initWithObserver: (nsIObserver*) aObserver;
@end

@implementation ObserverHolder
- (id) initWithObserver: (nsIObserver*) aObserver
{
    self = [super init];
    NS_ADDREF(mObserver = aObserver);
    return self;
}

- (void) dealloc
{
   NS_RELEASE(mObserver);
   [super dealloc];
}
@end

and then change notifyWithName to take an |nsIObserver* aObserver| and instead of adding [NSNumber numberWithUnsignedInt: aKey] add [[ObserverHolder alloc] initWithObserver: aObserver]?

Comment 20

[Peter Van der Beken [:peterv]]

Attachment: Store observer in clickcontext v1

This compiles but haven't tested it yet.

Comment 21

[Shawn Wilsher :sdwilsh]

cbarrett was the one who reviewed this code originally (for the cocoa stuff).  Any toolkit peer will do otherwise.

Comment 22

[Josh Aas]

Honestly I don't remember why I was against the lib last time we had this discussion. Somebody should commit to doing a thorough review of the code for the lib if it goes in, that should probably be me or smichaud.

Did you want to check in source or a binary?

Comment 23

[Samuel Sidler (old account; do not CC)]

(In reply to comment #22)
> Honestly I don't remember why I was against the lib last time we had this
> discussion.

I do! :)

And actually, it made quite a bit of sense, to me. See bug 362685 comment 19 and bug 362685 comment 21.

Comment 24

[Shawn Wilsher :sdwilsh]

Those aren't very strong reasons since we have the same issue with other third party libraries that we use such as SQLite and NSS.

Comment 25

[Josh Aas]

I think both of those projects have a lot more in common with Mozilla than Growl does. I'm not strongly one way or the other right now, but that comparison doesn't make sense to me. Furthermore, the roles that are played by SQLite and NSS are far more important than Growl's, they have a much higher value to us and that matters in an inclusion assessment.

Thanks for refreshing my memory, Sam. Since we have decided to use Growl, it is unlikely we're going to go back on that, and apparently the original impl is already something of a mess (updating is very painful according to sdwilsh, cross-boundary memory leaks like this one), we should probably just go with the framework despite the drawbacks. I should have looked into our original non-framework growl impl more.

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Josh, is anyone working on integrating the framework? Is there a bug for that or is this it?

Comment 27

[Peter Van der Beken [:peterv]]

Comment on attachment 298275 [details] [diff] [review]
Store observer in clickcontext v1

This doesn't work, the context goes through some sort of serialization.

Comment 28

[Shawn Wilsher :sdwilsh]

(In reply to comment #26)
> Josh, is anyone working on integrating the framework? Is there a bug for that
> or is this it?
I don't think anyone is working on this.  I could probably have cycles for it in about two weeks if nobody else has cycles.

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We talked about this at the leak meeting. Sounded like an ideal solution would be if we could find out if notifications will happen at all, but that didn't seem possible right now. 

Another, much less scary than integrating the whole framework, solution for FF3 would be to drop the reference to the window on a timeout.

Comment 30

[Ben Turner (not reading bugmail, use the needinfo flag!)]

P3 per leak meeting.

Comment 31

[Shawn Wilsher :sdwilsh]

(In reply to comment #29)
> Another, much less scary than integrating the whole framework, solution for FF3
> would be to drop the reference to the window on a timeout.
I think you meant observer - the problem is that the observer could be called still, and which point we could seg fault.

Comment 32

[Ben Turner (not reading bugmail, use the needinfo flag!)]

I think the plan was to make the actual observer a small tearoff-ish object that holds a weak reference to the window. When the window goes away it will null out the reference held by the tearoff, and then the tearoff can do something smart like unregistering itself.

Comment 33

[Shawn Wilsher :sdwilsh]

That doesn't sound like it has anything to do with growl then...

Comment 34

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Well, that was the "much less scary than integrating the whole framework solution for FF3" sicking was talking about in comment 29... Not ideal, but hopefully quick and easy to avoid these leaks.

Comment 35

[Shawn Wilsher :sdwilsh]

perhaps I'm misunderstanding - it looks like we are now trying to fix one specific leak that happened to leak a window as opposed to fixing the general condition.  So, what exactly are we looking to do with this bug at this point?

Comment 36

[Shawn Wilsher :sdwilsh]

Filed bug 421551 about upgrading the framework

Comment 37

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I do feel like this is something we should fix as it sounds really bad for the people that do run into this bug.

That said, I could probably be convinced that waiting for a dot-release is fine too.

Comment 38

[Mike Connor [:mconnor]]

This seems like it can wait for a dot release, based on current state.

Comment 39

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v1

This patch fixes the leaks for me... in most cases. Specifically we now properly handle observers that are implemented in JS in the scope of a window. We will still leak JS-implemented observers created in component scopes as well as C++ observers. Post-1.9 we may want to make growl require weak refs for its observers.

Comment 40

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 319467 [details] [diff] [review]
Patch, v1

jst, I'd love for you to look over this XPC stuff...

Comment 41

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 319467 [details] [diff] [review]
Patch, v1

Josh, would you look over the obj-c stuff?

Comment 42

[Josh Aas]

Comment on attachment 319467 [details] [diff] [review]
Patch, v1

>+@interface ObserverPair : NSObject
>+{
>+  nsIObserver *mObserver;
>+  nsIDOMWindow *mWindow;
>+}

Just put public in there for readability.

>+  ObserverPair* pair = [[ObserverPair alloc] initWithObserver:aObserver
>+                                                       window:parentWindow];
> 
>   mKey++;
>-  [mDict setObject: [NSValue valueWithPointer: aObserver]
>+  [mDict setObject: [pair autorelease]
>             forKey: [NSNumber numberWithUnsignedInt: mKey]];

By convention, something you're going to autorelease unconditionally should get autoreleased when you alloc it.

Probably have some more comments in a bit.

Comment 43

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v2

Patch with josh's comments addressed.

Comment 44

[Shawn Wilsher :sdwilsh]

Comment on attachment 319795 [details] [diff] [review]
Patch, v2

>+  ObserverPair* pair = [[ObserverPair alloc] initWithObserver: aObserver
>+                                                        window: parentWindow];
nit: spacing issue (colons don't line up)

>-  return os->AddObserver(this, "final-ui-startup", PR_FALSE);
>+  rv = os->AddObserver(this, "final-ui-startup", PR_FALSE);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  os->AddObserver(this, DOM_WINDOW_DESTROYED_TOPIC, PR_FALSE);
>+  os->AddObserver(this, "profile-before-change", PR_FALSE);
nit: if you don't care about the return value, please cast to void

r=sdwilsh with those fixed

Comment 45

[Dave Townsend [:mossop]]

Why hasn't this landed yet?

Comment 46

[Mike Connor [:mconnor]]

Definitely wanted.

Comment 47

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Pushed changeset 96be65910b79 on trunk.

Comment 48

[Daniel Veditz [:dveditz]]

Fixed in FF3.1, don't see why we can't live with it in 3.0.x