Closed Bug 1207011 Opened 5 years ago Closed 5 years ago

[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.

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(blocking-b2g:2.2r+, b2g-v2.2r fixed, b2g-master fixed)

RESOLVED FIXED
blocking-b2g 2.2r+
Tracking Status
b2g-v2.2r --- fixed
b2g-master --- fixed

People

(Reporter: jaliu, Assigned: jaliu)

References

Details

Attachments

(2 files, 3 obsolete files)

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

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.
Hi Ben,
Would you mind if I take this bug and ask you to review the patch?
Flags: needinfo?(btian)
Status: UNCONFIRMED → NEW
Ever confirmed: true
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?
Flags: needinfo?(btian)
Blocks: 1199528
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 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
Attachment #8664024 - Flags: review?(btian)
- revise previous patch based on #comment 6.

Thank Ben for the advice.
Attachment #8664024 - Attachment is obsolete: true
Attachment #8664738 - Flags: review?(btian)
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
Attachment #8664738 - Flags: review?(btian) → review+
- 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
Attachment #8665901 - Attachment is obsolete: true
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/2e7784fb1b3d

Also mark as 2.2r+ blocker some this bug blocks 2.2r required bluetooth feature.
Status: NEW → RESOLVED
blocking-b2g: --- → 2.2r+
Closed: 5 years ago
Resolution: --- → FIXED
Set checkin-needed for 2.2r branch landing.
Keywords: checkin-needed
Assignee: nobody → 6jamin
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.
You need to log in before you can comment on or make changes to this bug.