979668 - [B2G] [Bluetooth] Sender name is not shown in transfer requests

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Chris Kreinbring [:CKreinbring]]

Attachment: Screenshot of unknown Bluetooth sender

Description:
Received Bluetooth transfers are shown to be from an unknown sender instead of the sending device's name.

Repro Steps:
1) Update Buri to Build ID: 20140304040200
2) Pair to another device via Bluetooth.
3) From the other device send a file to the test device.
4) When the notification appears on the test device, observe the title of the transfer request then tap it.
5) Observe the name of the sending device in the transfer request screen.

Actual:
The name of the sender is shown as "unknown device".

Expected:
The name of the sender is shown correctly.

Environmental Variables
Device: Buri 1.4 mozilla RIL
Build ID: 20140304040200
Gecko: https://hg.mozilla.org/mozilla-central/rev/529b86b92b1d
Gaia: 6df4533f14ec2645bb13d8a803a5151583ca13a8
Platform Version: 30.0a1
Firmware Version: 1.2-device.cfg

Notes:
Repro frequency: 100%
Link to failed test case: https://moztrap.mozilla.org/manage/cases/?filter-id=4095
See attached screenshot and logcat logs

Comment 1

[Chris Kreinbring [:CKreinbring]]

Does not repro on Buri 1.3 mozilla RIL

Build ID: 20140304004001
Gecko: https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/8b90bef8738c
Gaia: 242e477761643a440e6b244d291fb9d0ce204993
Platform Version: 28.0
Firmware Version: V1.2-device.cfg

Comment 2

[Chris Kreinbring [:CKreinbring]]

Attachment: Log of unknown Bluetooth sender

Comment 3

[Sarah Parsons]

Last working:

Gaia   ec159aac19ff25912f1d68ffb44b29f797583ef5
SourceStamp 8b52fb24991d
BuildID 20140226132508
Version 30.0a1

First Broken: 

Gaia   c8d34e6e98d4b99921fda59ddd89f2dcdce201fc
SourceStamp bb9edb4d5144
BuildID 20140226133609
Version 30.0a1

Gecko/Gaia Swap: 

(first broken build with working Gaia) --FAILED

SourceStamp: bb9edb4d5144
Gaia: ec159aac19ff25912f1d68ffb44b29f797583ef5


(last working build with broken Gaia) -- PASSED

SourceStamp: 8b52fb24991d
Gaia: c8d34e6e98d4b99921fda59ddd89f2dcdce201fc


http://hg.mozilla.org//mozilla-central/pushloghtml?fromchange=8b52fb24991d&tochange=bb9edb4d5144

Comment 4

[Jason Smith [:jsmith]]

I can't follow the notation used in comment 3. Can you use the notation using Gaia & Gecko commits with last working gaia & first broken gecko here? Also, can you bisect this down on inbound builds?

Comment 5

[Sarah Parsons]

Here is the regression window for the inbound builds:

This issue started to occur on the inbound Buri 1.4 Build ID: 20140226085409

Gaia   9db2681f3fd49fba71666238dba4ee925eb9f89f
SourceStamp 0572e3c063ed
BuildID 20140226085409
Version 30.0a1

Last working Buri 1.4 inbound Build ID: 20140226075710

Gaia   9db2681f3fd49fba71666238dba4ee925eb9f89f
SourceStamp 04beead06aa7
BuildID 20140226075710
Version 30.0a1

http://hg.mozilla.org/integration/b2g-inbound/pushloghtml?fromchange=04beead06aa7&tochange=0572e3c063ed

Comment 6

[Ben Tian (inactive)]

Attachment: WIP fix: move accept() back into UnixSocket

The listening unix socket doesn't retrieve client address while accepting incoming connection [1], so bluetooth has no address to map the corresponding name. The address retrieval [2] was removed by bug 974410.

[1] http://dxr.mozilla.org/mozilla-central/source/ipc/unixfd/UnixSocketWatcher.cpp#101
[2] https://hg.mozilla.org/mozilla-central/rev/89c5263f9485#l1.634

Attached WIP fix moves the accept() call back into UnixSocket.cpp. Thomas, do you have any alternative to fix this bug?

Comment 7

[Ben Tian (inactive)]

Attachment: Patch 1: Move accept() call back to UnixSocket for address retrieval

Comment 8

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

Comment on attachment 8386597 [details] [diff] [review]
Patch 1: Move accept() call back to UnixSocket for address retrieval

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

Hi Ben, 

Thanks for taking care of this regression.

::: ipc/unixfd/UnixSocketWatcher.cpp
@@ +97,5 @@
>  
>    if (mConnectionStatus == SOCKET_IS_CONNECTED) {
>      OnSocketCanReceiveWithoutBlocking();
>    } else if (mConnectionStatus == SOCKET_IS_LISTENING) {
> +    OnAccept();

Please keep the call to |accept|here and pass the returned socket address and len to |OnAccepted|. You'll need to move |union sockaddr_any| to UnixSocketWatcher.h, but that's OK.

Comment 9

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

FYI: You might want to work on top of the patch in 979858.

Comment 10

[Ben Tian (inactive)]

Attachment: Patch 2: Pass socket address and length into OnAccepted

Rewrite patch and rebase on bug 979858.

Comment 11

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

Comment on attachment 8386637 [details] [diff] [review]
Patch 2: Pass socket address and length into OnAccepted

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

Just a few smaller things. Thanks a lot, Ben.

::: ipc/unixfd/UnixSocketWatcher.h
@@ +12,1 @@
>  #include "UnixFdWatcher.h"

You should move the other procol-specific include statements from UnixSocket.h here as well. Especially those protected by MOZ_B2G_BT_BLUEZ.

@@ +48,5 @@
>  
>    // Connect to a peer
>    nsresult Connect(const struct sockaddr* aAddr, socklen_t aAddrLen);
>  
> +  // Listen on socket for incoming connection requests

Good catch :D

::: ipc/unixsocket/UnixSocket.cpp
@@ +560,5 @@
>    MOZ_ASSERT(MessageLoopForIO::current() == GetIOLoop());
>    MOZ_ASSERT(GetConnectionStatus() == SOCKET_IS_LISTENING);
> +  MOZ_ASSERT(aAddr);
> +
> +  mAddr = *aAddr;

This should work, but might copy uninitialized bytes if mAddrLen is smaller than the size of aAddr. If we run valgrind on this line, it probably reports a false positive. I suggest to do

> MOZ_ASSERT(aAddrLen <= sizeof(mAddr));
> memcpy (&mAddr, aAddr, aAddrLen);

Comment 12

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

Comment on attachment 8386637 [details] [diff] [review]
Patch 2: Pass socket address and length into OnAccepted

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

Just a few smaller things. Thanks a lot, Ben.

::: ipc/unixfd/UnixSocketWatcher.cpp
@@ +98,5 @@
>    if (mConnectionStatus == SOCKET_IS_CONNECTED) {
>      OnSocketCanReceiveWithoutBlocking();
>    } else if (mConnectionStatus == SOCKET_IS_LISTENING) {
> +    sockaddr_any addr;
> +    socklen_t addrLen;

Almost forgot about this: addrLen needs to contain sizeof(addr). accept uses this value internally.

::: ipc/unixfd/UnixSocketWatcher.h
@@ +12,1 @@
>  #include "UnixFdWatcher.h"

You should move the other procol-specific include statements from UnixSocket.h here as well. Especially those protected by MOZ_B2G_BT_BLUEZ.

@@ +48,5 @@
>  
>    // Connect to a peer
>    nsresult Connect(const struct sockaddr* aAddr, socklen_t aAddrLen);
>  
> +  // Listen on socket for incoming connection requests

Good catch :D

::: ipc/unixsocket/UnixSocket.cpp
@@ +560,5 @@
>    MOZ_ASSERT(MessageLoopForIO::current() == GetIOLoop());
>    MOZ_ASSERT(GetConnectionStatus() == SOCKET_IS_LISTENING);
> +  MOZ_ASSERT(aAddr);
> +
> +  mAddr = *aAddr;

This should work, but might copy uninitialized bytes if mAddrLen is smaller than the size of aAddr. If we run valgrind on this line, it probably reports a false positive. I suggest to do

> MOZ_ASSERT(aAddrLen <= sizeof(mAddr));
> memcpy (&mAddr, aAddr, aAddrLen);

Comment 13

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

And another thing:

>    int fd = TEMP_FAILURE_RETRY(
>               accept(GetFd(), (struct sockaddr*)&addr, &addrLen));

You should use the reinterpret_cast<> from C++ here.

Comment 14

[Jason Smith [:jsmith]]

blocking+ - basic workflow with bluetooth not working. We might want to figure out if a backout of the regressing patch is a better path here, as that bug is not a blocker for the release that's causing a blocking regression.

Comment 15

[Ben Tian (inactive)]

Attachment: Patch 2 (v2): Pass socket address and length into OnAccepted, f=tzimmermann

Revise according to Thomas' suggestion.

Kyle, can you review my patch that passes socket address and length into |UnixSocket::OnAccepted| function?

Comment 16

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

Comment on attachment 8387346 [details] [diff] [review]
Patch 2 (v2): Pass socket address and length into OnAccepted, f=tzimmermann

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

Bouncing this back for feedback because it /looks/ like this could be cleaner but I might be missing something? If I'm wrong, just re-r? me, everything else looks fine.

::: ipc/unixfd/UnixSocketWatcher.h
@@ +32,5 @@
> +  sockaddr_rc rc;
> +  sockaddr_l2 l2;
> +#endif
> +  // ... others
> +};

Ok, if I'm reading this right, you should be able to push these definitions down into the .cpp file and make sockaddr_any an opaque struct. The only reason it's needed here is because of the type info required for UnixSocket, but all UnixSocket does is take the pointer and copy an amount of bytes equal to aAddrLen. It doesn't access any of the internals. So we should be able to keep all of this in the .cpp file and just opaquely declare the struct pointer, right?

Comment 17

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

(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #16)
> Comment on attachment 8387346 [details] [diff] [review]
> Patch 2 (v2): Pass socket address and length into OnAccepted, f=tzimmermann
> 
> Review of attachment 8387346 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Bouncing this back for feedback because it /looks/ like this could be
> cleaner but I might be missing something? If I'm wrong, just re-r? me,
> everything else looks fine.
> 
> ::: ipc/unixfd/UnixSocketWatcher.h
> @@ +32,5 @@
> > +  sockaddr_rc rc;
> > +  sockaddr_l2 l2;
> > +#endif
> > +  // ... others
> > +};
> 
> Ok, if I'm reading this right, you should be able to push these definitions
> down into the .cpp file and make sockaddr_any an opaque struct. The only
> reason it's needed here is because of the type info required for UnixSocket,
> but all UnixSocket does is take the pointer and copy an amount of bytes
> equal to aAddrLen. It doesn't access any of the internals. So we should be
> able to keep all of this in the .cpp file and just opaquely declare the
> struct pointer, right?

Hi Kyle,

I was thinking something similar, but there is still (at least) |UnixSocketConnector::GetSocketAddr| in UnixSocket.h, which takes sockaddr_any as argument.

I'd be in favor of removing struct sockaddr_any from the public interfaces, but maybe do this as a separate bug.

Comment 18

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

> I'd be in favor of removing struct sockaddr_any from the public interfaces,
> but maybe do this as a separate bug.

If someone opens a bug for this, feel free to assign it to me.

Comment 19

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

> I was thinking something similar, but there is still (at least)
> |UnixSocketConnector::GetSocketAddr| in UnixSocket.h, which takes
> sockaddr_any as argument.

And sockaddr_any is still stored in UnixSocketImpl, of course. We could also remove sockaddr_any completely and always call |getpeername| to retrieve the address of the socket's connected peer. That's certainly the cleanest solution and should free quite some* bytes per UnixSocketImpl. But this would still require changes to some of the interfaces.

* 64 bytes for sockaddr_any plus sizeof(nAddrLen)

Comment 20

[Ben Tian (inactive)]

Comment on attachment 8387346 [details] [diff] [review]
Patch 2 (v2): Pass socket address and length into OnAccepted, f=tzimmermann

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #17)
> I'd be in favor of removing struct sockaddr_any from the public interfaces,
> but maybe do this as a separate bug.
Agree. As both |UnixSocketConnector::GetSocketAddr| and |UnixSocketConnector::CreateAddr| still access the internals of sockaddr_any, its definition in UnixSocketWatcher.h is required. Moving the definition into .cpp or calling |getpeername| may be feasible but I prefer to do this in another bug.

Request r? again.

Comment 21

[Ben Tian (inactive)]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #18)
> > I'd be in favor of removing struct sockaddr_any from the public interfaces,
> > but maybe do this as a separate bug.
> 
> If someone opens a bug for this, feel free to assign it to me.
Opened bug 981467 to track.

Comment 22

[Ben Tian (inactive)]

try server: https://tbpl.mozilla.org/?tree=Try&rev=6f9a9c5037a0

Comment 23

[Ben Tian (inactive)]

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

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a149c02682ef

Comment 25

[Josh Schmitt [Joshs]]

Verified fixed for 1.4 Buri. Verified fixed for 1.4 and 2.0 Open C.

The senders name is shown in both the notifications and the message.

Buri:
1.4 Environmental Variables:
Device: Buri 1.4 MOZ
BuildID: 20140502000201
Gaia: 7b2b82d72cbdd1c7e0f4542cb3390802e65f473e
Gecko: 50be03cea340
Version: 30.0
Firmware Version: v1.2-device.cfg

Open_C:
1.4 Environmental Variables:
Device: Open_C 1.4
BuildID: 20140502000201
Gaia: 7b2b82d72cbdd1c7e0f4542cb3390802e65f473e
Gecko: 50be03cea340
Version: 30.0
Firmware Version: P821A10-ENG_20140410

Open_C:
2.0 Environmental Variables:
Device: Open_C 2.0
BuildID: 20140502040202
Gaia: 386b5478eb9c3970972966123517e993e8a1092a
Gecko: e2e1b19fcffc
Version: 32.0a1
Firmware Version: P821A10-ENG_20140410