Require ActorDestroy methods to be implemented for parent-side actors

RESOLVED FIXED in mozilla32

Status

()

Core
IPC
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Ben Turner (not reading bugmail, use the needinfo flag!), Assigned: Josh Aas)

Tracking

Trunk
mozilla32
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

ActorDestroy is currently optional in both child and parent actor implementations. We almost always need to handle crashes in the parent so I think we should just bite the bullet and start requiring it to be implemented on the parent side.
(Assignee)

Updated

4 years ago
Assignee: nobody → joshmoz

Comment 1

4 years ago
Roughly the same as bug 610803 (parent-only is correct). Please dup whichever way makes more sense.
Duplicate of this bug: 610803
(Assignee)

Comment 3

4 years ago
Created attachment 8415762 [details] [diff] [review]
fix v1.0

This makes ActorDestroy pure virtual on the parent side. Will attach another patch that adds ActorDestroy to everything missing it now.
(Assignee)

Comment 4

4 years ago
Created attachment 8415763 [details] [diff] [review]
add ActorDestroy where missing, v1.0

This works on Linux. Will probably need a bit more work to catch everything on all platforms.
(Assignee)

Comment 5

4 years ago
Created attachment 8416032 [details] [diff] [review]
add ActorDestroy where missing, v1.1

Fixes for B2G and Windows.
Attachment #8415763 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8415762 - Flags: review?(bent.mozilla)
(Assignee)

Updated

4 years ago
Attachment #8416032 - Flags: review?(bent.mozilla)
(Assignee)

Comment 6

4 years ago
Created attachment 8416236 [details]
Generated PBrowserStreamParent.h (v1.0)
(Assignee)

Comment 7

4 years ago
Created attachment 8416237 [details]
Generated PBrowserStreamParent.cpp (v1.0)

Ben asked me to post example copies of files generated with my patch.
(Assignee)

Comment 8

4 years ago
Created attachment 8416286 [details] [diff] [review]
fix v1.1

Fix bug in example code generation. Don't return false from void ActorDestroy method.
Attachment #8415762 - Attachment is obsolete: true
Attachment #8415762 - Flags: review?(bent.mozilla)
Attachment #8416286 - Flags: review?(bent.mozilla)
Attachment #8416286 - Flags: review?(bent.mozilla) → review+
Comment on attachment 8416032 [details] [diff] [review]
add ActorDestroy where missing, v1.1

Review of attachment 8416032 [details] [diff] [review]:
-----------------------------------------------------------------

This looks fine, though I'd change the comments in each of these to point to a *new* bug. We should just file one for each component to get people to investigate whether they need to do anything in these overrides.
Attachment #8416032 - Flags: review?(bent.mozilla) → review+
(Assignee)

Comment 10

4 years ago
Created attachment 8416605 [details] [diff] [review]
add ActorDestroy where missing, v1.2

Filed specific bugs for everything, this patch includes specific bug #s.
Attachment #8416032 - Attachment is obsolete: true
(Assignee)

Comment 11

4 years ago
pushed the patch adding ActorDestroy methods to all parent-side actors that don't have one

https://hg.mozilla.org/integration/mozilla-inbound/rev/023830b8ad98
(Assignee)

Comment 12

4 years ago
pushed patch requiring ActorDestroy on the parent side

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ece9c3282b7
https://hg.mozilla.org/mozilla-central/rev/023830b8ad98
https://hg.mozilla.org/mozilla-central/rev/2ece9c3282b7
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.