1119746 - Reliably establish connection between Gecko and bluetoothd

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

The current code for establishing connections between Gecko and bluetoothd contains race conditions and incompatible with BlueZ. This has to be fixed before we fully enable bluetoothd.

Comment 1

[howie [:howie]]

Hi Thomas, could you help to provide ETA on this? thank you!

Comment 2

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

Hi

(In reply to howie [:howie] from comment #1)
> Hi Thomas, could you help to provide ETA on this? thank you!

I'm actively working on this and have the basic code ready. This still needs several cleanups and reviews. I expect it to be ready by the end of next week (~Jan 16).

Comment 3

[howie [:howie]]

Thank you very much Thomas.

Comment 4

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

Attachment: [01] Bug 1119746: Signal active listening in |ListenSocket|

Comment 5

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

Attachment: [02] Bug 1119746: Support |ListenSocket| in |BluetoothDaemonConnection|

Comment 6

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

Attachment: [03] Bug 1119746: Fix ref-counting of bluetoothd channels

Comment 7

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

Attachment: [04] Bug 1119746: Listen for socket connections when starting Bluetooth

Comment 8

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

Attachment: [05] Bug 1119746: Support random postfix for Bluetooth daemon socket name

Comment 9

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

Attachment: Github tree for bluetoothd

Comment 10

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

Attachment: Github pull request for gonk-misc

FYI: The Bluetooth init script is part of the bluetoothd pull request.

Comment 11

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

Attachment: Github pull request for device-flame

Comment 12

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

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #10)
> Created attachment 8547552 [details]
> Github pull request for gonk-misc
> 
> FYI: The Bluetooth init script is part of the bluetoothd pull request.

I should mention that BlueZ 5 comes with it's own copy of init.bluetooth.rc. By adopting the same name, we'll be able to replace Bluedroid by BlueZ 5 easily.

Comment 13

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

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #12)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #10)
> > Created attachment 8547552 [details]
> > Github pull request for gonk-misc
> > 
> > FYI: The Bluetooth init script is part of the bluetoothd pull request.
> 
> I should mention that BlueZ 5 comes with it's own copy of init.bluetooth.rc.
> By adopting the same name, we'll be able to replace Bluedroid by BlueZ 5
> easily.

Does BlueZ 5 have a API layer similar to bluedroid now too? I thought the whole idea behind bluedroid was to drop dbus, but BlueZ couldn't do that due to GPL?

Comment 14

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

(In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment #13)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #12)
> > (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #10)
> > > Created attachment 8547552 [details]
> > > Github pull request for gonk-misc
> > > 
> > > FYI: The Bluetooth init script is part of the bluetoothd pull request.
> > 
> > I should mention that BlueZ 5 comes with it's own copy of init.bluetooth.rc.
> > By adopting the same name, we'll be able to replace Bluedroid by BlueZ 5
> > easily.
> 
> Does BlueZ 5 have a API layer similar to bluedroid now too? I thought the
> whole idea behind bluedroid was to drop dbus, but BlueZ couldn't do that due
> to GPL?

The BlueZ project wrote a Bluedroid driver that talks to the BlueZ 5 daemon. We use the same protocol for talking to our daemon. But it all happens at the raw socket level, no DBus is involved. More infos are at [1] and the protocol is at [2].

[1] https://lwn.net/Articles/597293/
[2] https://git.kernel.org/cgit/bluetooth/bluez.git/tree/android/hal-ipc-api.txt

Comment 15

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

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #14)
> The BlueZ project wrote a Bluedroid driver that talks to the BlueZ 5 daemon.
> We use the same protocol for talking to our daemon. But it all happens at
> the raw socket level, no DBus is involved. More infos are at [1] and the
> protocol is at [2].

Does this mean linux desktop bluetooth should now "just work" if I have a kernel with BlueZ 5?

Comment 16

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

(In reply to Kyle Machulis [:kmachulis] [:qdot] (USE NEEDINFO?) from comment #15)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #14)
> > The BlueZ project wrote a Bluedroid driver that talks to the BlueZ 5 daemon.
> > We use the same protocol for talking to our daemon. But it all happens at
> > the raw socket level, no DBus is involved. More infos are at [1] and the
> > protocol is at [2].
> 
> Does this mean linux desktop bluetooth should now "just work" if I have a
> kernel with BlueZ 5?

No, Desktop still uses DBus.

Comment 17

[Michael Wu [:mwu]]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #12)
> I should mention that BlueZ 5 comes with it's own copy of init.bluetooth.rc.
> By adopting the same name, we'll be able to replace Bluedroid by BlueZ 5
> easily.

Is it much different than our copy? I'm more inclined to just directly inline the thing rather than add a new init.rc import.

Comment 18

[Ben Tian (inactive)]

Comment on attachment 8547543 [details] [diff] [review]
[03] Bug 1119746: Fix ref-counting of bluetoothd channels

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

LGTM.

Comment 19

[Ben Tian (inactive)]

Comment on attachment 8547545 [details] [diff] [review]
[04] Bug 1119746: Listen for socket connections when starting Bluetooth

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

Please see my question below. Also in patch description I think "we" should be "when" as following.

  Currently, Gecko connects to a running instance of bluetoothd "when" it
  starts the daemon backend. ...

::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp
@@ +1947,5 @@
> +      BT_WARNING("Address too long for socket struct!");
> +      return false;
> +    }
> +
> +    memset(aAddr.un.sun_path, '\0', sNameOffset); // abstract socket

Why is '\0' required at the beginning of |sun_path|?

@@ +1994,5 @@
> + *
> + *  (2) Start the Bluetooth daemon: When the daemon starts up it will
> + *      open two socket connections to Gecko and thus create the command
> + *      and notification channels. Gecko already opened the listen socket
> + *      in step (1). Step (2) end with the creation of the command channel.

nit: end's'

@@ +2000,5 @@
> + *  (3) Start listening for the notification channel's socket connection:
> + *      At the end of step (2), the command channel was opened by the
> + *      daemon. In step (3), the daemon immediately tries to open the next
> + *      socket for the notification channel. Gecko will now accept an
> + *      incoming connection requests for this socket. The listen socket

I think 'an' is redundant, right?

... accept incoming connection requests for this socket.

Comment 20

[Ben Tian (inactive)]

Comment on attachment 8547546 [details] [diff] [review]
[05] Bug 1119746: Support random postfix for Bluetooth daemon socket name

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

r=me with comment addressed.

::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp
@@ +2096,5 @@
> +  // The listen socket's name is generated with a random postfix. This
> +  // avoids naming collisions if we still have a listen socket from a
> +  // previously failed cleanup. It also makes it hard for malicious
> +  // external programs to capture the socket name or connect before
> +  // the daemon can do so. If no random prefix can be generated, we

random 'postfix'

Comment 21

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

(In reply to Michael Wu [:mwu] from comment #17)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #12)
> > I should mention that BlueZ 5 comes with it's own copy of init.bluetooth.rc.
> > By adopting the same name, we'll be able to replace Bluedroid by BlueZ 5
> > easily.
> 
> Is it much different than our copy? I'm more inclined to just directly
> inline the thing rather than add a new init.rc import.

Yes, please have a look at [1]. BlueZ uses a fixed socket name, but I added the ability to configure the socket name as parameter when the daemon gets started. That makes the whole thing more robust, but required changes to the init script.

BTW Why do yo not what to import additional init scripts? Currently, we duplicate several rules for NFC and RIL in init.b2g.rc and init.target.rc. Having those rules in a shared script would seem like an improvement to me.

[1] https://git.kernel.org/cgit/bluetooth/bluez.git/tree/android/init.bluetooth.rc

Comment 22

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

Comment on attachment 8547548 [details]
Github tree for bluetoothd

I have left some comments on PR. Overall is good. Can you answer my question? Thanks.

Comment 23

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

Comment on attachment 8547545 [details] [diff] [review]
[04] Bug 1119746: Listen for socket connections when starting Bluetooth

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

::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp
@@ +1868,5 @@
>        // Close command channel
>        mCmdChannel->CloseSocket();
> +    case CMD_CHANNEL:
> +      // Stop daemon and close listen socket
> +      NS_WARN_IF(property_set("ctl.stop", "bluetoothd"));

This may cause compiling error.
../../../../../../../code/b2g37_v2_2/mozilla-b2g37_v2_2/dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp:1877:53: error: value computed is not used [-Werror=unused-value]
../../../../../../../code/b2g37_v2_2/mozilla-b2g37_v2_2/dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp: In member function 'virtual void mozilla::dom::bluetooth::BluetoothDaemonInterface::Init(mozilla::dom::bluetooth::BluetoothNotificationHandler*, mozilla::dom::bluetooth::BluetoothResultHandler*)':

@@ +2022,5 @@
> +  static const char BASE_SOCKET_NAME[] = "bluetoothd";
> +
> +  // If we could not cleanup before and an old instance of the
> +  // daemon is still running, we kill it here.
> +  NS_WARN_IF(property_set("ctl.stop", "bluetoothd"));

../../../../../../../code/b2g37_v2_2/mozilla-b2g37_v2_2/dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp:2072:49: error: value computed is not used [-Werror=unused-value]
cc1plus: all warnings being treated as errors

Comment 24

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

(In reply to Shawn Huang [:shawnjohnjr] from comment #23)
> Comment on attachment 8547545 [details] [diff] [review]
> [04] Bug 1119746: Listen for socket connections when starting Bluetooth
> 
> Review of attachment 8547545 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp
> @@ +1868,5 @@
> >        // Close command channel
> >        mCmdChannel->CloseSocket();
> > +    case CMD_CHANNEL:
> > +      // Stop daemon and close listen socket
> > +      NS_WARN_IF(property_set("ctl.stop", "bluetoothd"));
> 
> This may cause compiling error.
> ../../../../../../../code/b2g37_v2_2/mozilla-b2g37_v2_2/dom/bluetooth/
> bluedroid/BluetoothDaemonInterface.cpp:1877:53: error: value computed is not
> used [-Werror=unused-value]
> ../../../../../../../code/b2g37_v2_2/mozilla-b2g37_v2_2/dom/bluetooth/
> bluedroid/BluetoothDaemonInterface.cpp: In member function 'virtual void
> mozilla::dom::bluetooth::BluetoothDaemonInterface::Init(mozilla::dom::
> bluetooth::BluetoothNotificationHandler*,
> mozilla::dom::bluetooth::BluetoothResultHandler*)':
> 
> @@ +2022,5 @@
> > +  static const char BASE_SOCKET_NAME[] = "bluetoothd";
> > +
> > +  // If we could not cleanup before and an old instance of the
> > +  // daemon is still running, we kill it here.
> > +  NS_WARN_IF(property_set("ctl.stop", "bluetoothd"));
> 
> ../../../../../../../code/b2g37_v2_2/mozilla-b2g37_v2_2/dom/bluetooth/
> bluedroid/BluetoothDaemonInterface.cpp:2072:49: error: value computed is not
> used [-Werror=unused-value]
> cc1plus: all warnings being treated as errors

Thanks for testing. Which platform is that?

Comment 25

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

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #24)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #23)
> > Comment on attachment 8547545 [details] [diff] [review]
> > [04] Bug 1119746: Listen for socket connections when starting Bluetooth
> > 
> > Review of attachment 8547545 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp
> > @@ +1868,5 @@
> > >        // Close command channel
> > >        mCmdChannel->CloseSocket();
> > > +    case CMD_CHANNEL:
> > > +      // Stop daemon and close listen socket
> > > +      NS_WARN_IF(property_set("ctl.stop", "bluetoothd"));
> > 
> > This may cause compiling error.
> > ../../../../../../../code/b2g37_v2_2/mozilla-b2g37_v2_2/dom/bluetooth/
> > bluedroid/BluetoothDaemonInterface.cpp:1877:53: error: value computed is not
> > used [-Werror=unused-value]
> > ../../../../../../../code/b2g37_v2_2/mozilla-b2g37_v2_2/dom/bluetooth/
> > bluedroid/BluetoothDaemonInterface.cpp: In member function 'virtual void
> > mozilla::dom::bluetooth::BluetoothDaemonInterface::Init(mozilla::dom::
> > bluetooth::BluetoothNotificationHandler*,
> > mozilla::dom::bluetooth::BluetoothResultHandler*)':
> > 
> > @@ +2022,5 @@
> > > +  static const char BASE_SOCKET_NAME[] = "bluetoothd";
> > > +
> > > +  // If we could not cleanup before and an old instance of the
> > > +  // daemon is still running, we kill it here.
> > > +  NS_WARN_IF(property_set("ctl.stop", "bluetoothd"));
> > 
> > ../../../../../../../code/b2g37_v2_2/mozilla-b2g37_v2_2/dom/bluetooth/
> > bluedroid/BluetoothDaemonInterface.cpp:2072:49: error: value computed is not
> > used [-Werror=unused-value]
> > cc1plus: all warnings being treated as errors
> 
> Thanks for testing. Which platform is that?

I compiled on flame-kk, maybe it's because gecko enabled '-Werror'.

Comment 26

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

(In reply to Shawn Huang [:shawnjohnjr] from comment #25)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #24)
> > (In reply to Shawn Huang [:shawnjohnjr] from comment #23)
> 
> I compiled on flame-kk, maybe it's because gecko enabled '-Werror'.
See bug 1073003.

Comment 27

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

(In reply to Shawn Huang [:shawnjohnjr] from comment #25)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #24)
> > (In reply to Shawn Huang [:shawnjohnjr] from comment #23)
> > > Comment on attachment 8547545 [details] [diff] [review]
> > > [04] Bug 1119746: Listen for socket connections when starting Bluetooth
> > > 
> > > Review of attachment 8547545 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > ::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp
> > > @@ +1868,5 @@
> > > >        // Close command channel
> > > >        mCmdChannel->CloseSocket();
> > > > +    case CMD_CHANNEL:
> > > > +      // Stop daemon and close listen socket
> > > > +      NS_WARN_IF(property_set("ctl.stop", "bluetoothd"));
> > > 
> > > This may cause compiling error.
> > > ../../../../../../../code/b2g37_v2_2/mozilla-b2g37_v2_2/dom/bluetooth/
> > > bluedroid/BluetoothDaemonInterface.cpp:1877:53: error: value computed is not
> > > used [-Werror=unused-value]
> > > ../../../../../../../code/b2g37_v2_2/mozilla-b2g37_v2_2/dom/bluetooth/
> > > bluedroid/BluetoothDaemonInterface.cpp: In member function 'virtual void
> > > mozilla::dom::bluetooth::BluetoothDaemonInterface::Init(mozilla::dom::
> > > bluetooth::BluetoothNotificationHandler*,
> > > mozilla::dom::bluetooth::BluetoothResultHandler*)':
> > > 
> > > @@ +2022,5 @@
> > > > +  static const char BASE_SOCKET_NAME[] = "bluetoothd";
> > > > +
> > > > +  // If we could not cleanup before and an old instance of the
> > > > +  // daemon is still running, we kill it here.
> > > > +  NS_WARN_IF(property_set("ctl.stop", "bluetoothd"));
> > > 
> > > ../../../../../../../code/b2g37_v2_2/mozilla-b2g37_v2_2/dom/bluetooth/
> > > bluedroid/BluetoothDaemonInterface.cpp:2072:49: error: value computed is not
> > > used [-Werror=unused-value]
> > > cc1plus: all warnings being treated as errors
> > 
> > Thanks for testing. Which platform is that?
> 
> I compiled on flame-kk, maybe it's because gecko enabled '-Werror'.

I guess I should update. :/ flame-kk always built for me.

Comment 28

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

Comment on attachment 8547548 [details]
Github tree for bluetoothd

Updated github pull request:

  - removed out-commented code
  - print custom error for unknown command-line options
  - removed unnecessary triggers from init script

Comment 29

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

One thing I don't understand, i saw there are two places (init.target.rc/init.b2g.rc) actually import "/init.bluetooth.rc". Why do we need to import twice?

Comment 30

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

Comment on attachment 8547546 [details] [diff] [review]
[05] Bug 1119746: Support random postfix for Bluetooth daemon socket name

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

::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp
@@ +1827,5 @@
> +    case LISTEN_SOCKET: {
> +        // Init, step 2: Start Bluetooth daemon */
> +        nsCString value("bluetoothd:-a ");
> +        value.Append(mListenSocketName);
> +        if (NS_WARN_IF(property_set("ctl.start", value.get()) < 0)) {

I'm a little bit curious here, can "ctl.start" take parameters? I thought ctl.start only takes 'service name' which mentioned in init.bluetooth.rc.

Comment 31

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

(In reply to Shawn Huang [:shawnjohnjr] from comment #30)
> Comment on attachment 8547546 [details] [diff] [review]
> [05] Bug 1119746: Support random postfix for Bluetooth daemon socket name
> 
> Review of attachment 8547546 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp
> @@ +1827,5 @@
> I'm a little bit curious here, can "ctl.start" take parameters? I thought
> ctl.start only takes 'service name' which mentioned in init.bluetooth.rc.
Opps, looks like I asked a silly question. I was wrong about ctl.start. :( It looks like parameters indeed pass to bluetoothd.

Comment 32

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

(In reply to Shawn Huang [:shawnjohnjr] from comment #30)
> Comment on attachment 8547546 [details] [diff] [review]
> [05] Bug 1119746: Support random postfix for Bluetooth daemon socket name
> 
> Review of attachment 8547546 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp
> @@ +1827,5 @@
> > +    case LISTEN_SOCKET: {
> > +        // Init, step 2: Start Bluetooth daemon */
> > +        nsCString value("bluetoothd:-a ");
> > +        value.Append(mListenSocketName);
> > +        if (NS_WARN_IF(property_set("ctl.start", value.get()) < 0)) {
> 
> I'm a little bit curious here, can "ctl.start" take parameters? I thought
> ctl.start only takes 'service name' which mentioned in init.bluetooth.rc.

Init has some extra magic for ctl.{start,stop,restart}. Part of that is to allow passing parameters for one-shot services.

Comment 33

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

(In reply to Shawn Huang [:shawnjohnjr] from comment #29)
> One thing I don't understand, i saw there are two places
> (init.target.rc/init.b2g.rc) actually import "/init.bluetooth.rc". Why do we
> need to import twice?

For some reason I don't know, Flame uses init.target.rc instead of init.b2g.rc, or something like that. It doesn't work on Flame if I don't include the Bluetooth script in init.target.rc.

Comment 34

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

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #33)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #29)
> > One thing I don't understand, i saw there are two places
> > (init.target.rc/init.b2g.rc) actually import "/init.bluetooth.rc". Why do we
> > need to import twice?
> 
> For some reason I don't know, Flame uses init.target.rc instead of
> init.b2g.rc, or something like that. It doesn't work on Flame if I don't
> include the Bluetooth script in init.target.rc.

Got it. It looks like only qc platforms don't use init.b2g.rc but the rest of projects use init.b2g.rc file.

Comment 35

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

Hi

> 
> ::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp
> @@ +1947,5 @@
> > +      BT_WARNING("Address too long for socket struct!");
> > +      return false;
> > +    }
> > +
> > +    memset(aAddr.un.sun_path, '\0', sNameOffset); // abstract socket
> 
> Why is '\0' required at the beginning of |sun_path|?

Usually the names of Unix sockets are file paths. But we don't want the socket to show up in the file system; for reasons of resource cleanup and security.

Linux has the feature of 'abstract' socket names.  The extra '\0' at the beginning of the name signals to Linux that the name is just a string and not a file path. The man page has all the details. [1]

> 
> @@ +2000,5 @@
> > + *  (3) Start listening for the notification channel's socket connection:
> > + *      At the end of step (2), the command channel was opened by the
> > + *      daemon. In step (3), the daemon immediately tries to open the next
> > + *      socket for the notification channel. Gecko will now accept an
> > + *      incoming connection requests for this socket. The listen socket
> 
> I think 'an' is redundant, right?

There's an 's' at the end of request. And I'll rewrite the paragraph to make it easier to understand. Thanks for proof-reading all these comments.

[1] http://man7.org/linux/man-pages/man7/unix.7.html

Comment 36

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

Attachment: [04] Bug 1119746: Listen for socket connections when starting Bluetooth (v2)

Changes since v1:

  - fixed typos in comments and commit message

Comment 37

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

Attachment: [05] Bug 1119746: Support random postfix for Bluetooth daemon socket name (v2)

Changes since v1:

  - fixed typos in comments

Comment 38

[Ben Tian (inactive)]

Comment on attachment 8548774 [details] [diff] [review]
[04] Bug 1119746: Listen for socket connections when starting Bluetooth (v2)

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

r=me with nit addressed. Thanks for explanation in comment 35.

::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp
@@ +2000,5 @@
> + *  (3) Start listening for the notification channel's socket connection:
> + *      At the end of step (2), the command channel was opened by the
> + *      daemon. In step (3), the daemon immediately tries to open the
> + *      next socket for the notification channel. Gecko will accept the
> + *      incomming connection request for the notification channel. The

typo: incoming

Comment 39

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

(In reply to Ben Tian [:btian] from comment #38)
> Comment on attachment 8548774 [details] [diff] [review]
> [04] Bug 1119746: Listen for socket connections when starting Bluetooth (v2)
> 
> Review of attachment 8548774 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with nit addressed. Thanks for explanation in comment 35.
> 
> ::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp
> @@ +2000,5 @@
> > + *  (3) Start listening for the notification channel's socket connection:
> > + *      At the end of step (2), the command channel was opened by the
> > + *      daemon. In step (3), the daemon immediately tries to open the
> > + *      next socket for the notification channel. Gecko will accept the
> > + *      incomming connection request for the notification channel. The
> 
> typo: incoming

I made this mistake several times before. I don't know why I never get this right. :D

Comment 40

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

Attachment: [04] Bug 1119746: Listen for socket connections when starting Bluetooth (v3)

Changes since v2:

  - fixed another typo

Comment 41

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

Attachment: [04] Bug 1119746: Listen for socket connections when starting Bluetooth (v4)

Changes since v3:

  - fix compiler error about unused return value

In the previous patch, I forgot about the compile-time error that Shawn reported. I updated, but it still don't see the error.

Comment 42

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

Attachment: [05] Bug 1119746: Support random postfix for Bluetooth daemon socket name (v3)

Changes since v2:

  - rebased onto [04](v4)

Comment 43

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

Great! mwu already r+ :)
Thomas, can you apply v2.2 approval, so we can uplift to v2.2 today. Thanks!

Comment 44

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

Pushed to try:

https://hg.mozilla.org/try/rev/44082c0d5c1e
https://hg.mozilla.org/try/rev/a9a2829f004e
https://hg.mozilla.org/try/rev/ea5ec04d685f
https://hg.mozilla.org/try/rev/0633464eecd3
https://hg.mozilla.org/try/rev/7fcdc0e29d12

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

Comment 45

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

Comment on attachment 8547541 [details] [diff] [review]
[01] Bug 1119746: Signal active listening in |ListenSocket|

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

We need this patch to establish connections between Gecko and bluetoothd, our Bluetooth daemon.

User impact if declined: 

No bluetoothd in v2.2

Testing completed: 

Tested locally on JB, KK platforms

Risk to taking this patch (and alternatives if risky): 

Low. The changes are self-contained in Bluetooth and can be rolled back easily.

String or UUID changes made by this patch:

None.

Comment 46

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

Comment on attachment 8547542 [details] [diff] [review]
[02] Bug 1119746: Support |ListenSocket| in |BluetoothDaemonConnection|

[Approval Request Comment]

Please see comment 45 for these information.

Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 47

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

Comment on attachment 8547543 [details] [diff] [review]
[03] Bug 1119746: Fix ref-counting of bluetoothd channels

[Approval Request Comment]

Please see comment 45 for these information.

Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 48

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

Comment on attachment 8548810 [details] [diff] [review]
[04] Bug 1119746: Listen for socket connections when starting Bluetooth (v4)

[Approval Request Comment]

Please see comment 45 for these information.

Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 49

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

Comment on attachment 8548813 [details] [diff] [review]
[05] Bug 1119746: Support random postfix for Bluetooth daemon socket name (v3)

[Approval Request Comment]

Please see comment 45 for these information.

Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 50

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

Try looks good, please check-in the patches. Thanks!

Comment 51

[Ben Tian (inactive)]

Land patch [01] ~ [05] to b2g-inbound. Clear 'checkin-needed' and sheriff should uplift these changes to v2.2 once we get b2g37 approval.

https://hg.mozilla.org/integration/b2g-inbound/rev/4610ef691ced
https://hg.mozilla.org/integration/b2g-inbound/rev/75befda22a48
https://hg.mozilla.org/integration/b2g-inbound/rev/6dd3d2ad46dc
https://hg.mozilla.org/integration/b2g-inbound/rev/261481178423
https://hg.mozilla.org/integration/b2g-inbound/rev/cff11d5366a7

Comment 52

[Ben Tian (inactive)]

Merge github changes.

bluetoothd: https://github.com/mozilla-b2g/platform_system_bluetoothd/commit/bdc5a9f9602652c3c2626e020da03674a2c2e1b6
gonk-misc: https://github.com/mozilla-b2g/gonk-misc/commit/320dad6b787c296ba701bca8377f71ab5fd72ee8
device-flame: https://github.com/mozilla-b2g/device-flame/commit/e7c90613521145db090dd24147afd5ceb5703190

Comment 53

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

[Tracking Requested - why for this release]:

Comment 54

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

Comment on attachment 8547548 [details]
Github tree for bluetoothd

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

Please see comment 45 for these information.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 55

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

Comment on attachment 8547552 [details]
Github pull request for gonk-misc

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

Please see comment 45 for these information.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 56

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

Comment on attachment 8547555 [details]
Github pull request for device-flame

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

Please see comment 45 for these information.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 57

[bhavana bajaj [:bajaj]]

Eric, can you please help with some exploratory QA around this on master/central ?

Comment 58

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

(In reply to bhavana bajaj [:bajaj] from comment #57)
> Eric, can you please help with some exploratory QA around this on
> master/central ?

Hi bhavana,
QA cannot test because "Bug 1065336 - [Bluetooth] Add support for Bluedroid daemon to Gecko" has not been submitted, so bluetooth daemon is not been enabled.

Bug 1065336 will turn on bluetooth daemon feature and it has been blocked by the current bug (bug 119746). We need to land this patch set first.

Comment 59

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4610ef691ced
https://hg.mozilla.org/mozilla-central/rev/75befda22a48
https://hg.mozilla.org/mozilla-central/rev/6dd3d2ad46dc
https://hg.mozilla.org/mozilla-central/rev/261481178423
https://hg.mozilla.org/mozilla-central/rev/cff11d5366a7

Comment 60

[bhavana bajaj [:bajaj]]

Comment on attachment 8547541 [details] [diff] [review]
[01] Bug 1119746: Signal active listening in |ListenSocket|

I would have preferred one roll-up patch for approval ease ;) may be something to keep in  mind next time approval is seeked :)

Comment 61

[Eric Chang [:ericcc] [:echang]]

Current status: BT L build is not yet ready, daemon not on, need to resolve the root access issue, I think we can test regression after most of the bugs are resolved. Will check with RD if we need specific tests on certain bugs.

Comment 62

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/platform_system_bluetoothd/commit/e15d1bc7b1fb8ccdce8fedec113ab8b893a40e5c
v2.2: https://github.com/mozilla-b2g/gonk-misc/commit/4e87ee8f630b8c94916291406ce07a0de13641a7
v2.2: https://github.com/mozilla-b2g/device-flame/commit/a3747b0794b91d485bdac1d029b37655022ce61e

FWIW, https://github.com/mozilla-b2g/b2g-manifest/blob/v2.2/flame-kk.xml says that device-flame is still pinned to kitkat rather than the v2.2 branch, so I'm guessing that last one was a no-op. Not sure if that's something you want to be fixing, though.

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/cc02f5875f24
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/f665d6780586
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/3eade1f05e1d
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/8d2b4cf6dd57
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/98a618f1dedd