1142408 - [Bluetooth][PTS][2.2] AVRCP- TC_TG_NFY_BV_02_C failed

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Shing Lyu [:shinglyu]]

Attachment: _2015_03_11_17_32_44.zip

The test runs forever until timed out

##STR

Test case : TC_TG_NFY_BV_02_C started
	- MTC: SDP Service record was successfully registered.
	- A2DP SNK SDP Service record successfully registered
	- A2DP SNKsuccessfully registered
	- MTC INCONC: IUT failed to successfully respond to AVDTP CONNECT.
	- AVDTP: Exiting AVDTP1 PTC
	- AVCTP: Exiting AVCTP1 PTC
	- CM_PTC_EXIT
	- MTC: Test case ended
Final Verdict:INCONC
TC_TG_NFY_BV_02_C finished

## Version
* V18D
* 2014/03/12's pvt build for 2.2
* adb shell setprop ro.moz.bluetooth.backend bluetoothd

Comment 1

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

This looks like you have not yet paired before. Due to the test case needs to establish connection from PTS machine (incoming connection). So I would suggest you can run one HFP AG-AG_ATA_BV_02_I to make sure you paired with PTS machine, then you can cancel AG-AG_ATA_BV_02_I test case and start to test AVRCP test cases.

PTS cannot handle this situation if you haven't pair with phone (if you don't pair the incoming connection will be blocked by the phone).

Comment 2

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

This looks like you have not yet paired before. Due to the test case needs to establish connection from PTS machine (incoming connection). So I would suggest you can run one HFP AG-AG_ATA_BV_02_I to make sure you paired with PTS machine, then you can cancel AG-AG_ATA_BV_02_I test case and start to test AVRCP test cases.

PTS cannot handle this situation if you haven't pair with phone (if you don't pair the incoming connection will be blocked by the phone).

Comment 3

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

I will take this bug.

03-16 15:47:44.046 E/bt-avp  ( 1643): opcode 0
03-16 15:47:44.046 D/bt-btif ( 1643): btif_av_state_opening_handler event:BTA_AV_META_MSG_EVT flags 0
03-16 15:47:44.046 E/bluetoothd( 1643): PDU overflow

If fails when getting register notification with event track changed. This is a regression.

Comment 4

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

PDU overflow

#0  read_pdu_at_va (ap=..., fmt=<optimized out>, offset=3, pdu=0xb6e8c010) at system/bluetoothd/src/bt-proto.c:140
#1  read_pdu_at (pdu=pdu@entry=0xb6e8c010, offset=<optimized out>, fmt=0xb6f7ce24 "m") at system/bluetoothd/src/bt-proto.c:157
#2  0xb6f7ae58 in read_btrc_uid_t (pdu=pdu@entry=0xb6e8c010, off=<optimized out>, p_val=p_val@entry=0xbe9974f0 "\n") at system/bluetoothd/src/bt-proto.c:445
#3  0xb6f7ae9a in read_btrc_register_notification_t (pdu=pdu@entry=0xb6e8c010, off=<optimized out>, event_id=BTRC_EVT_TRACK_CHANGE, param=param@entry=0xbe9974f0) at system/bluetoothd/src/bt-proto.c:465
#4  0xb6f7b69c in opcode_register_notification_rsp (cmd=0xb6e8c010) at system/bluetoothd/src/bt-rc-io.c:676
#5  0xb6f7aabc in handle_pdu (handler=<optimized out>, cmd=<optimized out>, value=<optimized out>, field=0xb6f7cfad "opcode") at system/bluetoothd/src/bt-proto.c:50
#6  handle_pdu_by_opcode (cmd=<optimized out>, handler=<optimized out>) at system/bluetoothd/src/bt-proto.c:64

Comment 5

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

PDU overflow, offset+len: 11, pdu->len: 10

According to spec, command parameters:
  Event (1 octet)
  Type (1 octet)
  Data length (1 octet)
  Data (variable)

It looks like one parameter 'Data length' is missing :(
See:
http://hg.mozilla.org/releases/mozilla-b2g37_v2_2/file/18619f8f6c5c/dom/bluetooth/bluedroid/BluetoothDaemonAvrcpInterface.cpp#l268

Comment 9

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1142408 - Add data length parameter for Register Notification Response Command

I've tested notification related test cases, NFY_BV_02, 04, 05, 08, these test cases are pass.

Comment 10

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

2.2? for PTS certification requirement.

Comment 11

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8578041 [details] [diff] [review]
Bug 1142408 - Add data length parameter for Register Notification Response Command

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

Thanks for finding and fixing this bug. I set the patch to r- because it could introduce similar problems for other parameters. See below for an idea on how to handle the issue.

::: dom/bluetooth/bluedroid/BluetoothDaemonAvrcpInterface.cpp
@@ +257,5 @@
>    BluetoothAvrcpResultHandler* aRes)
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +  uint8_t len = 8; // UID size

The value of |len| depends on the values of |aEvent| and |aType|. Please add a method to compute the size of the data. The best place is probably something like |size_t BluetoothAvrcpEventParamPair::GetLength|.

The code in the local method would then look like this

>  ...
>  BluetoothAvrcpEventParamPair data(aEvent, aParam);
> 
>  PackPDU(aEvent, aType, data->GetLength(), data, *pdu);
>  ...

The code for packing is at [1].

[1] https://mxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothDaemonHelpers.cpp#1139

Comment 12

[Shing Lyu [:shinglyu]]

Hi Shawn,

Sorry I was on sick leave yesterday. Seems like Teri have already helped you run the PTS again. Do you still need a fresh log right now?

Comment 13

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Shing Lyu [:slyu] from comment #12)
> Hi Shawn,
> 
> Sorry I was on sick leave yesterday. Seems like Teri have already helped you
> run the PTS again. Do you still need a fresh log right now?

Nope :) Thanks for all the help. I made a stupid mistake to test in-gecko HAL interface instead of bluetooth daemon last week so this is why I did not notice that we have these Notification series bugs.
Thanks a lot!

Comment 14

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1142408 - Add data length parameter for Register Notification Response Command

Comment 15

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8578573 [details] [diff] [review]
Bug 1142408 - Add data length parameter for Register Notification Response Command

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

::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.h
@@ +86,5 @@
>    { }
>  
> +  size_t GetLength()
> +  {
> +    return sizeof(mParam);

This won't work. The structure [1] is quite large and the actual size depends on the value of |mEvent|. I suggest to handle each case separately in a switch statement. You can probably use the packing code [2] as template.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/bluetooth/BluetoothCommon.h#443
[2] https://mxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothDaemonHelpers.cpp#1139

Comment 16

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Hmm, a bit strange, I tested this patch, I got the correct result. I tested notification of TRACK_CHANGE, or PLAYSTATUS_CHANGED, track numbers and play status both correct.

Comment 17

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Maybe it's because |len| actually been ignored?
https://github.com/mozilla-b2g/platform_system_bluetoothd/blob/v2.2/src/bt-proto.c#L458

Comment 18

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Shawn Huang [:shawnjohnjr] from comment #17)
> Maybe it's because |len| actually been ignored?
> https://github.com/mozilla-b2g/platform_system_bluetoothd/blob/v2.2/src/bt-
> proto.c#L458

Lol :D

I'd still prefer to fix this correctly.

Comment 19

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1142408 - Add data length parameter for Register Notification Response Command

Comment 20

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8579178 [details] [diff] [review]
Bug 1142408 - Add data length parameter for Register Notification Response Command

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

One of the cases seems to return an incorrect value; r+ with this fixed. Thanks a lot!

::: dom/bluetooth/bluedroid/BluetoothDaemonAvrcpInterface.cpp
@@ +263,5 @@
>                             1 + // Event
>                             1 + // Type
>                             1 + // Data length
>                             256)); // Maximum data length
> +  BluetoothAvrcpEventParamPair data(aEvent, aParam);

Nit: could you add an empty line before this one and remove the one on line 270.

::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.h
@@ +105,5 @@
> +      case AVRCP_EVENT_PLAY_POS_CHANGED:
> +        size = sizeof(mParam.mSongPos);
> +        break;
> +      case AVRCP_EVENT_APP_SETTINGS_CHANGED:
> +        size = sizeof(mParam.mIds) + sizeof(mParam.mValues) + sizeof(mParam.mNumAttr);

We are packing pairs of ids and values. So this should be '(sizeof(mParam.mIds[0]) + sizeof(mParam.mValues[0])) * mNumAttr'.

Comment 21

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: (v2.2) Bug 1142408 - Add data length parameter for Register Notification Response Command, r=tzimmermann

Comment 22

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbee172c2389

Comment 23

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8579253 [details] [diff] [review]
(v2.2) Bug 1142408 - Add data length parameter for Register Notification Response Command, r=tzimmermann

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): daemon feature
User impact if declined: Can not receive notifcation/fail to pass certification
Testing completed: Pass PTS test case
Risk to taking this patch (and alternatives if risky):Add missing parameter 
String or UUID changes made by this patch: None

Comment 24

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #22)
> try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bbee172c2389
Try server seems to be green.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/dc2322ecd24a

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/dc2322ecd24a

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/744630454912