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

RESOLVED FIXED in Firefox 23

Status

defect
P2
normal
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: zhang.wei38, Assigned: echou)

Tracking

unspecified
B2G C4 (2jan on)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:tef+, blocking-basecamp:-, firefox21 wontfix, firefox22 wontfix, firefox23 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 verified)

Details

Attachments

(7 attachments, 4 obsolete attachments)

3.12 MB, text/plain
Details
1.08 MB, text/plain
Details
1.13 MB, text/plain
Details
430.88 KB, text/plain
Details
7.57 KB, patch
Details | Diff | Splinter Review
9.32 KB, patch
Details | Diff | Splinter Review
10.16 KB, patch
Details | Diff | Splinter Review
Posted file 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.
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.
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
OS: All → Gonk (Firefox OS)
Hardware: All → ARM
Resolution: --- → INVALID
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.
Posted file Paired with V790
It looks like it could be reproduced by the reporter. 
Reopen it.
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: INVALID → ---
Assignee: nobody → gyeh
blocking-basecamp: --- → ?
(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?
(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?
blocking-basecamp: ? → +
Priority: -- → P2
Target Milestone: --- → B2G C4 (2jan on)
what kind of file type is being transferred in comment 0?   can you attach that test file here so others can try and reproduce?
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
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.
Flags: needinfo?(Firefox_Mozilla)
to general after discussing with echo
Component: Gaia::Bluetooth File Transfer → General
QA Contact: wachen
QAWANTED with new build. per triage this morning, issue reporter is using 01/04 build
Keywords: qawanted
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.
blocking-basecamp: + → -
Flags: needinfo?(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.
QA Contact: wachen
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.
QA Contact: wachen
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.
I just checked the hcidump log provided by Firefox_Mozilla@126.com 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. Firefox_Mozilla@126.com, 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
Flags: needinfo?(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.
Flags: needinfo?(Firefox_Mozilla)
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.
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.
Flags: needinfo?(zhang.wei38)
Keywords: qawanted
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.
(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.
QA Contact: atsai
per partner, v790 Android -> v790 Firefox OS will observe such problem
blocking-b2g: --- → tef?
Blocks: 849704
blocking-b2g: tef? → tef+
Duplicate of this bug: 849704
Gina, since this is now a TEF+ bug can you please take a look?
Flags: needinfo?(gyeh)
I'll take a look.
Assignee: gyeh → echou
Flags: needinfo?(gyeh)
I've got a solution, but bug 843868 has to be fixed first. Therefore mark this issue depending on bug 843868.
Depends on: 843868
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.
Depends on: 851046
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!
Flags: needinfo?(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.
Flags: needinfo?(echou)
Current status is BT team are figuring out how to land bug 851046, and then tackling this piece.
* 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.
Attachment #735047 - Flags: review?(kyle)
Attachment #735047 - Flags: review?(gyeh)
* 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.
Attachment #735062 - Flags: review?(mrbkap)
Attachment #735062 - Flags: review?(gyeh)
Component: General → Bluetooth
Attachment #735047 - Flags: review?(kyle) → review+
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.
Attachment #735062 - Flags: review?(mrbkap) → review-
* Fixed all problems based on Blake's review comment.
Attachment #735062 - Attachment is obsolete: true
Attachment #735062 - Flags: review?(gyeh)
Attachment #736117 - Flags: review?(mrbkap)
Attachment #736117 - Flags: review?(gyeh)
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?
Attachment #736117 - Flags: review?(mrbkap)
(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.
* Check if mRfcommSocket/mL2capSocket is null before using them in Connect()
Attachment #736117 - Attachment is obsolete: true
Attachment #736117 - Flags: review?(gyeh)
Attachment #736631 - Flags: review?(mrbkap)
Attachment #736631 - Flags: review?(gyeh)
Comment on attachment 735047 [details] [diff] [review]
patch 1: v1: completes L2CAP/EL2CAP socket implementation

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

Looks good.
Attachment #735047 - Flags: review?(gyeh) → review+
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) {
    ......
  }
}
Attachment #736631 - Flags: review?(gyeh) → review+
Comment on attachment 736631 [details] [diff] [review]
patch 2: v3: Manage two server sockets (RFCOMM/EL2CAP) in BluetoothOppManager

Looks good, thanks!
Attachment #736631 - Flags: review?(mrbkap) → review+
* Removed mPrevSocketStatus from header file and re-organize the content of OnDisconnect() based on Gina's review comment.
Attachment #736631 - Attachment is obsolete: true
* Please use this patch as the patch 1 for b2g18. Patch 2 should just work.
https://hg.mozilla.org/mozilla-central/rev/c41e47d57607
https://hg.mozilla.org/mozilla-central/rev/0984b5e72dc8
Status: REOPENED → RESOLVED
Closed: 7 years ago6 years ago
Resolution: --- → FIXED
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
Flags: needinfo?(zhang.wei38)
You need to log in before you can comment on or make changes to this bug.