Closed Bug 1072142 Opened 10 years ago Closed 10 years ago

[woodduck] Read the 2nd socket message info directly

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:2.0M+, b2g-v2.0 unaffected, b2g-v2.0M fixed, b2g-v2.1 unaffected, b2g-v2.2 unaffected)

RESOLVED FIXED
2.1 S6 (10oct)
blocking-b2g 2.0M+
Tracking Status
b2g-v2.0 --- unaffected
b2g-v2.0M --- fixed
b2g-v2.1 --- unaffected
b2g-v2.2 --- unaffected

People

(Reporter: shawnjohnjr, Assigned: shawnjohnjr)

References

Details

(Whiteboard: [2.0M_only][blueangel])

Attachments

(1 file, 3 obsolete files)

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.
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → ARM
Assignee: nobody → shuang
blocking-b2g: --- → 2.0M?
Attachment #8494459 - Attachment description: bug1072142.patch → Bug 1072142 - [woodduck] Read the 2nd socket message info directly (v2.0M)
Attachment #8494466 - Attachment description: Bug 1072142 - [woodduck] Read the 2nd socket message info directly → Bug 1072142 - [woodduck] Read the 2nd socket message info directly (master)
per discussion, 2.0m+
blocking-b2g: 2.0M? → 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.
Attachment #8494466 - Flags: review?(tzimmermann) → review-
Just a question for my understanding: on Woodduck, we receive one single message that includes 20 bytes of data + the file descriptor?
Whiteboard: [partner-blocker]
Attachment #8494466 - Attachment is obsolete: true
Attachment #8494466 - Flags: feedback?(btian)
(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.
Attachment #8494459 - Flags: feedback?(btian) → feedback+
Attachment #8494459 - Attachment is obsolete: true
Attachment #8494459 - Flags: review?(tzimmermann)
Attachment #8495794 - Flags: review?(tzimmermann)
Attachment #8495794 - Flags: feedback+
Attachment #8495794 - Flags: review?(tzimmermann) → review+
Attachment #8495895 - Attachment description: Bug 1072142 - [woodduck] Read the 2nd socket message info directly (v2.0M) → Bug 1072142 - [woodduck] Read the 2nd socket message info directly, r=tzimmermann, f=btian (v2.0M)
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!
Flags: needinfo?(shuang)
We can land on v2.0 M first?
Flags: needinfo?(shuang)
(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
Status: NEW → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [partner-blocker] → [partner-blocker][2.0M_only]
Add tag in whiteboard to track specific platform dependency bug.
Whiteboard: [partner-blocker][2.0M_only] → [partner-blocker][2.0M_only][blueangel]
Target Milestone: --- → 2.1 S6 (10oct)
Blocks: Woodduck
Whiteboard: [partner-blocker][2.0M_only][blueangel] → [2.0M_only][blueangel]
Blocks: Woodduck_P2
Priority: -- → P1
No longer blocks: Woodduck_P2
You need to log in before you can comment on or make changes to this bug.