329937 - Just say no to kungFuDeathGrip(this)

Status: NEW

(Core :: XPCOM, defect, P3)

Description

[Darin Fisher]

Just say no to kungFuDeathGrip(this)

It is almost always wrong.  When a xpcom method is called, the caller should have an owning reference.  If they did, then there would be no reason to take a kungFuDeathGrip on this.  I assert that kungFuDeathGrips on this increase the complexity of code.  We should avoid them if possible.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What if the caller has an owning reference in a member variable, but we nest calls to arbitrary user JavaScript or event processing within the call, nulling out the member variable?  That's a common reason for things like this.  Is it required to copy owning member variables before using them?

That said, I wish we didn't nest event loops and that pretty much all JavaScript notifications happened pretty much directly off the main event loop...

Comment 2

[Darin Fisher]

> What if the caller has an owning reference in a member variable, but we nest
> calls to arbitrary user JavaScript or event processing within the call, nulling
> out the member variable?  That's a common reason for things like this.  Is it
> required to copy owning member variables before using them?

This question was also asked in #content today.  My answer is:

"if the caller allows its reference to the callee to be cleared during the method call, then it is up to the caller to ensure that it has a local reference for the duration of the method call."

I think that makes the most sense.  You need to protect yourself when you call out into code that you don't control.

In other words, when we feel the temptation to kungFuDeathGrip(this), perhaps we should look at who is calling this and determine if we can't ensure that they take a local reference to this instead.


> That said, I wish we didn't nest event loops and that pretty much all
> JavaScript notifications happened pretty much directly off the main event
> loop...

How would synchronous XMLHttpRequest work then? ;-)  OK, this is actually a different conversation entirely.  Here is probably not the best forum for that.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I suppose the proposal seems reasonable in that it gives a consistent rule for where the ugliness should go, but I don't think it makes anything less ugly.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Actually, I even agree with it; I'd rather see the whole kungFuDeathGrip business go away, but kungFuDeathGrip(this) is unusually bad.

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #4)
> Actually, I even agree with it; I'd rather see the whole kungFuDeathGrip
> business go away,

What would you have instead?  Either conservative GC at one extreme, or flatten all control flows through idle events.  Neither is more appealing to me than a fixed status quo, at least in the next year.

> but kungFuDeathGrip(this) is unusually bad.

Agreed.

/be 

Comment 6

[Jesse Ruderman]

Bug 359203 added something that looks a bit like a kungFuDeathGrip(this), but with a weak pointer and a different name.

Comment 7

[Sumit Agrawal[:sumit4iit]]

Hi Benjamin,

I would like to take this bug on. Will you please give me some pointers on this bug as I am new to mozilla codebase? 

Thanks.

Comment 8

[Mike Hoye [:mhoye]]

Sumit - Are you still interested in this bug? Please let me know, and we'll help you get started.

Comment 9

[Sumit Agrawal[:sumit4iit]]

Sure I would love to.

Comment 10

[Mike Hoye [:mhoye]]

Great! You'll need to get your development environment set up, as per:

https://developer.mozilla.org/en-US/docs/Developer_Guide/Build_Instructions

Once you're set up and ready to go, I'll ask bsmedberg to give you some guidance as to the next steps.

Comment 11

[Sumit Agrawal[:sumit4iit]]

Mike I have set up the build environment. I was able to compile it successfully. Please let me know about the further steps.

Comment 12

[Mike Hoye [:mhoye]]

Excellent; I'm going to use the needinfo flag here to ask my colleage Benjamin Smedberg for more information.

Comment 13

[Benjamin Smedberg]

This is not an easy bug at all and requires significant experience. Here's a search of the existing 350 cases of kungFuDeathGrip:

http://mxr.mozilla.org/mozilla-central/search?string=kungfudeathgrip

Not all of these are deathgrip of `this`, so you first need to filter out the ones which are gripping themself which is the bug here.

For each of these cases, you would need to understand why the code in question currently has a deathgrip: this may involve reading hg blame or even back into cvs blame (the git blame from github.com/mozilla/gecko-dev may be most useful to you since it combines the HG and CVS history). It will likely involve reading a bunch of bug history.

The correct patch in each case may be different. It may involve removing the deathgrip completely, or moving it to the caller who should actually be holding the reference.

You need to file a separate bug for each instance, submitting the patch from the appropriate reviewer for each module. I can help you find the correct module owner if it's not clear from the modules list at https://wiki.mozilla.org/Modules/All

Comment 14

[Jeff Walden [:Waldo]]

I don't think this is quite as terribly complicated as comment 13 makes it out to be.

At least as to kFDG of this, inside a member function, it should be reasonably straightforward to move the protected code to a static function (perhaps a static member function -- perhaps changing an existing member function to be static to eliminate the problem fully), that accepts as its first argument a smart pointer. The code can pass in this as a smart pointer, and the "gripped" code will be safe against refcount dropping to zero.

This is effectively the pattern SpiderMonkey followed, to convert large numbers of its internal functions to an exact-GC model from a previous conservative stack-scanning GC. It's mildly obnoxious to do, but it's fundamentally straightforward.

Doing this ought not require special understanding of the code in question, and it should be incrementally doable in bite-sized patches that should be fairly quick to review.