823803 - [Open_][bluetooth] Bluetooth file which The test machine to accept not be able to view, when accept completed-617001924405

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect, P2)

Description

[zhangwei]

Attachment: 1212200911logs.zip

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:16.0) Gecko/16.0 Firefox/16.0
Build ID: 20120625030537

Steps to reproduce:

1、The test machine to accept Bluetooth file and view;
2、The test machine sending a Bluetooth File to another machine;

build infomation:
gecko:   	 revision="3cbade1974968bb1e0fbb0c3386239715244a7a7"
gaia: 	 	 revision="aab72f365d73f624ede32b522f27d072c409e42e"
gonk-misc:   revision="654358494ba601a46ef9838debc95417ae464cc6"
dalvik:      revision="ca1f327d5acc198bb4be62fa51db2c039032c9ce"
librecovery: revision="e1bd90051c9e937221eb1f91c94e3cde747311a7"
moztt:       revision="6ee1f8987ef36d688f97064c003ad57849dfadf2"
external/jsmin:    revision="cec896f0affaa0226c02605ad28d42df1bc0e393"
external/opensans: revision="b5b4c226ca1d71e936153cf679dda6d3d60e2354"
device/qcom/b2g_common/mozilla-b2g: revision="41c17a6abfd5f488ec99d9aa246f5b07583403c7"


Actual results:

1、Bluetooth file which The test machine to accept not be able to view, when accept completed, the accept file can not be found in the gallery.
2、when the test machine send Bluetooth file, the receiving machine does not have any reaction.

the /data/local/logs/ is show in the Annex.

Comment 1

[Walter Chen[:ypwalter][:wachen]]

Hi,
Please try again with a newer version.
Build ID: 20120625030537 means a build about half a year ago.

We have daily build for B2g cell phones.
I don't think that this bug could be reproduce anymore in new version.

Comment 2

[Firefox_Mozilla]

we try again. In the new build version, files such as pictures can be sent by Firefox OS device and received by Android OS device. But in reverse, when the files such as pictures is sent by the Android OS  device, the Firefox OS device can not receive. The Android OS device shown the sent failure. 
The Android OS device which we use is V790.
When we use another Android device of MTK, Firefox OS receive file successful, but send file failure.
we add attachment above.

Comment 3

[Firefox_Mozilla]

Attachment: Paired with V790

Comment 4

[Firefox_Mozilla]

Attachment: Paired with MTK device.zip

Comment 5

[Kai-Chih Hu [:khu]]

It looks like it could be reproduced by the reporter. 
Reopen it.

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to khu from comment #5)
> It looks like it could be reproduced by the reporter. 
> Reopen it.

Hey Kevin do you know if that's a platform issue or a gaia issue?

Comment 7

[Kai-Chih Hu [:khu]]

(In reply to Vivien Nicolas (:vingtetun) from comment #6)
> (In reply to khu from comment #5)
> > It looks like it could be reproduced by the reporter. 
> > Reopen it.
> 
> Hey Kevin do you know if that's a platform issue or a gaia issue?

Good point. I am not sure, but I heard that Gina could work on similar issue. Not sure if Gina could help to clarify?

Comment 8

[Tony Chung [:tchung]]

what kind of file type is being transferred in comment 0?   can you attach that test file here so others can try and reproduce?

Comment 9

[Eric Chou [:ericchou] [:echou]]

Hi zhangwei,

(In reply to zhangwei from comment #0)
> Actual results:
> 
> 1、Bluetooth file which The test machine to accept not be able to view, when
> accept completed, the accept file can not be found in the gallery.

It looks like that the picture was received correctly but not shown. I think it's because the picture is too large to be processed in Gallery app. We have size limit for viewing a photo in Gallery, once the file exceeds the limit, you would be unable to see neither the picture nor the thumbnail.

Please see bug 815079 for more information. (bug 815079 comment 4, bug 815079 comment 22)

> 2、when the test machine send Bluetooth file, the receiving machine does not
> have any reaction.

Works for me in this case.

Eric

Comment 10

[Gina Yeh [:gyeh] [:ginayeh]]

For the Android device of MTK case, can you try to pair with that device via Settings app first? The error message seems like you haven't pair with that device before.

For the V790 case, we need more information. Please see https://bugzilla.mozilla.org/show_bug.cgi?id=828518#c1 for more information to get hcidump.

Comment 11

[Joe Cheng [:jcheng] (please needinfo)]

to general after discussing with echo

Comment 12

[Joe Cheng [:jcheng] (please needinfo)]

QAWANTED with new build. per triage this morning, issue reporter is using 01/04 build

Comment 13

[Joe Cheng [:jcheng] (please needinfo)]

tested with Andrew in person. no problem sending and receiving file with Android device.
BB- until we have more firm information on the reproducibility of this issue.

Comment 14

[Firefox_Mozilla]

Attachment: hcidump log for paired with V790

Comment 15

[Firefox_Mozilla]

The hcidump log for paired with V790 is uploaded.
For the Android device of MTK case, we first paired with MTK Phone in settings. Then Go to Gallery to transfer images.

Comment 16

[Joe Cheng [:jcheng] (please needinfo)]

Triage: Gina, can you please take a look at the log attached in comment 14? does it help in the investigation?

issue reporter: can you please provide the commercial model names of the device you are having problems with? will see if they can be obtained?
and also, can you try more android devices? since it works fine with Android devices that most Moz people has access to.

Comment 17

[Gina Yeh [:gyeh] [:ginayeh]]

From the attached hcidump log, it seems like something related to "OPP over L2cap feature" which is not supported in V1. Will give more details after discussing with Eric and Shawn.

Comment 18

[Eric Chou [:ericchou] [:echou]]

I just checked the hcidump log provided by [e-mail] in comment 14. The main problem is that V790 requested to connect to an invalid psm "5255":

2013-01-17 12:14:52.602834 > ACL data: handle 5 flags 0x02 dlen 12
    L2CAP(s): Connect req: psm 5255 scid 0x0041

2013-01-17 12:14:52.602956 < ACL data: handle 5 flags 0x00 dlen 16
    L2CAP(s): Connect rsp: dcid 0x0000 scid 0x0041 result 2 status 0
      Connection refused - PSM not supported

Coincidentally, the hex value of 5255 is 0x1487, and if you search "0x1487" in the hcidump log, you would notice that there is one matched result:

          record #6
              aid 0x0000 (SrvRecHndl)
                 uint 0x10007
              aid 0x0001 (SrvClassIDList)
                 < uuid-16 0x1105 (OBEXObjPush) >
              aid 0x0004 (ProtocolDescList)
                 < < uuid-16 0x0100 (L2CAP) > <
                 uuid-16 0x0003 (RFCOMM) uint 0xc > <
                 uuid-16 0x0008 (OBEX) > >
              aid 0x0005 (BrwGrpList)
                 < uuid-16 0x1002 (PubBrwsGrp) >
              aid 0x0009 (BTProfileDescList)
                 < < uuid-16 0x1105 (OBEXObjPush) uint 0x102 > >
              aid 0x0100 (SrvName)
                 str "OBEX Object Push"
              aid 0x0200 (VersionNumList)
                 uint 0x1487  ==========================================> !!!!!
              aid 0x0303 (SuppFormatsList)
                 < uint 0x1 uint 0x2 uint 0xff >

This is an SDP record of ObjectPush, letting remote devices know which channel/PSM should be connected with if they want to transfer files via OPP. I was wondering if there is any possibilities that V790 has actually got a wrong attribute and used it as PSM. [e-mail], could you check if this part of code of V790 is right?

Please feel free to let me know if you have any questions. Thank you.

Eric

Comment 19

[Firefox_Mozilla]

> I was wondering if there is any possibilities that V790 has actually got a
> wrong attribute and used it as PSM. 

I don't think PSM 5525 is a wrong attribute. We have check the hci log with android devices,
PSM 5525 is also used under android system, and can be support well. 

I'm not sure whether it is added by qualcomm, so I will confirm with qualcomm. thanks.

Comment 20

[Nick Davidson]

issue still repro with Unagi
Build ID: 20130208070202
Kernel: Dec 5
Gecko  http://hg.mozilla.org/releases/mozilla-b2g18/rev/4518b78583d0
Gaia   7e54ca673277b20b1d91d18477dc44d6ad226761

Able to send Pic files to Samsung galaxy S and SII but sending pics from those devises via bluetooth to Unagi failed.

Comment 21

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Can you close out of gallery and try this again?

I believe you are hitting the same issue as bug 838898.

If you close off gallery (via long tapping and then hitting the x for gallery) and then try the bluetooth and it opens in gallery... I believe you are hitting bug 836783.

If gallery is closed, I am not running into this issue.

Comment 22

[Firefox_Mozilla]

When we close the Gallery, it also can not receive the image which sent by V790 via Bluetooth.
We choice an image for V790 and send it via Bluetooth. It shown "sending failure" in V790 and Mozilla Phone does not receive any hit.

Comment 23

[Firefox_Mozilla]

(In reply to Gina Yeh from comment #10)
> For the Android device of MTK case, can you try to pair with that device via
> Settings app first? The error message seems like you haven't pair with that
> device before.

We had paired with MTK device in settings first. Then do other BT operation.

Comment 24

[Joe Cheng [:jcheng] (please needinfo)]

per partner, v790 Android -> v790 Firefox OS will observe such problem

Comment 26

[Lucas Adamski [:ladamski]]

Gina, since this is now a TEF+ bug can you please take a look?

Comment 27

[Eric Chou [:ericchou] [:echou]]

I'll take a look.

Comment 28

[Eric Chou [:ericchou] [:echou]]

I've got a solution, but bug 843868 has to be fixed first. Therefore mark this issue depending on bug 843868.

Comment 29

[Eric Chou [:ericchou] [:echou]]

When Gecko Bluetooth calls "AddReservedService" to install Object Push Profile(OPP) service record, current BlueZ version (from Code Aurora) would register OPP service with version 1.2 and a attribute "GoepL2capPsm". The value of GoepL2capPsm is fixed: 5255. This is just like telling other devices that "B2G can receive file via L2CAP". However, we never create a L2CAP server socket listening to port 5255 in Gecko. Therefore any L2CAP connection requests would be rejected.

To solve this, first, we need to decouple BluetoothOppManager and UnixSocketConsumer then add L2CAP socket creation related code.

Comment 30

[Kai-Chih Hu [:khu]]

Hi, Eric, it looks like this case depends on bug 851046 which is under review. Does it mean that once bug 851046 is landed, this case could be resolved as well? Or, there has other additional efforts we need to pay after bug 851046 is landed? 

It looks like there are 2 steps: 
1. To decouple BluetoothOppManager and UnixSocketConsumer. 
2. To add L2CAP socket creation related code. 

I don't know if bug 851046 covers both steps. Thanks!

Comment 31

[Eric Chou [:ericchou] [:echou]]

(In reply to khu from comment #30)
> Hi, Eric, it looks like this case depends on bug 851046 which is under
> review. Does it mean that once bug 851046 is landed, this case could be
> resolved as well? Or, there has other additional efforts we need to pay
> after bug 851046 is landed? 
> 
> It looks like there are 2 steps: 
> 1. To decouple BluetoothOppManager and UnixSocketConsumer. 
> 2. To add L2CAP socket creation related code. 
> 

Yes, you're right. Step 1 would be done once Bug 851046 lands, and Blake will review Bug 851046 some time this week. I'll send another patch to this bug based on BluetoothSocket implementation to finish step 2.

Comment 32

[Dietrich Ayala (:dietrich)]

Current status is BT team are figuring out how to land bug 851046, and then tackling this piece.

Comment 33

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 1: v1: completes L2CAP/EL2CAP socket implementation

* This version of BlueZ from Code Aurora has added GOEP_PSM to Object Push Profile service record, which means that remote devices may connect with us via both L2CAP and RFCOMM. This patch completes L2CAP/EL2CAP socket implementation.

Comment 34

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 2: v1: Manage two server sockets (RFCOMM/EL2CAP) in BluetoothOppManager

* In order to solve the problem, BluetoothOppManager should maintain two server sockets at the same time, one is RFCOMM socket, another is EL2CAP socket. I've tested with V790/Galaxy Tab 7/HTC Sensation XL and it worked well.

Blake, please help me with making sure that the ownership-related implementation is correct.
Gina, please check if the modification of BluetoothOppManager logic is correct.

Thanks in advance.

Comment 35

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 735062 [details] [diff] [review]
patch 2: v1: Manage two server sockets (RFCOMM/EL2CAP)  in BluetoothOppManager

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

It is important to remember that for an nsRefPtr<T> foo;

foo = nullptr; // drops the reference and doesn't leak
foo.forget(); // doesn't touch the reference count and will leak unless there's other code that's expecting it.

::: dom/bluetooth/BluetoothOppManager.cpp
@@ +262,5 @@
>    }
>  
> +  // Stop listening because currently we only support one connection at a time.
> +  mRfcommSocket->Disconnect();
> +  mRfcommSocket.forget();

Here, and every other time you call forget, you should replace the call with a set to null. This leaks the sockets in question.

@@ +336,5 @@
> +      new BluetoothSocket(this, BluetoothSocketType::EL2CAP, true, true);
> +
> +    if (!mL2capSocket->Listen(BluetoothReservedChannels::CHANNEL_OPUSH_L2CAP)) {
> +      NS_WARNING("[OPP] Can't listen on L2CAP socket!");
> +      mL2capSocket.forget();

Should you also be nulling out mRfcommSocket if this fails?

@@ +1328,5 @@
> +   * As for outbound connections, we do nothing since sockets have been already
> +   * handled in function Connect().
> +   */
> +  if (aSocket == mRfcommSocket) {
> +    mL2capSocket->Disconnect();

Can we assert !mSocket here?

@@ +1330,5 @@
> +   */
> +  if (aSocket == mRfcommSocket) {
> +    mL2capSocket->Disconnect();
> +
> +    mRfcommSocket.forget();

You should set mSocket to mRfcommSocket here before you drop your reference (this needs to not be a forget() call).

If we wanted to be clever, we *could* do mRfcommSocket.swap(mSocket) (relying on the assertion that mSocket is null).

@@ +1335,5 @@
> +    mL2capSocket.forget();
> +  } else if (aSocket == mL2capSocket) {
> +    mRfcommSocket->Disconnect();
> +
> +    mRfcommSocket.forget();

Ditto as for the above case.

@@ +1396,5 @@
>      if (!mSuccessFlag) {
> +      DeleteReceivedFile();
> +    } else if (mDsFile) {
> +      nsString data;
> +      CopyASCIItoUTF16("modified", data);

This should be: NS_NAMED_LITERAL_STRING(data, "modified");

::: dom/bluetooth/BluetoothOppManager.h
@@ +183,5 @@
>    nsRefPtr<BluetoothReplyRunnable> mRunnable;
>    nsRefPtr<DeviceStorageFile> mDsFile;
> +
> +  // If a connection has been established, mSocket will be the socket
> +  // communicating with the remote socket.

I'd add that we maintain the invariant that if mSocket is non-null mRfcommSocket and mL2capSocket must be null (and vice versa).

@@ +188,2 @@
>    nsRefPtr<BluetoothSocket> mSocket;
> +  

Super-nit: nuke trailing whitespace.

Comment 36

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 2: v2: Manage two server sockets (RFCOMM/EL2CAP) in BluetoothOppManager

* Fixed all problems based on Blake's review comment.

Comment 37

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 736117 [details] [diff] [review]
patch 2: v2: Manage two server sockets (RFCOMM/EL2CAP) in BluetoothOppManager

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

This looks much better. I have one more question, though.

::: dom/bluetooth/BluetoothOppManager.cpp
@@ +261,5 @@
>      return false;
>    }
>  
> +  MOZ_ASSERT(mRfcommSocket);
> +  MOZ_ASSERT(mL2capSocket);

What happens if Listen() had already failed? It seems like we'll crash in here in that case. Do we need to retry or send an event somewhere if that happens?

Comment 38

[Eric Chou [:ericchou] [:echou]]

(In reply to Blake Kaplan (:mrbkap) from comment #37)
> Comment on attachment 736117 [details] [diff] [review]
> patch 2: v2: Manage two server sockets (RFCOMM/EL2CAP) in BluetoothOppManager
> 
> Review of attachment 736117 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks much better. I have one more question, though.
> 
> ::: dom/bluetooth/BluetoothOppManager.cpp
> @@ +261,5 @@
> >      return false;
> >    }
> >  
> > +  MOZ_ASSERT(mRfcommSocket);
> > +  MOZ_ASSERT(mL2capSocket);
> 
> What happens if Listen() had already failed? It seems like we'll crash in
> here in that case. Do we need to retry or send an event somewhere if that
> happens?

Good point. So, we should still be able to do Connect() even if Listen() failed, which means that we can just check if mRfcommSocket/mL2capSocket is null before using them.

Comment 39

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 2: v3: Manage two server sockets (RFCOMM/EL2CAP) in BluetoothOppManager

* Check if mRfcommSocket/mL2capSocket is null before using them in Connect()

Comment 40

[Gina Yeh [:gyeh] [:ginayeh]]

Comment on attachment 735047 [details] [diff] [review]
patch 1: v1: completes L2CAP/EL2CAP socket implementation

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

Looks good.

Comment 41

[Gina Yeh [:gyeh] [:ginayeh]]

Comment on attachment 736631 [details] [diff] [review]
patch 2: v3: Manage two server sockets (RFCOMM/EL2CAP) in BluetoothOppManager

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

Thanks for your patience. No big problem and just some suggestions for you.

Btw, wwe don't need mPrevSocketStatus in class BluetoothOppManager, right? Did I miss something?

::: dom/bluetooth/BluetoothOppManager.cpp
@@ +1424,2 @@
>    }
>  

Suggestion: The logic here is kind of hard to understand. How about putting codes which are dealing with transfer failure together in one if block? I think it would increase the readability here.

Ex.
// When the transfer is failed, broadcast a system message and delete the in-complete file if we were receiving a file. On the contrary, we notify file-watcher that the file has been successfully created.
if (!mSuccessFlag) {
  if (mTransferMode) {
    DeleteReceivedFile();
  }
  FileTransferComplete();
} else {
  if (mTransferMode && mDsFile) {
    ......
  }
}

Comment 42

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 736631 [details] [diff] [review]
patch 2: v3: Manage two server sockets (RFCOMM/EL2CAP) in BluetoothOppManager

Looks good, thanks!

Comment 43

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 1: final: completes L2CAP/EL2CAP socket implementation, r=qdot, r=gyeh

Comment 44

[Eric Chou [:ericchou] [:echou]]

Attachment: patch 2: final: Manage two server sockets (RFCOMM/EL2CAP) in BluetoothOppManager, r=mrbkap, r=gyeh

* Removed mPrevSocketStatus from header file and re-organize the content of OnDisconnect() based on Gina's review comment.

Comment 45

[Eric Chou [:ericchou] [:echou]]

try: https://tbpl.mozilla.org/?tree=Try&rev=2d7da639b299

Comment 46

[Eric Chou [:ericchou] [:echou]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c41e47d57607
https://hg.mozilla.org/integration/mozilla-inbound/rev/0984b5e72dc8

Comment 47

[Eric Chou [:ericchou] [:echou]]

Attachment: (b2g18) patch 1: final

* Please use this patch as the patch 1 for b2g18. Patch 2 should just work.

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c41e47d57607
https://hg.mozilla.org/mozilla-central/rev/0984b5e72dc8

Comment 49

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/19abeedc86d6
https://hg.mozilla.org/releases/mozilla-b2g18/rev/4b707b7fb5fe

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/c0731d4fa61d
https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/b88d456b44e8

Comment 50

[Jeni]

Verified fixed on 

Unagi Build ID: 20130429070204
Kernel Date: Feb 21
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/45aa5ba0ed53
Gaia: cf2d4136f0ebc66039637fdbeb72ed184dfbc0f2

Issue repros on 

Leo Build ID: 20130426070204
Kernel Date: Mar 15
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/6c2493de1441
Gaia: c9046a7acef33328977840176ff5574720d2c74c