Closed Bug 979668 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.4+, firefox28 wontfix, firefox29 wontfix, firefox30 verified, b2g-v1.4 verified)

VERIFIED FIXED
1.4 S3 (14mar)
blocking-b2g 1.4+
Tracking Status
firefox28 --- wontfix
firefox29 --- wontfix
firefox30 --- verified
b2g-v1.4 --- verified

People

(Reporter: ckreinbring, Assigned: ben.tian)

References

Details

(Keywords: regression, Whiteboard: burirun1.4-1)

Attachments

(3 files, 3 obsolete files)

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
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
blocking-b2g: --- → 1.4?
Component: Gaia::Bluetooth File Transfer → Bluetooth
QA Contact: sparsons
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
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?
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
Blocks: 974410
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?
Flags: needinfo?(tzimmermann)
Assignee: nobody → btian
Attachment #8386585 - Attachment is obsolete: true
Attachment #8386597 - Flags: feedback?(tzimmermann)
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.
Attachment #8386597 - Flags: feedback?(tzimmermann) → feedback-
Flags: needinfo?(tzimmermann)
FYI: You might want to work on top of the patch in 979858.
Rewrite patch and rebase on bug 979858.
Attachment #8386597 - Attachment is obsolete: true
Attachment #8386637 - Flags: feedback?(tzimmermann)
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);
Attachment #8386637 - Flags: feedback?(tzimmermann) → feedback+
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);
And another thing:

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

You should use the reinterpret_cast<> from C++ here.
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.
blocking-b2g: 1.4? → 1.4+
Revise according to Thomas' suggestion.

Kyle, can you review my patch that passes socket address and length into |UnixSocket::OnAccepted| function?
Attachment #8386637 - Attachment is obsolete: true
Attachment #8387346 - Flags: review?(kyle)
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?
Attachment #8387346 - Flags: review?(kyle)
Attachment #8387346 - Flags: feedback?(tzimmermann)
Attachment #8387346 - Flags: feedback?(btian)
(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.
> 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.
Flags: needinfo?(btian)
> 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 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.
Attachment #8387346 - Flags: review?(kyle)
Attachment #8387346 - Flags: feedback?(tzimmermann)
Attachment #8387346 - Flags: feedback?(btian)
Flags: needinfo?(btian)
Attachment #8387346 - Flags: review?(kyle) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/a149c02682ef
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.4 S3 (14mar)
Keywords: verifyme
QA Contact: sparsons → jschmitt
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
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.