1161020 - Cleanup interface of |UnixSocketConnector|

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

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

The interface of |UnixSocketConnector| should be replaced because it is inconsistent and doesn't follow any design.

The new interface shall provide methods for

  - creating a listen socket,
  - creating a stream socket, and
  - accepting a stream socket from a listen socket.

Comment 1

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=542dd0303875

Comment 2

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

Attachment: [01] Bug 1161020: Added clean interface to |UnixSocketConnector|

Comment 3

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

Attachment: [02] Bug 1161020: Implement new socket-connector interface for NFC

Comment 4

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

Attachment: [03] Bug 1161020: Implement new socket-connector interface for RIL

Comment 5

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

Attachment: [04] Bug 1161020: Implement new socket-connector interface for key store

Comment 6

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

Attachment: [05] Bug 1161020: Implement new socket-connector interface for Bluetooth daemon socket

Comment 7

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

Attachment: [06] Bug 1161020: Implement new socket-connector interface for Bluetooth sockets

Comment 8

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

Attachment: [07] Bug 1161020: Use new socket-connector interface in socket classes

Comment 9

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

Attachment: [08] Bug 1161020: Remove old interface and implementation from socket-connector classes

Comment 10

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

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

Comment 11

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

I tried to split up this patch set to make it easily review-able. The first patch adds a new interface for socket connectors. Patches [02] to [06] implement the interface in different existing connectors. Patch [07] converts socket classes to use the new interface. And finally patch [08] removes all old code.

Comment 12

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 8603283 [details] [diff] [review]
[08] Bug 1161020: Remove old interface and implementation from socket-connector classes

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

Nice! This is so much cleaner than it was. :D

Comment 13

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Comment on attachment 8603277 [details] [diff] [review]
[04] Bug 1161020: Implement new socket-connector interface for key store

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

Awesome!

Comment 14

[Ben Tian (inactive)]

Comment on attachment 8603279 [details] [diff] [review]
[06] Bug 1161020: Implement new socket-connector interface for Bluetooth sockets

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

LGTM.

Comment 15

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8603278 [details] [diff] [review]
[05] Bug 1161020: Implement new socket-connector interface for Bluetooth daemon socket

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

Hi Thomas,
Sorry for late reply. Code goods good.

::: dom/bluetooth/bluedroid/BluetoothDaemonConnector.cpp
@@ +7,5 @@
> +
> +#include "BluetoothDaemonConnector.h"
> +#include <fcntl.h>
> +#include "nsISupportsImpl.h" // For MOZ_COUNT_CTOR, MOZ_COUNT_DTOR
> +#include "nsThreadUtils.h" // For NS_IsMainThread.

Perhaps we don't add comments for include.

@@ +227,5 @@
> +
> +bool
> +BluetoothDaemonConnector::SetUpListenSocket(int aFd)
> +{
> +  return true;

I'm not sure why we directly return true. Because it's "Deprecated"? If so, can you add some comments?

Comment 16

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

Hi

> ::: dom/bluetooth/bluedroid/BluetoothDaemonConnector.cpp
> @@ +7,5 @@
> > +
> > +#include "BluetoothDaemonConnector.h"
> > +#include <fcntl.h>
> > +#include "nsISupportsImpl.h" // For MOZ_COUNT_CTOR, MOZ_COUNT_DTOR
> > +#include "nsThreadUtils.h" // For NS_IsMainThread.

I took this from some original connector, but I can remove these comments. 'nsThreadUtils.h' is removed by the final patch anyway.

> Perhaps we don't add comments for include.
> 
> @@ +227,5 @@
> > +
> > +bool
> > +BluetoothDaemonConnector::SetUpListenSocket(int aFd)
> > +{
> > +  return true;
> 
> I'm not sure why we directly return true. Because it's "Deprecated"? If so,
> can you add some comments?

The deprecated code is completely removed by the final patch. I only copied these deprecated interfaces around to make the overall patch set easier to understand.

Comment 17

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Sorry for being late. Will try to catch up tomorrow.

Comment 18

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Comment on attachment 8603276 [details] [diff] [review]
[03] Bug 1161020: Implement new socket-connector interface for RIL

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

Thank you.

::: ipc/ril/Ril.cpp
@@ -270,5 @@
> -    aAddrSize = strlen(aAddress) + offsetof(struct sockaddr_un, sun_path) + 1;
> -    break;
> -  case AF_INET:
> -    aAddr.in.sin_family = af;
> -    aAddr.in.sin_port = htons(RIL_TEST_PORT + mClientId);

As we are going to remove this line, please remove [1] accordingly, thank you.

[1] https://dxr.mozilla.org/mozilla-central/source/ipc/ril/Ril.cpp#37

Comment 19

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

Attachment: [03] Bug 1161020: Implement new socket-connector interface for RIL (v2)

Changes since v1:

  - removed unused constant

Comment 20

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

Attachment: [05] Bug 1161020: Implement new socket-connector interface for Bluetooth daemon socket (v2)

Changes since v1:

  - rebased on latest m-c
  - removed comments from include statements

The final patch removes the deprecated code.

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/f90b8d3d6b70
https://hg.mozilla.org/integration/b2g-inbound/rev/13753f9043f7
https://hg.mozilla.org/integration/b2g-inbound/rev/34a20b05af6c
https://hg.mozilla.org/integration/b2g-inbound/rev/ac23206e80bd
https://hg.mozilla.org/integration/b2g-inbound/rev/a8f42d85ce3f
https://hg.mozilla.org/integration/b2g-inbound/rev/384de663084c
https://hg.mozilla.org/integration/b2g-inbound/rev/4f782be31f87
https://hg.mozilla.org/integration/b2g-inbound/rev/976e19eac8b5

Comment 22

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

Thanks everyone!

https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=976e19eac8b5

Comment 23

[Carsten Book [:Tomcat]]

sorry had to back this out for memory leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=1947450&repo=b2g-inbound

Comment 24

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/b2g-inbound/rev/b0784c6cf439

Comment 25

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

I added some leak checking to Bluetooth. The problem might be somewhere else and the patch set only uncovered the leak.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c39e192dbdc

Comment 26

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

Resolving some shutdown problems in Bluetooth:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d4965e2b760

Comment 27

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

The leak happens even without the new patches, so it's an old problem. I opened bug 1166175 to investigate this further.

Comment 28

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=038275854ed5

Comment 29

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

Attachment: [01] Bug 1161020: Added clean interface to |UnixSocketConnector| (v2)

Changes since v1:

  - removed leak checks, see bug 1166175

Comment 30

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

Attachment: [02] Bug 1161020: Implement new socket-connector interface for NFC

Changes since v1:

  - removed leak checks, see bug 1166175

Comment 31

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

Attachment: [03] Bug 1161020: Implement new socket-connector interface for RIL (v3)

Changes since v2:

  - removed leak checks, see bug 1166175

Comment 32

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

Attachment: [04] Bug 1161020: Implement new socket-connector interface for key store (v2)

Changes since v1:

  - removed leak checks, see bug 1166175

Comment 33

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

Attachment: [05] Bug 1161020: Implement new socket-connector interface for Bluetooth daemon socket (v3)

Changes since v2:

  - removed leak checks, see bug 1166175

Comment 34

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

Attachment: [06] Bug 1161020: Implement new socket-connector interface for Bluetooth sockets (v2)

Changes since v1:

  - removed leak checks, see bug 1166175

Comment 35

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

Attachment: [08] Bug 1161020: Remove old interface and implementation from socket-connector classes (v2)

Changes since v1:

  - rebased

Comment 36

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/7e5b9e5c24ff
https://hg.mozilla.org/integration/b2g-inbound/rev/1acea1f3dfe5
https://hg.mozilla.org/integration/b2g-inbound/rev/0ba5a258797d
https://hg.mozilla.org/integration/b2g-inbound/rev/85939a63419f
https://hg.mozilla.org/integration/b2g-inbound/rev/b8c39f01f5f4
https://hg.mozilla.org/integration/b2g-inbound/rev/854336cbcbb9
https://hg.mozilla.org/integration/b2g-inbound/rev/6d5e774afa38
https://hg.mozilla.org/integration/b2g-inbound/rev/3529de81a668

Comment 37

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

https://treeherder.mozilla.org/#/jobs?repo=b2g-inbound&revision=3529de81a668

Comment 38

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7e5b9e5c24ff
https://hg.mozilla.org/mozilla-central/rev/1acea1f3dfe5
https://hg.mozilla.org/mozilla-central/rev/0ba5a258797d
https://hg.mozilla.org/mozilla-central/rev/85939a63419f
https://hg.mozilla.org/mozilla-central/rev/b8c39f01f5f4
https://hg.mozilla.org/mozilla-central/rev/854336cbcbb9
https://hg.mozilla.org/mozilla-central/rev/6d5e774afa38
https://hg.mozilla.org/mozilla-central/rev/3529de81a668