Cleanup interface of |UnixSocketConnector|

RESOLVED FIXED in Firefox 41

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: tzimmermann, Assigned: tzimmermann)

Tracking

unspecified
2.2 S13 (29may)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(8 attachments, 9 obsolete attachments)

26.81 KB, patch
qdot
: review+
Details | Diff | Splinter Review
7.51 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
14.57 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
15.25 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
16.17 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
14.16 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
18.11 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
27.61 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
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.
Summary: Cleanup interface if |UnixSocketConnector| → Cleanup interface of |UnixSocketConnector|
Depends on: 1162524
Depends on: 1162585
Attachment #8603278 - Attachment description: [05] Bug 1161020: Implement new socket-connector interface for Bluetooth daemon socket05_update_bluetooth_daemon.patch → [05] Bug 1161020: Implement new socket-connector interface for Bluetooth daemon socket
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.
Attachment #8603272 - Flags: review?(kyle) → review+
Attachment #8603281 - Flags: review?(kyle) → review+
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
Attachment #8603283 - Flags: review?(kyle) → review+
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!
Attachment #8603277 - Flags: review?(chuckli0706) → review+
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.
Attachment #8603279 - Flags: review?(btian) → review+
Attachment #8603273 - Flags: review?(allstars.chh) → review+
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?
Attachment #8603278 - Flags: review?(shuang) → review+
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.
Sorry for being late. Will try to catch up tomorrow.
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
Attachment #8603276 - Flags: review?(htsai) → review+
Changes since v1:

  - removed unused constant
Attachment #8603276 - Attachment is obsolete: true
Attachment #8606940 - Flags: review+
Changes since v1:

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

The final patch removes the deprecated code.
Attachment #8603278 - Attachment is obsolete: true
Attachment #8606943 - Flags: review+
sorry had to back this out for memory leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=1947450&repo=b2g-inbound
Flags: needinfo?(tzimmermann)
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
Flags: needinfo?(tzimmermann)
The leak happens even without the new patches, so it's an old problem. I opened bug 1166175 to investigate this further.
See Also: → 1166175
Changes since v1:

  - removed leak checks, see bug 1166175
Attachment #8603272 - Attachment is obsolete: true
Attachment #8607466 - Flags: review+
Changes since v1:

  - removed leak checks, see bug 1166175
Attachment #8603273 - Attachment is obsolete: true
Attachment #8607467 - Flags: review+
Changes since v2:

  - removed leak checks, see bug 1166175
Attachment #8606940 - Attachment is obsolete: true
Attachment #8607469 - Flags: review+
Changes since v1:

  - removed leak checks, see bug 1166175
Attachment #8603277 - Attachment is obsolete: true
Attachment #8607470 - Flags: review+
Changes since v2:

  - removed leak checks, see bug 1166175
Attachment #8606943 - Attachment is obsolete: true
Attachment #8607471 - Flags: review+
Changes since v1:

  - removed leak checks, see bug 1166175
Attachment #8603279 - Attachment is obsolete: true
Attachment #8607472 - Flags: review+
You need to log in before you can comment on or make changes to this bug.