525342 - IPDL: Fix destructors

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

They're broken.  See the "Simultaneous destructors" thread on m.d.t.dom for the full discussion.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: backup of IPDL-only changes

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Unfortunately implementing this new scheme introduces a circular manager/managed dependency.  I looked into hacky workarounds, but couldn't find anything simple enough to make it more attractive than fixing bug 525172.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I got the fix for this bug finished up tonight, but when applying on top of bug 525172 I got a really nasty ugly error (the patch I wrote for this bug on top of 524172 was "ignored" by hg).  Will fix sometime in the near future.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: v1

There's a fair amount of crud in this patch, but the essential part is

  manager->SendFooDestructor(foo) ==> Foo::Sendelete(foo)
  manager->RecvFooDestructor() ==>  foo->Recvdelete()
  DeallocFoo(foo, ...) ==> DeallocFoo(foo)

r? to bent and bsmedberg, as this is a rather invasive change.

Comment 5

[Benjamin Smedberg]

Comment on attachment 412445 [details] [diff] [review]
v1

There are a few general style nits: I'm not happy with the delete() name... it seems like it would conflict too easily with a non-special message name. If we want to avoid confusion with C++ (and therefore ~PProtocolName), perhaps something like __delete__ from python? In any case, let's use Delete() instead of delete()... the unwritten IPDL style guide has decided that UpperCase message names are standard.

There are a couple cases where operators like -> are at the beginning of the line instead of the end of the line. Also, a couple places do

  static_cast<Foo*>(something)->Method(
    param, param2);

In general I'd prefer these to be:
  static_cast<Foo*>(something)->
    Method(param, param2);

because it's hard for me to read the opening paren and the first argument on the next line

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #5)
> (From update of attachment 412445 [details] [diff] [review])
> There are a few general style nits: I'm not happy with the delete() name... it
> seems like it would conflict too easily with a non-special message name.

What do you mean?  Like if someone created a |Delete()| message?

> If we
> want to avoid confusion with C++ (and therefore ~PProtocolName), perhaps
> something like __delete__ from python? In any case, let's use Delete() instead
> of delete()... the unwritten IPDL style guide has decided that UpperCase
> message names are standard.
> 

Yeah, I kind of like __delete__ ...  No need to automagically title-case that either, since it wouldn't look raunchy like Senddelete() does (Send__delete__()).

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: v2, mainly delete --> __delete__

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Pushed http://hg.mozilla.org/projects/electrolysis/rev/5160e99545e9

Comment 10

[Jason Duell]

cjones: Any particular reason why you chose to use "static Send__delete(pActor)" instead of just making this a member function, i.e. "pActor->Send_delete"?

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Two reasons: first, it feels weird to me to make a method call that results in the instance being deleted (but yes, this can happen other ways with RPC).  If it were The Mozilla Way, I would have made the signature Send__delete__(Foo**) and null'd out the pointer.  Second, I suspect without evidence that Send__delete__() would be invoked on more null and already-deleted pointers than other message methods, so having __delete__ be a static function means we invoke __delete__ without reading the instance's vtable pointer and can then give a better error message than "crash" if the pointer is indeed invalid.

But I'm not particularly wedded to this design.  Do you have a use case for which you'd prefer Send__delete__() to be a method call?

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #11)
> Second, I suspect without evidence that
> Send__delete__() would be invoked on more null and already-deleted pointers
> than other message methods, so having __delete__ be a static function means we
> invoke __delete__ without reading the instance's vtable pointer and can then
> give a better error message than "crash" if the pointer is indeed invalid.
> 

Actually, duh, Send__delete__() as a instance method wouldn't be virtual, so this point is bogus :S.