The root cause is that we receive size=20 data directly, instead of receiving the 1st message (size=4) first then the 2nd message (size=16). On AOSP or CAF platform, we received the message info separately, this is why we don't see this problem. On woodduck, we received 20 bytes directly from stack, and sometimes we got 4 and 16 messages separately. The result is inconsistent. Android is fine, because they read it from InputStream, but Gecko reads from libevent callback, whenever there is an incoming data. Since this is no clean interface defined this behavior, I think we can handle inside Gecko.

per discussion, 2.0m+

Comment on attachment 8494466 [details] [diff] [review] Bug 1072142 - [woodduck] Read the 2nd socket message info directly (master) Review of attachment 8494466 [details] [diff] [review]: ----------------------------------------------------------------- r- for now, because I don't understand why the patch works. Shawn, can you give some more infos? ::: dom/bluetooth/bluedroid/BluetoothSocketHALInterface.cpp @@ +158,5 @@ > case MSG1_SIZE: > status = RecvMsg2(); > break; > + case TOTAL_MSG_SIZE: > + status = RecvMsg2(); I don't understand this case. |mLen| is actually a misnomer, as it actually contains the current read offset in the buffer. That means we'd take this case after having processed 20 bytes already. Bu t |IsComplete| below should be true at 20 bytes, and we'd be stopping.

Just a question for my understanding: on Woodduck, we receive one single message that includes 20 bytes of data + the file descriptor?

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #4) > Comment on attachment 8494466 [details] [diff] [review] > Bug 1072142 - [woodduck] Read the 2nd socket message info directly (master) > > Review of attachment 8494466 [details] [diff] [review]: > ----------------------------------------------------------------- > > r- for now, because I don't understand why the patch works. Shawn, can you > give some more infos? > > ::: dom/bluetooth/bluedroid/BluetoothSocketHALInterface.cpp > @@ +158,5 @@ > > case MSG1_SIZE: > > status = RecvMsg2(); > > break; > > + case TOTAL_MSG_SIZE: > > + status = RecvMsg2(); > > I don't understand this case. |mLen| is actually a misnomer, as it actually > contains the current read offset in the buffer. That means we'd take this > case after having processed 20 bytes already. Bu t |IsComplete| below should > be true at 20 bytes, and we'd be stopping. Opps. It looks like my patch is wrong. Let me update it again.

I think a possible approach could be to fill the 20-byte buffer with each read and get the file descriptor. When the complete message has been read, continue by calling |Proceed|.

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #5) > Just a question for my understanding: on Woodduck, we receive one single > message that includes 20 bytes of data + the file descriptor? Yes. On woodduck, the stack is different but follows BT HAL interface. What I saw on woodduck is that we received 20 bytes of data directly. Android read the 1st channel http://androidxref.com/4.4.4_r1/xref/frameworks/base/core/java/android/bluetooth/BluetoothSocket.java#320 Then wait for the 2nd message incoming, read 16 bytes http://androidxref.com/4.4.4_r1/xref/frameworks/base/core/java/android/bluetooth/BluetoothSocket.java#324 Android is fine and without any problem with this stack, because they read it from InputStream (which reads from buffer, but Gecko reads from libevent callback, whenever there is an incoming data.) http://androidxref.com/4.4.4_r1/xref/frameworks/base/core/java/android/bluetooth/BluetoothSocket.java#480

Comment on attachment 8494459 [details] [diff] [review] Bug 1072142 - [woodduck] Read the 2nd socket message info directly (v2.0M) Review of attachment 8494459 [details] [diff] [review]: ----------------------------------------------------------------- f=me with comment addressed. ::: dom/bluetooth/bluedroid/BluetoothSocket.cpp @@ +752,5 @@ > // If this is server socket, read header of next message for client fd > mImpl->mReadMsgForClientFd = mIsServer; > } else if (mReceivedSocketInfoLength == TOTAL_SOCKET_INFO_LENGTH) { > + if (aMessage->mSize == TOTAL_SOCKET_INFO_LENGTH) { > + // Some bluetooth stack might send total socket info length (20) directly nit: revise comment as following: // Some bluetooth stacks send socket info as a single // 20-byte message so we read from last 16 bytes only. @@ +754,5 @@ > } else if (mReceivedSocketInfoLength == TOTAL_SOCKET_INFO_LENGTH) { > + if (aMessage->mSize == TOTAL_SOCKET_INFO_LENGTH) { > + // Some bluetooth stack might send total socket info length (20) directly > + // so we only need to read from the 2nd message > + offset = 4; offset = FIRST_SOCKET_INFO_MSG_LENGTH; @@ +755,5 @@ > + if (aMessage->mSize == TOTAL_SOCKET_INFO_LENGTH) { > + // Some bluetooth stack might send total socket info length (20) directly > + // so we only need to read from the 2nd message > + offset = 4; > + } nit: newline here.

Since this patch is to fix the different behavior of blueangel stack and blocks so many test cases. I prefer to land on V2.0M first and let partner test further. I'm currently have some problems to make master codebase work on woodduck now.

Dear Shawn, Really appreciate your help on this bug, i know you have put much effort fixing this. Since it blocks many test cases and partner is asking when can we fix this. Is it possible for you to give an ETA landing date? Thank you very much!

We can land on v2.0 M first?

(In reply to Josh Cheng from comment #14) > Dear Shawn, > > Really appreciate your help on this bug, i know you have put much effort > fixing this. Since it blocks many test cases and partner is asking when can > we fix this. Is it possible for you to give an ETA landing date? > > Thank you very much! Already r+, so we need to wait for 2.0M sheriff merge this patch into 2.0M branch.

(In reply to Shawn Huang [:shawnjohnjr] from comment #16) > (In reply to Josh Cheng from comment #14) > > Dear Shawn, > > > > Really appreciate your help on this bug, i know you have put much effort > > fixing this. Since it blocks many test cases and partner is asking when can > > we fix this. Is it possible for you to give an ETA landing date? > > > > Thank you very much! > Already r+, so we need to wait for 2.0M sheriff merge this patch into 2.0M > branch. Hi Shawn, Understood! Thank you very much!

There will be another bug to follow this issue for master. Merge into 2.0m only. http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/ffb2fa8340f3

Add tag in whiteboard to track specific platform dependency bug.