1283744 - Clean up adhoc transport memory management

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

We have a bunch of adhoc code to manage deleting Transports. It'd be nice to share this across top-level protocols.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: patch

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e7f6d471352

Comment 3

[Andrew McCreight [:mccr8] (PTO until 12-2)]

I'd only bother with ASan at first. My naive attempts to fix this caused use-after-frees.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: patch

Adding a flag to SetTransport() didn't work, because SetTransport is usually called twice - once in the actor setup code, and again in IPDL. It might be possible to move the "SetOwnsTransport" call entirely into IPDL, since it's generating the Open() calls. But I didn't look that far yet.

This is looking better on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d23bce3962b36109e62c6dc25f94778acf5ab792

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: patch

whoops wrong file

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8767444 [details] [diff] [review]
patch

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

This is definitely an improvement and I think we could land it, modulo the review comments. But I think we could improve without much more work, assuming I understand things. The most common pattern for calling SetTransport seems to be as follows:

                    Transport* t;
                    PGMPContentChild* p;
                    if ((!(t = mozilla::ipc::OpenDescriptor(td, mozilla::ipc::Transport::MODE_CLIENT)))) {
                        return MsgValueError;
                    }
                    if ((!(p = AllocPGMPContentChild(t, pid)))) {
                        return MsgProcessingError;
                    }
                    (p)->IToplevelProtocol::SetTransport(t);
                    IToplevelProtocol::AddOpenedActor(p);

I would feel better if we stored the Transport in a UniquePtr that would be passed to SetTransport. SetTransport would always cause the top-level protocol to take ownership of the Transport. Looking at the other SetTransport callers, I see only a few exceptions:

1. ContentParent calls it and I don't see why the call is necessary. Can you try removing this call and seeing if anything breaks? There aren't any GetTransport calls in ContentParent.cpp and I don't see any others that might be on a ContentParent.

2. This calls just seems unnecessary to me:
http://searchfox.org/mozilla-central/rev/a7c8e9f3cc323fd707659175a46826ad12899cd1/gfx/vr/ipc/VRManagerParent.cpp#27

All the other callers are either from generated code for Bridge/Open or else happen for Nuwa CloneTopLevelProtocol. In both cases, the actor will own the transport and we can use UniquePtr.

Once this is done, we don't need SetOwnsTransport.

::: gfx/layers/ipc/CompositorBridgeParent.cpp
@@ -2794,5 @@
>                                              Transport::MODE_SERVER);
>        PCompositorBridgeParent* compositor =
>          CompositorBridgeParent::Create(transport, base::GetProcId(aPeerProcess));
>        compositor->CloneManagees(this, aCtx);
> -      compositor->IToplevelProtocol::SetTransport(transport);

Did you mean to change this?

::: ipc/glue/BackgroundImpl.cpp
@@ +1520,5 @@
>    if (!actor->Open(mTransport, mOtherPid, XRE_GetIOMessageLoop(), ParentSide)) {
>      actor->Destroy();
>      return NS_ERROR_FAILURE;
>    }
> +  actor->SetOwnsTransport();

Could this be done in the constructor instead? Or do you only want to do it if the Open call succeeds? Who owns the transport if Open fails? (As far as I can tell, no one does.)

@@ +1908,5 @@
>      }
>  
>      return NS_OK;
>    }
> +  strongActor->SetOwnsTransport();

Same here.

::: ipc/glue/ProtocolUtils.h
@@ +239,5 @@
>      {
>          mTrans = aTrans;
>      }
> +    void SetOwnsTransport() {
> +        mOwnsTransport = true;

I'm a bit worried about thread safety here, but we can always make this atomic if TSan complains.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: v2

I think this might be close to the idea in comment #6 - the NUWA code still has some weird boilerplate (it'd be nice if ownership was transferred via Open somehow), but this is still better.

Linux-only try run looked positive, so doing a full run now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ecbbe5c2f60c0e34d56ecd46932d016e53e0f6d

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: v2

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8768315 [details] [diff] [review]
v2

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

Awesome!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/032810ad5283
Clean up Transport memory management in IPDL. (bug 1283744, r=billm)

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/032810ad5283