1261307 - Convert B2G socket IPC code to |UniquePtr|

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Thomas Zimmermann [:tzimmermann] [:tdz]]

|nsAutoPtr| is going away. Instances have to be replaced with |UniquePtr|.

Comment 1

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [01] Bug 1261307: Convert |StreamSocketConsumer::ReceiveSocketData| to |UniquePtr|

Comment 2

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1261307: Convert RIL sockets to |UniquePtr|

Comment 3

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 1261307: Convert NFC IPC code to |UniquePtr|

Comment 4

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [04] Bug 1261307: Convert Unix socket IPC code to |UniquePtr|

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8737130 [details] [diff] [review]
[01] Bug 1261307: Convert |StreamSocketConsumer::ReceiveSocketData| to |UniquePtr|

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

r=me.  Thanks for doing this.

::: ipc/unixsocket/StreamSocketConsumer.h
@@ +27,4 @@
>     * @param aBuffer Data received from the socket.
>     */
>    virtual void ReceiveSocketData(int aIndex,
> +                                 UniquePtr<UnixSocketBuffer>& aBuffer) = 0;

It would be better for this parameter to be a |const UniquePtr&| so it's clear that implementations aren't going to assign into it.  (The internal |UnixSocketBuffer*| itself isn't |const|, so can still be manipulated normally.)  But the lack of const-ness is pre-existing, and doesn't seem worth fixing up here.  Followup bug?

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 8737131 [details] [diff] [review]
[02] Bug 1261307: Convert RIL sockets to |UniquePtr|

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

r=me with the two MakeUnique changes below.

::: ipc/ril/Ril.cpp
@@ +233,4 @@
>      return NS_ERROR_FAILURE;
>    }
>  
> +  mSocket->SendSocketData(raw.release());

It would be splendid if |SendSocketData| took a |UniquePtr| here, so we had less raw pointer manipulation.  But that's a patch for another day.

@@ +339,4 @@
>    }
>  
>    // Now that we're set up, connect ourselves to the RIL thread.
> +  sRilWorkers[aClientId].reset(new RilWorker(aDispatcher));

This is a little more idiomatic as:

  sRilWorkers[aClientId] = MakeUnique<RilWorker>(aDispatcher);

@@ +386,4 @@
>  
>      MOZ_ASSERT(!sRilConsumers[mClientId]);
>  
> +    UniquePtr<RilConsumer> rilConsumer = MakeUnique<RilConsumer>();

We usually write this as:

  auto rilConsumer = MakeUnique<RilConsumer>();

since the type is easily seen in context, and this way you don't repeat the type.

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8737132 [details] [diff] [review]
[03] Bug 1261307: Convert NFC IPC code to |UniquePtr|

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

::: dom/nfc/gonk/NfcService.cpp
@@ +490,4 @@
>    MOZ_ASSERT(!mThread);
>    MOZ_ASSERT(!mNfcConsumer);
>  
> +  UniquePtr<NfcConsumer> nfcConsumer = MakeUnique<NfcConsumer>(this);

|auto| usage would be appropriate here.

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8737133 [details] [diff] [review]
[04] Bug 1261307: Convert Unix socket IPC code to |UniquePtr|

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

r=me with the changes to ListenSocket and the ConnectionOrientedSocketIO ownership stuff.  Whether you make the changes around |Duplicate| and |ReceiveTask| is up to you.

::: ipc/unixsocket/ListenSocket.cpp
@@ +361,5 @@
>    // We first prepare the connection-oriented socket with a
>    // socket connector and a socket I/O class.
>  
> +  UnixSocketConnector* connectorPtr;
> +  nsresult rv = mIO->GetConnector()->Duplicate(connectorPtr);

This could be instead:

  UniquePtr<UnixSocketConnector> connector;
  nsresult rv = mIO->GetConnector()->Duplicate(connector);

and then e.g. DaemonSocketConnector::Duplicate(UniquePtr<UnixSocketConnector>&) would just have:

  aConnector = MakeUnique<DaemonSocketConnector>(*this);

@@ +374,2 @@
>                                  mIO->GetConsumerThread(), mIO->GetIOLoop(),
> +                                ioPtr);

You could make this:

UniquePtr<ConnectionOrientedSocketIO> io;
rv = aCOSocket->PrepareAccept(..., io);

...but I suppose that would require more invasive changes.

Actually, looking at implementations of PrepareAccept, I'm not sure using UniquePtr is appropriate here, since the implementing classes all hold a reference to the object they return.

@@ +376,5 @@
>    if (NS_FAILED(rv)) {
>      return rv;
>    }
> +
> +  UniquePtr<ConnectionOrientedSocketIO> io(ioPtr);

I don't know that holding a UniquePtr here is appropriate, as that suggests that you own the pointer.  But AFAICT, |aCOSocket| is also holding a pointer and |aCOSocket| is the real owner of the pointer, not this code.  I think using a raw pointer for ConnectionOrientedSocketIO here is more appropriate, perhaps with a comment, and avoids the illusion that you are passing ownership around.

::: ipc/unixsocket/SocketBase.cpp
@@ +403,4 @@
>  void
>  SocketDeleteInstanceTask::Run()
>  {
> +  mIO.reset(); // delete instance

FWIW, |mIO = nullptr| accomplishes the same thing here, though I think your version makes it more obvious that something interesting is happening.

::: ipc/unixsocket/StreamSocket.cpp
@@ +234,4 @@
>  StreamSocketIO::ConsumeBuffer()
>  {
>    GetConsumerThread()->PostTask(FROM_HERE,
> +                                new ReceiveTask(this, mBuffer.release()));

Making ReceiveTask accept a |UniquePtr| here and passing |Move(mBuffer)| would be more idiomatic.

Comment 9

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [02] Bug 1261307: Convert RIL sockets to |UniquePtr| (v2)

Changes since v1:

  - use 'auto' and |MakeUnique| as outlined in comment 6

Comment 10

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [03] Bug 1261307: Convert NFC IPC code to |UniquePtr| (v2)

Changes since v1:

  - use 'auto' for variable holding the value returned by |MakeUnique|

Comment 11

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Attachment: [04] Bug 1261307: Convert Unix socket IPC code to |UniquePtr| (v2)

Changes since v1:

  - provide |Duplicate| method that takes a |UniquePtr|
  - don't store output argument of |PrepareAccept| in |UniquePtr|

Comment 12

[Thomas Zimmermann [:tzimmermann] [:tdz]]

> ::: ipc/unixsocket/StreamSocketConsumer.h
> @@ +27,4 @@
> >     * @param aBuffer Data received from the socket.
> >     */
> >    virtual void ReceiveSocketData(int aIndex,
> > +                                 UniquePtr<UnixSocketBuffer>& aBuffer) = 0;
> 
> It would be better for this parameter to be a |const UniquePtr&| so it's
> clear that implementations aren't going to assign into it.

IIRC one of the users of this interface (RIL?, NFC?) take the contained buffer for further processing on a different thread. Having this 'const' would be better, but would first require some refactoring elsewhere.

Comment 13

[Thomas Zimmermann [:tzimmermann] [:tdz]]

> This could be instead:
> 
>   UniquePtr<UnixSocketConnector> connector;
>   nsresult rv = mIO->GetConnector()->Duplicate(connector);
> 
> and then e.g.
> DaemonSocketConnector::Duplicate(UniquePtr<UnixSocketConnector>&) would just
> have:
> 
>   aConnector = MakeUnique<DaemonSocketConnector>(*this);

This would require changes to a number of users. Something I'd like to avoid in this bug report. So I added |UnixSocketConnector::Duplicate(UniquePtr<UnixSocketConnector>&)|, which wraps the raw pointer and the call to the original |Duplicate| method.
 
> Actually, looking at implementations of PrepareAccept, I'm not sure using
> UniquePtr is appropriate here, since the implementing classes all hold a
> reference to the object they return.

Thanks for finding this. Seems to have been a bug in the code.

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e7d1dab0ba6f
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d72a998142a
https://hg.mozilla.org/integration/mozilla-inbound/rev/26d254216676
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e2855721acc

Comment 15

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Thanks!

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7e2855721acc

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e7d1dab0ba6f
https://hg.mozilla.org/mozilla-central/rev/6d72a998142a
https://hg.mozilla.org/mozilla-central/rev/26d254216676
https://hg.mozilla.org/mozilla-central/rev/7e2855721acc