1207011 - [PBAP] Some platforms can not handle OBEX Body and End-Of-Body when they were sent in the same OBEX packet, including PTS 6.2.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Jamin Liu [:jaliu][:Jamin]]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:40.0) Gecko/20100101 Firefox/40.0
Build ID: 20150826023504

Comment 1

[Jamin Liu [:jaliu][:Jamin]]

Some platforms can not handle Body and End-Of-Body when they were sent in the same OBEX packet. For instance, FxOS can't pass the PBAP test case TC_PSE_PBD_BV_03_C with PTS 6.2. The error message shows "PTC OBEX : BODY and END OF BODY cannot be sent in the same command. "

End-of-Body is used to identify the last chunk of the object body.
In practice, some platforms can only handle zero length End-of-Body header separately with Body header.

Therefore, I think we should send OBEX End-of-Body header individually to improve the compatibility with other devices.

Comment 2

[Jamin Liu [:jaliu][:Jamin]]

Hi Ben,
Would you mind if I take this bug and ask you to review the patch?

Comment 3

[Ben Tian (inactive)]

Sure please:)

(In reply to Jamin Liu [:jaliu][:Jamin] from comment #2)
> Hi Ben,
> Would you mind if I take this bug and ask you to review the patch?

Comment 4

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Send Bluetooth OBEX End-of-Body header individually to improve the compatibility with other devices. (v1)

Comment 5

[Jamin Liu [:jaliu][:Jamin]]

I tested difference approaches for FxOS PBAP feature with PTS 6.2 and Trywin PND.

Approach 1: (current implementation on FxOS)
{Success:[Body: n][EndOfBody: 0]}

Approach 2: (used by Attachment #8664024 [details] [diff])
{Continue:[Body: n]}, {Success:[EndOfBody :0]}

Approach 3: (the most efficient approach)
{Success:[EndOfBody: n]}

It turned out ...
--> PTS 6.2 accepts approach 2 and approach 3.
--> Trywin DTN-5600 accepts approach 1 and approach 2.

According to IrOBEX v1.2 3.3.3.5, it seems that approach 2 is the recommended approach.
To improve the compatibility, I believe approach 2 would be a better choice.

Comment 6

[Ben Tian (inactive)]

Comment on attachment 8664024 [details] [diff] [review]
Send Bluetooth OBEX End-of-Body header individually to improve the compatibility with other devices. (v1)

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

Please see my comment below. I suggest to check |mVCardDataStream->Available()| before read to save the |Read| call.

::: dom/bluetooth/bluedroid/BluetoothPbapManager.cpp
@@ +896,5 @@
>      if (NS_FAILED(rv)) {
>        BT_LOGR("Failed to read from input stream. rv=0x%x",
>                static_cast<uint32_t>(rv));
>        return false;
>      }

nit: newline after }

@@ +897,5 @@
>        BT_LOGR("Failed to read from input stream. rv=0x%x",
>                static_cast<uint32_t>(rv));
>        return false;
>      }
> +    // End-of-Body is used to identify the last chunk of the object body.

nit: use multiple line comment style

  /**
   * ...
   */

@@ +906,1 @@
>      if (numRead) {

Check |mVCardDataStream->Available()| [1] before read to save the |Read| call. Also revise comments accordingly.
[1] https://dxr.mozilla.org/mozilla-central/source/obj-x86_64-unknown-linux-gnu/dist/include/nsIInputStream.h#66

  if (mPhonebookSizeRequired) {
    ...
  } else {
    MOZ_ASSERT(mVCardDataStream);

    uint64_t bytesAvailable = 0;
    nsresult rv = mVCardDataStream->Available(&bytesAvailable);
    if (NS_FAILED(rv)) {
      BT_LOGR("Failed to get available bytes from input stream. rv=0x%x",
              static_cast<uint32_t>(rv));
      return false;
    }

    /**
     * In practice, some platforms can only handle zero length End-of-Body
     * header separately with Body header.
     * Thus, append End-of-Body only if the data stream had been sent out,
     * otherwise, send 'Continue' to request for next GET request.
     */
    if (!bytesAvailable) {
      // Append End-of-Body
    } else {
      // Read from stream and append Body
    }
  }

@@ -919,5 @@
>    SendObexData(res, opcode, index);
>    delete [] res;
>    return true;
>  }
> --

Restore the newline. Note your patch differs from [1] with some newlines removed here. Please confirm your patch can be applied to m-c.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothPbapManager.cpp#923

Comment 7

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Send Bluetooth OBEX End-of-Body header individually to improve the compatibility with other devices. (v2)

- revise previous patch based on #comment 6.

Thank Ben for the advice.

Comment 8

[Ben Tian (inactive)]

Comment on attachment 8664738 [details] [diff] [review]
Send Bluetooth OBEX End-of-Body header individually to improve the compatibility with other devices. (v2)

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

r=me with comment addressed.

::: dom/bluetooth/bluedroid/BluetoothPbapManager.cpp
@@ +880,5 @@
>      opcode = ObexResponseCode::Success;
>      index += AppendHeaderEndOfBody(&res[index]);
>      mPhonebookSizeRequired = false;
>    } else {
>      MOZ_ASSERT(mVCardDataStream);

nit: newline after MOZ_ASSERT

@@ +889,3 @@
>                static_cast<uint32_t>(rv));
>        return false;
>      }

nit: newline after }

@@ +911,5 @@
> +
> +      // Read vCard data from input stream
> +      uint32_t numRead = 0;
> +      nsAutoArrayPtr<char> buf(new char[remainingPacketSize]);
> +      nsresult rv = mVCardDataStream->Read(buf, remainingPacketSize, &numRead);

Reuse |rv| declared above.

@@ +917,5 @@
> +        BT_LOGR("Failed to read from input stream. rv=0x%x", rv);
> +        return false;
> +      }
> +
> +      // ----  Part 2b: [headerId:1][length:2][Body:var] ---- //

Assert non-zero |numRead| as following:

    ...
  }

  // |numRead| must be non-zero as |bytesAvailable| is non-zero
  MOZ_ASSERT(numRead);

  // ---- Part 2b: [headerId:1][length:2][Body:var] ---- //

@@ -918,5 @@
>    SendObexData(res, opcode, index);
>    delete [] res;
>    return true;
>  }
> --

Again, please restore the newline as your patch differs from [1] with some newlines removed here. Please confirm your patch can be applied to m-c.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothPbapManager.cpp#923

Comment 9

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Send Bluetooth OBEX End-of-Body header individually to improve the compatibility with other devices. (v3) r=btian

- Revise previous patch based on #comment 8
Thank Ben for review the patch.

Try server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6528aaa2db0a

Comment 10

[Jamin Liu [:jaliu][:Jamin]]

Attachment: Send Bluetooth OBEX End-of-Body header individually to improve the compatibility with other devices. (v3.1) r=btian

- Fix the compile error on https://treeherder.mozilla.org/#/jobs?repo=try&revision=6528aaa2db0a   ( remove one extra ')' in BluetoothPbapManager::ReplyToGet() )

This results of try server are all green.
Treeherder:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=5901c262a2ac

Comment 11

[Pulsebot]

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

Comment 12

[Ben Tian (inactive)]

http://hg.mozilla.org/mozilla-central/rev/2e7784fb1b3d

Also mark as 2.2r+ blocker some this bug blocks 2.2r required bluetooth feature.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2e7784fb1b3d

Comment 14

[Ben Tian (inactive)]

Set checkin-needed for 2.2r branch landing.

Comment 15

[Jamin Liu [:jaliu][:Jamin]]

Attachment: (for 2.2r) Send Bluetooth OBEX End-of-Body header individually to improve the compatibility with other devices. r=btian

- rebased on v2.2r branch.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/e0241e971e39

Comment 17

[Ben Tian (inactive)]

Comment on attachment 8667829 [details] [diff] [review]
Send Bluetooth OBEX End-of-Body header individually to improve the compatibility with other devices. (v3.1) r=btian

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

::: dom/bluetooth/bluedroid/BluetoothPbapManager.cpp
@@ +843,5 @@
>     * - Part 3: [headerId:1][length:2][EndOfBody:0]
>     * Otherwise,
> +   * - Part 2a: [headerId:1][length:2][EndOfBody:0]
> +   *   or
> +   * - Part 2b: [headerId:1][length:2][Body:var]

The 2.2R patch in comment 15 doesn't revise comment here as m-c patch. I'll revise it in later 2.2R patch.