Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: cjones, Assigned: cjones)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

They're broken.  See the "Simultaneous destructors" thread on m.d.t.dom for the full discussion.
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.
Depends on: 525172
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.
Created attachment 412445 [details] [diff] [review]
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.
Attachment #412138 - Attachment is obsolete: true
Attachment #412445 - Flags: review?(bent.mozilla)
Attachment #412445 - Flags: review?(benjamin)

Comment 5

9 years ago
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
Attachment #412445 - Flags: review?(benjamin) → review+
(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__()).
Created attachment 413509 [details] [diff] [review]
v2, mainly delete --> __delete__
Attachment #412445 - Attachment is obsolete: true
Attachment #412445 - Flags: review?(bent.mozilla)
Duplicate of this bug: 527923
Pushed http://hg.mozilla.org/projects/electrolysis/rev/5160e99545e9
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 10

9 years ago
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"?
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?
(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.
You need to log in before you can comment on or make changes to this bug.