[woodduck] Read the 2nd socket message info directly

RESOLVED FIXED in Firefox OS v2.0M

Status

Firefox OS
Bluetooth
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: shawnjohnjr, Assigned: shawnjohnjr)

Tracking

(Blocks: 1 bug)

unspecified
2.1 S6 (10oct)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

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

Details

(Whiteboard: [2.0M_only][blueangel])

Attachments

(1 attachment, 3 obsolete attachments)

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?
Blocks: 1071407
Blocks: 1071424
Blocks: 1055400
Blocks: 1071390
Created attachment 8494459 [details] [diff] [review]
Bug 1072142 - [woodduck] Read the 2nd socket message info directly (v2.0M)
Created attachment 8494466 [details] [diff] [review]
Bug 1072142 - [woodduck] Read the 2nd socket message info directly (master)
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)
Attachment #8494459 - Flags: review?(tzimmermann)
Attachment #8494466 - Flags: review?(tzimmermann)
Attachment #8494459 - Flags: feedback?(btian)
Attachment #8494466 - Flags: feedback?(btian)

Comment 3

3 years ago
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?

Updated

3 years ago
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 9

3 years ago
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)
Created attachment 8495794 [details] [diff] [review]
Bug 1072142 - [woodduck] Read the 2nd socket message info directly (v2.0M)
Attachment #8495794 - Flags: review?(tzimmermann)
Attachment #8495794 - Flags: feedback+
Attachment #8495794 - Flags: review?(tzimmermann) → review+
Attachment #8495794 - Attachment is obsolete: true
Created attachment 8495895 [details] [diff] [review]
Bug 1072142 - [woodduck] Read the 2nd socket message info directly, r=tzimmermann, f=btian (v2.0M)
Blocks: 1072199
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.

Comment 14

3 years ago
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)
Keywords: checkin-needed
(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.

Comment 17

3 years ago
(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
Last Resolved: 3 years ago
status-b2g-v2.0M: --- → fixed
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]
status-b2g-v2.1: --- → unaffected
status-b2g-v2.2: --- → unaffected
Target Milestone: --- → 2.1 S6 (10oct)

Updated

3 years ago
Blocks: 1054172

Updated

3 years ago
Blocks: 1080337

Updated

3 years ago
Whiteboard: [partner-blocker][2.0M_only][blueangel] → [2.0M_only][blueangel]

Updated

3 years ago
Blocks: 1080481

Updated

3 years ago
status-b2g-v2.0: --- → affected

Updated

3 years ago
Duplicate of this bug: 1071407

Updated

3 years ago
Duplicate of this bug: 1071390

Updated

3 years ago
Duplicate of this bug: 1071424

Updated

3 years ago
status-b2g-v2.0: affected → unaffected

Updated

3 years ago
Blocks: 1107999

Updated

3 years ago
Priority: -- → P1

Updated

3 years ago
No longer blocks: 1107999
You need to log in before you can comment on or make changes to this bug.