Closed Bug 1261307 Opened 10 years ago Closed 10 years ago

Convert B2G socket IPC code to |UniquePtr|

Categories

(Firefox OS Graveyard :: General, defect)

Product:
Firefox OS Graveyard
Component:
General
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox48 fixed)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox48 --- fixed

People

(Reporter: tzimmermann, Assigned: tzimmermann)

Details

Attachments

(4 files, 3 obsolete files)

[01] Bug 1261307: Convert |StreamSocketConsumer::ReceiveSocketData| to |UniquePtr|
4.18 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
[02] Bug 1261307: Convert RIL sockets to |UniquePtr| (v2)
6.22 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
[03] Bug 1261307: Convert NFC IPC code to |UniquePtr| (v2)
3.46 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
[04] Bug 1261307: Convert Unix socket IPC code to |UniquePtr| (v2)
5.27 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
|nsAutoPtr| is going away. Instances have to be replaced with |UniquePtr|.
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?
Attachment #8737130 - Flags: review?(nfroyd) → review+
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.
Attachment #8737131 - Flags: review?(nfroyd) → review+
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.
Attachment #8737132 - Flags: review?(nfroyd) → review+
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.
Attachment #8737133 - Flags: review?(nfroyd) → review+
Changes since v1: - use 'auto' and |MakeUnique| as outlined in comment 6
Attachment #8737131 - Attachment is obsolete: true
Attachment #8738088 - Flags: review+
Changes since v1: - use 'auto' for variable holding the value returned by |MakeUnique|
Attachment #8737132 - Attachment is obsolete: true
Attachment #8738089 - Flags: review+
Changes since v1: - provide |Duplicate| method that takes a |UniquePtr| - don't store output argument of |PrepareAccept| in |UniquePtr|
Attachment #8737133 - Attachment is obsolete: true
Attachment #8738090 - Flags: review+
> ::: 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.
> 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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: