1180556 - [Bluetooth] Pack PBAP replies to OBEX response message and reply to remote device.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

Pack PBAP replys to OBEX response message and reply to remote device. the PBAP replys includes replyTovCardPulling, replyToPhonebookPulling and replyTovCardListing.

This bug should also support vCardSelecting.

Comment 1

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

Attachment: Pack PBAP replys to OBEX response message and reply to remote device. (v0)

Comment 2

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

Attachment: Pack PBAP replys to OBEX response message and reply to remote device. (v1)

- revise previous patch based on the changes in Bug 1180554 and Bug 1180555.

Comment 3

[Ben Tian (inactive)]

Comment on attachment 8637053 [details] [diff] [review]
Pack PBAP replys to OBEX response message and reply to remote device. (v1)

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

Please see my comment below.

::: dom/bluetooth/ObexBase.cpp
@@ +23,5 @@
>  
>    aRetBuf[0] = aHeaderId;
>    aRetBuf[1] = (headerLength & 0xFF00) >> 8;
>    aRetBuf[2] = headerLength & 0x00FF;
>    memcpy(&aRetBuf[3], aData, (aLength < aBufferSize - 3) ? aLength

Revise as following to simplify variables here:

  int headerLength = aLength + 3;
  int writtenLength = (headerLength < aBufferSize) ? headerLength : aBufferSize;

  aRetBuf[0] = aHeaderId;
  aRetBuf[1] = (headerLength & 0xFF00) >> 8;
  aRetBuf[2] = headerLength & 0x00FF;
  memcpy(&aRetBuf[3], aData, writtenLength - 3);

  return writtenLength;

@@ +89,5 @@
> +  // [length] fields are each one byte in length.
> +
> +  aRetBuf[0] = aTagId;
> +  aRetBuf[1] = aLength;
> +  memcpy((aRetBuf + 2), aValue, aLength);

Handle |aBufferSize - 2 < aLength| case.

::: dom/bluetooth/bluedroid/BluetoothPbapManager.cpp
@@ +42,5 @@
>      }
>    };
>  
> +  /*
> +   * The format of a OBEX header is

nit: an OBEX

@@ +45,5 @@
> +  /*
> +   * The format of a OBEX header is
> +   * [headerId:1][header length:2]
> +   */
> +  static const uint32_t kObextHeaderSize = 3;

Move this definition to ObexBase{.h, .cpp}, and revise code requiring this constant.

nit: kObex' 'HeaderSize

@@ +83,5 @@
> +
> +private:
> +  nsRefPtr<Blob> mBlob;
> +  uint16_t mPhonebookSize;
> +  bool mHasPhonebookSize;

Remove this redundant member variable if it's useless.

@@ +916,5 @@
> +BluetoothPbapManager::SendGetResponse(Blob* aBlob)
> +{
> +  MOZ_ASSERT(mRemoteMaxPacketLength >= 255);
> +
> +  MOZ_ASSERT(aSocket);

Where is |aSocket| from?

@@ +923,5 @@
> +
> +  // Section 3.2 "Response format", IrOBEX 1.2
> +  // [opcode:1][length:2][Headers:var]
> +  unsigned int index = 3;
> +

nit: remove the newline to be consistent.

::: dom/bluetooth/bluedroid/BluetoothPbapManager.h
@@ +50,5 @@
>  
>  class BluetoothPbapManager : public BluetoothSocketObserver
>                             , public BluetoothProfileManagerBase
>  {
> +  class SendGetResponseTask;

Rename to |ReplyToGetTask|.

@@ +124,5 @@
>    void ReplyToDisconnectOrAbort();
>    void ReplyToSetPath();
>    void ReplyError(uint8_t aError);
>    void SendObexData(uint8_t* aData, uint8_t aOpcode, int aSize);
> +  void SendGetResponse(Blob* aBlob);

Rename to |ReplyToGet| to conform with other reply functions.

@@ +125,5 @@
>    void ReplyToSetPath();
>    void ReplyError(uint8_t aError);
>    void SendObexData(uint8_t* aData, uint8_t aOpcode, int aSize);
> +  void SendGetResponse(Blob* aBlob);
> +  void SendGetResponse(Blob* aBlob, uint16_t aPhonebookSize);

Integrate into one function by making |aPhonebookSize| as 0 by default, and revise |SendGetResponseTask| as well.

Comment 4

[Ben Tian (inactive)]

Comment on attachment 8637053 [details] [diff] [review]
Pack PBAP replys to OBEX response message and reply to remote device. (v1)

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

Forgot to write down comment we just discussed offline.

::: dom/bluetooth/bluedroid/BluetoothPbapManager.cpp
@@ +927,5 @@
> +
> +  uint8_t* req = new uint8_t[mRemoteMaxPacketLength];
> +
> +  // The headersLen of response, including the header of 'Body'
> +  unsigned int headersLen = index + kObextHeaderSize;

Please revise length composition here to make it more understandable.

@@ +943,5 @@
> +    (uint8_t*) (aBlob + mSentBlobLength),
> +    blobLen);
> +  index += headerBodyLen;
> +
> +  if (opcode == ObexResponseCode::Success) {

Integrate with opcode selection above.

@@ +983,5 @@
> +
> +  index += AppendHeaderAppParameters(&req[index], 252, appParameters, 4);
> +
> +  // The headersLen of response, including the header of 'Body'
> +  unsigned int headersLen = index + kObextHeaderSize;

Can we wrap this section as a function and reuse it?

@@ +1004,5 @@
> +    index += AppendHeaderEndOfBody(&req[index]);
> +    mBlob = nullptr;
> +    mSentBlobLength = 0;
> +  } else {
> +    mSentBlobLength += (headerBodyLen - 3);

Is the 3 |kObextHeaderSize|? Please replace it if so.

Comment 5

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

Attachment: Pack PBAP replys to OBEX response message and reply to remote device. (v2)

- Revise previous patch based on #comment 3 and #comment 4

Thank Ben for reviewing the patch.

Comment 6

[Ben Tian (inactive)]

Comment on attachment 8638469 [details] [diff] [review]
Pack PBAP replys to OBEX response message and reply to remote device. (v2)

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

Please see my comment below.

::: dom/bluetooth/ObexBase.cpp
@@ +18,5 @@
>  int
>  AppendHeader(uint8_t aHeaderId, uint8_t* aRetBuf, int aBufferSize,
>               const uint8_t* aData, int aLength)
>  {
> + int headerLength = aLength + 3;

nit: 2-space indention.

@@ +85,5 @@
> +AppendAppParameter(uint8_t* aRetBuf, int aBufferSize, const uint8_t aTagId,
> +                   const uint8_t* aValue, int aLength)
> +{
> +  // An application parameter is a [tag]-[length]-[value] triplet. The [tag] and
> +  // [length] fields are each one byte in length.

nit: ... are 1-byte length each.

@@ +87,5 @@
> +{
> +  // An application parameter is a [tag]-[length]-[value] triplet. The [tag] and
> +  // [length] fields are each one byte in length.
> +
> +  if (aBufferSize - 2 < aLength) {

I prefer |aBufferSize < aLength + 2| to be more intuitive and conform with returned |aLength + 2|.

@@ +89,5 @@
> +  // [length] fields are each one byte in length.
> +
> +  if (aBufferSize - 2 < aLength) {
> +    // aBufferSize should be larger than size of AppParameter + header.
> +    MOZ_ASSERT(false);

Keep either the if-return or assertion. if-return handles something 'might' happen; while MOZ_ASSERT fails to ensure something that should never happen. I think this is if-return case, isn't it?

@@ +95,5 @@
> +  }
> +
> +  aRetBuf[0] = aTagId;
> +  aRetBuf[1] = aLength;
> +  memcpy((aRetBuf + 2), aValue, aLength);

Replace with |&aRetBuf[2]| to conform with |AppendHeader| function.

::: dom/bluetooth/ObexBase.h
@@ +19,5 @@
> + * Section 3.2 "Response format"
> + * The format of an OBEX response header is
> + * [response code:1][response length:2]
> + */
> +static const uint32_t kObexResHeaderSize = 3;

Rename to |kObex'Resp'HeaderSize| or |kObex'Response'HeaderSize| since 'Res' may be regarded as 'result'.

::: dom/bluetooth/bluedroid/BluetoothPbapManager.cpp
@@ +50,5 @@
>  
> +class BluetoothPbapManager::ReplyToGetTask final : public nsRunnable
> +{
> +public:
> +  ReplyToGetTask(Blob* aBlob, uint16_t aPhonebookSize)

Please reconsider whether this task is required for additional dispatching. Also use default parameters to simplify function declaration.

@@ +103,5 @@
>    sInShutdown = true;
>    Disconnect(nullptr);
>    sPbapManager = nullptr;
> +  mLastOpcode = 0;
> +  mRequirePhonebookSize = false;

I think these variables should be cleared during disconnection rather than shutdown.

@@ +611,5 @@
> +      // Section 5 "Phone Book Access Profile Functions", PBAP 1.2
> +      // Replying 'PhonebookSize' is mandatory if 'MaxListCount' parameter is
> +      // present in the request with a value of 0, else it is excluded.
> +      if (maxListCount == 0) {
> +        mRequirePhonebookSize = true;

Simplify as following:

  mRequirePhonebookSize = !maxListCount;

@@ +840,5 @@
> +  nsRefPtr<ReplyToGetTask> task =
> +    new ReplyToGetTask(aBlob, aPhonebookSize);
> +
> +  if (NS_FAILED(NS_DispatchToCurrentThread(task))) {
> +

nit: remove extra newline.

@@ +904,5 @@
>  
>  void
> +BluetoothPbapManager::ReplyToGet(Blob* aBlob, uint16_t aPhonebookSize)
> +{
> +  MOZ_ASSERT(mRemoteMaxPacketLength >= 255);

Why is the assertion required? What happens if remote device can only receive packets smaller than 255 bytes?

@@ +912,5 @@
> +  // Part 2: [headerId:1][length:2][PhonebookSize:4]  (optional)
> +  // Part 3: [headerId:1][length:2][Body:var]
> +  // Part 4: [headerId:1][length:2][EndOfBody:0]      (optional)
> +
> +  // ---- Part 1, move index for [response code:1][length:2] ---- //

Move |res| declaration before comment // ---- Part 1, since the declaration doesn't be long to part 1.

@@ +913,5 @@
> +  // Part 3: [headerId:1][length:2][Body:var]
> +  // Part 4: [headerId:1][length:2][EndOfBody:0]      (optional)
> +
> +  // ---- Part 1, move index for [response code:1][length:2] ---- //
> +  unsigned int index = kObexResHeaderSize;

When do you assign value for response header (res[0~2])?

@@ +921,5 @@
> +  if (mRequirePhonebookSize) {
> +    // Section 6.2.1 "Application Parameters Header", PBAP 1.2
> +    // appParameters: [header ID:1][length:2][PhonebookSize:4]
> +    // phonebookSize: [tag ID:1][length:1][value:2]
> +    uint8_t appParameters[4];

Reorder as following:

  // convert little endian to big endian
  uint8_t phonebookSize[2];
  phonebookSize[0] = (aPhonebookSize & 0xFF00) >> 8;
  phonebookSize[1] = aPhonebookSize & 0x00FF;

  // Section 6.2.1 "Application Parameters Header", PBAP 1.2
  // appParameters: [headerId:1][length:2][PhonebookSize:4], where
  //                [PhonebookSize:4] = [tagId:1][length:1][value:2]
  uint8_t appParameters[4];
  AppendAppParameter(appParameters, ...

@@ +932,5 @@
> +    AppendAppParameter(appParameters,
> +                       sizeof(appParameters),
> +                       (uint8_t) AppParameterTag::PhonebookSize,
> +                       phonebookSize,
> +                       sizeof(appParameters));

This should be |sizeof(phonebookSize)|.

@@ +937,5 @@
> +
> +    index += AppendHeaderAppParameters(&res[index],
> +                                       mRemoteMaxPacketLength,
> +                                       appParameters,
> +                                       /* sizeof app app parameters*/ 4);

Replace with |sizeof(appParameters)|.

@@ +945,5 @@
> +  // The headersLen of response, including the header of 'Body'
> +  unsigned int headersLen = index + kObexBodyHeaderSize;
> +
> +  ErrorResult rv;
> +  uint64_t blobLen = aBlob->GetSize(rv);

Get blob size for first time and remember for later use instead of query for every packet. Also handle unsuccessful |rv| case.

@@ +947,5 @@
> +
> +  ErrorResult rv;
> +  uint64_t blobLen = aBlob->GetSize(rv);
> +
> +

nit: remove extra newline.

@@ +948,5 @@
> +  ErrorResult rv;
> +  uint64_t blobLen = aBlob->GetSize(rv);
> +
> +
> +  // Add a additional 'kObexBodyHeaderSize' for the header of 'EndOfBody'

nit: 'an' additional

@@ +950,5 @@
> +
> +
> +  // Add a additional 'kObexBodyHeaderSize' for the header of 'EndOfBody'
> +  // response data = [Body:blobLen + headersLen][EndOfBody:kObexBodyHeaderSize]
> +  unsigned int responseSize = blobLen + headersLen + kObexBodyHeaderSize;

Can we move the declaration right before if condition below, where |responseSize| is used at first time?

Also should |responseSize| be |index + kObexBodyHeaderSize| instead? Since index (w/ headerBodyLen) is the really packed size.

nit: switch the order of |blobLen| and |headerLen| in both comment and assignment to match real packing order.

@@ +957,5 @@
> +  unsigned int headerBodyLen = AppendHeaderBody(
> +    &res[index],
> +    mRemoteMaxPacketLength - headersLen,
> +    (uint8_t*) (aBlob + mSentBlobLength),
> +    blobLen);

Should this be |blobLen - mSentBlobLength|?

@@ +993,2 @@
>  
> +  SendObexData(res, aError, index);

Remove |index| and simplify to

  SendObexData(res, aError, kObexBodyHeaderSize);

::: dom/bluetooth/bluedroid/BluetoothPbapManager.h
@@ +153,5 @@
>    bool mConnected;
>    nsString mDeviceAddress;
>  
> +  /**
> +   * The max OBEX packet length from remote device.

Revise comment as following to be less ambiguous.

  Maximum packet length that remote device can receive

Comment 7

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

Attachment: Pack PBAP replys to OBEX response message and reply to remote device. (v3)

Comment 8

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

(In reply to Ben Tian [:btian] from comment #6)
> Comment on attachment 8638469 [details] [diff] [review]
> Pack PBAP replys to OBEX response message and reply to remote device. (v2)

Thank Ben for reviewing the patch.

> Why is the assertion required? What happens if remote device can only
> receive packets smaller than 255 bytes?
Instead of using magic number directly, I put a static const variable at ObexBase.h for this value and leave a comment there.  Please refer to #Attachment #8641533 [details] [diff].

> @@ +913,5 @@
> > +  // Part 3: [headerId:1][length:2][Body:var]
> > +  // Part 4: [headerId:1][length:2][EndOfBody:0]      (optional)
> > +
> > +  // ---- Part 1, move index for [response code:1][length:2] ---- //
> > +  unsigned int index = kObexResHeaderSize;
> 
> When do you assign value for response header (res[0~2])?
res[0~2] will be set when SendObexData() is being called.
I should leave a comment here.

> @@ +957,5 @@
> > +  unsigned int headerBodyLen = AppendHeaderBody(
> > +    &res[index],
> > +    mRemoteMaxPacketLength - headersLen,
> > +    (uint8_t*) (aBlob + mSentBlobLength),
> > +    blobLen);
> 
> Should this be |blobLen - mSentBlobLength|?
I think we should read data from Blob by using nsIInputStream.
Please refer to #Attachment #8641533 [details] [diff].

Comment 9

[Ben Tian (inactive)]

Comment on attachment 8641533 [details] [diff] [review]
Pack PBAP replys to OBEX response message and reply to remote device. (v3)

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

Please see my comment below.

::: dom/bluetooth/ObexBase.cpp
@@ +89,5 @@
> +  // [length] fields are 1-byte length each.
> +
> +  if (aBufferSize < aLength + 2) {
> +    // aBufferSize should be larger than size of AppParameter + header.
> +    NS_WARNING("Return buffer size is too small for the AppParameter");

Adopt BT_WARNING instead for consistency.

::: dom/bluetooth/bluedroid/BluetoothPbapManager.cpp
@@ +265,5 @@
> +      // As long as the response is Continue, The client continues to issue GET
> +      // requests until the final body information (in an End-of-Body header)
> +      // arrives, along with the response code 0xA0 Success.
> +      if (mLastOpcode == ObexResponseCode::Continue) {
> +        if (ReplyToGet(mVCardDataStream)) {

!ReplyToGet

@@ +266,5 @@
> +      // requests until the final body information (in an End-of-Body header)
> +      // arrives, along with the response code 0xA0 Success.
> +      if (mLastOpcode == ObexResponseCode::Continue) {
> +        if (ReplyToGet(mVCardDataStream)) {
> +          BT_WARNING("Failed to reply to PBAP GET request.");

Should we do something to handle the failure?

@@ +862,5 @@
> +                                 uint16_t aPhonebookSize)
> +{
> +  if (!aStream || mRemoteMaxPacketLength < kObexMinimumMaxSize) {
> +    MOZ_ASSERT(false);
> +    return false;

Separate for the 2 different conditions since |aStream| should never be nullptr.

  MOZ_ASSERT(aStream);

  if (mRemoteMaxPacketLength < kObexMinimumMaxSize) {
    BT_LOGR(/* error log */);
    return false;
  }

@@ +874,5 @@
> +
> +  uint8_t* res = new uint8_t[mRemoteMaxPacketLength];
> +
> +  // ---- Part 1, move index for [response code:1][length:2] ---- //
> +  // res[0~2] will be set when SendObexData() is being called.

Simplify to

  // res[0~2] will be set in SendObexData()

@@ +902,5 @@
> +  }
> +
> +  // ---- Part 3, add [headerId:1][length:2][Body:var] to response ---- //
> +  uint64_t streamLen;
> +  aStream->Available(&streamLen);

Remove |streamLen| for following revised if-condition.

@@ +907,5 @@
> +
> +  // The headersLen of response, including the header of 'Body'
> +  unsigned int headersLen = index + kObexBodyHeaderSize;
> +
> +  uint32_t availablePacketSize = mRemoteMaxPacketLength - headersLen;

Remove |headersLen| and rename |availablePacketSize| to |remainingPacketSize| as following:

  // Remaining packet size to append Body, excluding Body's header
  uint32_t remainingPacketSize = mRemoteMaxPacketLength - kObexBodyHeaderSize - index;
  
  // Read vcard data from input stream
  uint32_t numRead = 0;
  nsAutoArrayPtr<char> buffer(new char[remainingPacketSize]);
  nsresult rv = aStream->Read(buffer, remainingPacketSize, &numRead);

@@ +912,5 @@
> +
> +  uint32_t numRead;
> +  nsAutoArrayPtr<char> buffer(new char[availablePacketSize]);
> +  nsresult rv = mVCardDataStream->Read(buffer, availablePacketSize, &numRead);
> +

nit: remove newline here.

@@ +913,5 @@
> +  uint32_t numRead;
> +  nsAutoArrayPtr<char> buffer(new char[availablePacketSize]);
> +  nsresult rv = mVCardDataStream->Read(buffer, availablePacketSize, &numRead);
> +
> +  if (NS_FAILED(rv)) {

Please confirm whether this condition is hit if there's no data from input stream to read.

@@ +918,5 @@
> +    BT_WARNING("Failed to read from input stream");
> +    return false;
> +  }
> +
> +  // headerBodyLen = length of Body's header + length of Body

Remove this comment.

@@ +920,5 @@
> +  }
> +
> +  // headerBodyLen = length of Body's header + length of Body
> +  if (numRead) {
> +    unsigned int headerBodyLen = AppendHeaderBody(

Revise as following since |headerBodyLen| is unused any more.

  // Append Body's header and Body
  if (numRead) {
    index += AppendHeaderBody(
      ...
  }

@@ +922,5 @@
> +  // headerBodyLen = length of Body's header + length of Body
> +  if (numRead) {
> +    unsigned int headerBodyLen = AppendHeaderBody(
> +      &res[index],
> +      mRemoteMaxPacketLength - headersLen,

Replace with |availablePacketSize|.

@@ +929,5 @@
> +    index += headerBodyLen;
> +  }
> +  // Add an additional 'kObexBodyHeaderSize' for the header of 'EndOfBody'
> +  // response data = [Body:blobLen + headersLen][EndOfBody:kObexBodyHeaderSize]
> +  unsigned int responseSize = streamLen + headersLen + kObexBodyHeaderSize;

Remove this variable and revise if-condition below.

@@ +932,5 @@
> +  // response data = [Body:blobLen + headersLen][EndOfBody:kObexBodyHeaderSize]
> +  unsigned int responseSize = streamLen + headersLen + kObexBodyHeaderSize;
> +
> +  uint8_t opcode;
> +  if (responseSize > mRemoteMaxPacketLength) {

Revise with |numRead| and |availablePacketSize| as following:

  // More GET requests are required if remaining packet size isn't
  // enough for 1) number of bytes read and 2) one EndOfBody's header
  if (numRead + kObexBodyHeaderSize > availablePacketSize) {
    opcode = ObexResponseCode::Continue;
  } else {
    ...

@@ +944,5 @@
> +    aStream = nullptr;
> +  }
> +
> +  SendObexData(res, opcode, index);
> +

nit: remove new line here.

@@ +956,5 @@
> +                                             Blob* aBlob)
> +{
> +  // PBAP can only handle one OBEX BODY transfer at the same time.
> +  if (mVCardDataStream) {
> +    BT_WARNING("Shouldn't handle multiple PBAP response at the same time");

nit: multiple PBAP response's'

::: dom/bluetooth/bluedroid/BluetoothPbapManager.h
@@ +72,5 @@
>     * @param aActor [in]          a blob actor containing the vCard objects
>     * @param aPhonebookSize [in]  the number of vCard indexes in the blob
> +   *
> +   * @return true if the response packet has been packed correctly and started
> +   *         to be sended to the remote device.

nit: to be sent.

Add false otherwise at the end.

  ... to be sent to the remote device; false otherwise.

@@ +83,5 @@
>     * @param aBlob [in]           a blob contained the vCard objects
>     * @param aPhonebookSize [in]  the number of vCard indexes in the blob
> +   *
> +   * @return true if the response packet has been packed correctly and started
> +   *         to be sended to the remote device.

Ditto.

@@ +94,5 @@
>     * @param aActor [in]          a blob actor containing the vCard objects
>     * @param aPhonebookSize [in]  the number of vCard indexes in the blob
> +   *
> +   * @return true if the response packet has been packed correctly and started
> +   *         to be sended to the remote device.

Ditto.

@@ +105,5 @@
>     * @param aBlob [in]           a blob contained the vCard objects
>     * @param aPhonebookSize [in]  the number of vCard indexes in the blob
> +   *
> +   * @return true if the response packet has been packed correctly and started
> +   *         to be sended to the remote device.

Ditto.

@@ +115,5 @@
>     *
>     * @param aActor [in]  a blob actor containing the vCard objects
> +   *
> +   * @return true if the response packet has been packed correctly and started
> +   *         to be sended to the remote device.

Ditto.

@@ +125,5 @@
>     *
>     * @param aBlob [in]  a blob contained the vCard objects
> +   *
> +   * @return true if the response packet has been packed correctly and started
> +   *         to be sended to the remote device.

Ditto.

@@ +199,5 @@
> +   */
> +  nsCOMPtr<nsIInputStream> mVCardDataStream;
> +
> +  /**
> +   * A flag to indicate 'PhonebookSize' is mandatory for next OBEX response

nit: indicate whether 'PhonebookSize' ...

Comment 10

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

Attachment: Pack PBAP replys to OBEX response message and reply to remote device. (v4)

- revise previous patch based on #comment 9

Comment 11

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

(In reply to Ben Tian [:btian] from comment #9)
> Comment on attachment 8641533 [details] [diff] [review]
> Pack PBAP replys to OBEX response message and reply to remote device. (v3)
Thank you for reviewing the patch.

> @@ +266,5 @@
> > +      // requests until the final body information (in an End-of-Body header)
> > +      // arrives, along with the response code 0xA0 Success.
> > +      if (mLastOpcode == ObexResponseCode::Continue) {
> > +        if (ReplyToGet(mVCardDataStream)) {
> > +          BT_WARNING("Failed to reply to PBAP GET request.");
> 
> Should we do something to handle the failure?
Yes. I should reply an error response here.
Thanks.

> @@ +913,5 @@
> > +  uint32_t numRead;
> > +  nsAutoArrayPtr<char> buffer(new char[availablePacketSize]);
> > +  nsresult rv = mVCardDataStream->Read(buffer, availablePacketSize, &numRead);
> > +
> > +  if (NS_FAILED(rv)) {
> 
> Please confirm whether this condition is hit if there's no data from input
> stream to read.
Got it.
I've tested it on my device.
The return value of Read() is 'success' even when numRead is 0.

Comment 12

[Ben Tian (inactive)]

Comment on attachment 8642795 [details] [diff] [review]
Pack PBAP replys to OBEX response message and reply to remote device. (v4)

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

Please see my comment below.

::: dom/bluetooth/ObexBase.h
@@ +33,5 @@
> + * Section 3.3.1.4 "Minimum OBEX Packet Length", IrOBEX ver 1.2
> + * The minimum size of the OBEX Maximum packet length allowed for negotiation is
> + * 255 bytes.
> + */
> +static const uint32_t kObexMinimumMaxSize = 255;

Rename to |kObexLeastMaxSize| since MinimumMax is less clear to understand.

::: dom/bluetooth/bluedroid/BluetoothPbapManager.cpp
@@ +223,5 @@
>        }
>  
> +      // Save the max packet length from remote information
> +      mRemoteMaxPacketLength = ((static_cast<int>(data[5]) << 8) | data[6]);
> +

Can we check whether |mRemoteMaxPacketLength| is large enough and reply error here if not?

@@ +260,5 @@
>        ReplyToSetPath();
>        break;
>      }
>      case ObexRequestCode::Get:
>      case ObexRequestCode::GetFinal: {

When do we receive GetFinal?

@@ +264,5 @@
>      case ObexRequestCode::GetFinal: {
> +      // As long as the response is Continue, The client continues to issue GET
> +      // requests until the final body information (in an End-of-Body header)
> +      // arrives, along with the response code 0xA0 Success.
> +      if (mLastOpcode == ObexResponseCode::Continue) {

Can we replace this condition with |if (mVCardDataStream)|? Since it's non-nullptr only during transfer and we can remove |mLastOpcode|.

@@ +803,5 @@
> +    return false;
> +  }
> +
> +  if (!GetInputStreamFromBlob(mVCardDataStream, aBlob)) {
> +    return false;

Should we reply error response to remote device?

@@ +864,5 @@
> +{
> +  if (!aStream || mRemoteMaxPacketLength < kObexMinimumMaxSize) {
> +    MOZ_ASSERT(false);
> +    return false;
> +  }

Remove the whole section.

@@ +869,5 @@
> +
> +  MOZ_ASSERT(aStream);
> +
> +  if (mRemoteMaxPacketLength < kObexMinimumMaxSize) {
> +    BT_WARNING("OBEX Maximum packet length is too short.");

Print out |mRemoteMaxPacketLength| and revise log as following

  BT_WARNING("OBEX Maximum packet length %d is shorter than %d bytes",
             mRemoteMaxPacketLength, kObexMinimumMaxSize);

@@ +906,5 @@
> +    index += AppendHeaderAppParameters(&res[index],
> +                                       mRemoteMaxPacketLength,
> +                                       appParameters,
> +                                       sizeof(appParameters));
> +  }

Can we reset |mRequirePhonebookSize| here, right after appending phone book size? So that we don't have to reset in |SendObexData|.

@@ +912,5 @@
> +  // ---- Part 3, add [headerId:1][length:2][Body:var] to response ---- //
> +  // The headersLen of response, including the header of 'Body'
> +  unsigned int headersLen = index + kObexBodyHeaderSize;
> +
> +  uint32_t availablePacketSize = mRemoteMaxPacketLength - headersLen;

Remove the two declaration.

@@ +923,5 @@
> +  uint32_t numRead = 0;
> +  nsAutoArrayPtr<char> buffer(new char[remainingPacketSize]);
> +  nsresult rv = aStream->Read(buffer, remainingPacketSize, &numRead);
> +  if (NS_FAILED(rv)) {
> +    BT_WARNING("jamin: Failed to read from input stream.");

nit: remove 'jamin:'.

Comment 13

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

(In reply to Ben Tian [:btian] from comment #12)
> Comment on attachment 8642795 [details] [diff] [review]
> Pack PBAP replys to OBEX response message and reply to remote device. (v4)
Thank you for viewing the patch.

> @@ +223,5 @@
> >        }
> >  
> > +      // Save the max packet length from remote information
> > +      mRemoteMaxPacketLength = ((static_cast<int>(data[5]) << 8) | data[6]);
> > +
> 
> Can we check whether |mRemoteMaxPacketLength| is large enough and reply
> error here if not?
Good advice.

> @@ +260,5 @@
> >        ReplyToSetPath();
> >        break;
> >      }
> >      case ObexRequestCode::Get:
> >      case ObexRequestCode::GetFinal: {
> 
> When do we receive GetFinal?
If all of the headers of the PBAP request can be placed into one packet, it we will receive GetFinal.
Since the minimum size of the OBEX maximum packet length is 255 bytes, all of PBAP requests should be able to be placed into one OBEX packet.
I will leaved a comment on the next patch.

> @@ +264,5 @@
> >      case ObexRequestCode::GetFinal: {
> > +      // As long as the response is Continue, The client continues to issue GET
> > +      // requests until the final body information (in an End-of-Body header)
> > +      // arrives, along with the response code 0xA0 Success.
> > +      if (mLastOpcode == ObexResponseCode::Continue) {
> 
> Can we replace this condition with |if (mVCardDataStream)|? Since it's
> non-nullptr only during transfer and we can remove |mLastOpcode|.
Sure. :)

> @@ +803,5 @@
> > +    return false;
> > +  }
> > +
> > +  if (!GetInputStreamFromBlob(mVCardDataStream, aBlob)) {
> > +    return false;
> 
> Should we reply error response to remote device?
Yes, I believe we should reply |ObexResponseCode::InternalServerError|.

Comment 14

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

Attachment: Pack PBAP replys to OBEX response message and reply to remote device. (v5)

- revise previous patch based on #comment 12.

Comment 15

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

Attachment: Pack PBAP replys to OBEX response message and reply to remote device. (v5.1)

- revise one comment in BluetoothPbapManager::ReceiveSocketData()

Comment 16

[Ben Tian (inactive)]

Comment on attachment 8642938 [details] [diff] [review]
Pack PBAP replys to OBEX response message and reply to remote device. (v5.1)

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

r=me with comment addressed. Thanks for the effort!

::: dom/bluetooth/bluedroid/BluetoothPbapManager.cpp
@@ +225,5 @@
> +      mRemoteMaxPacketLength = ((static_cast<int>(data[5]) << 8) | data[6]);
> +
> +      if (mRemoteMaxPacketLength < kObexLeastMaxSize) {
> +        BT_WARNING("OBEX Maximum packet length %d is shorter than %d bytes",
> +                    mRemoteMaxPacketLength, kObexMinimumMaxSize);

Use BT_LOGR to be informative since it's abnormal case. Also revise log (sorry again) as following and replace |kObexMinimumMaxSize| with |kObexLeastMaxSize|.

  BT_LOGR("Remote maximum packet length %d is smaller than %d bytes",
          mRemoteMaxPacketLength, kObexLeastMaxSize);

@@ +936,5 @@
> +  if (numRead) {
> +    index += AppendHeaderBody(&res[index],
> +                             remainingPacketSize,
> +                             (uint8_t*) buffer.forget(),
> +                             numRead);

nit: one more space indention to align with |&res[index]|.

Comment 17

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

Attachment: Pack PBAP replies to OBEX response message and reply to remote device. (v6), r=btian

- revise previous patch based on #comment 16

Thank Ben for reviewing the patch.

Comment 18

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

Treeherder:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4c4c52e8ba7e (ICS build was busted.)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=144cddc7877e (ICS build was fixed by #Attachment 8649650 [details] [diff] in #Bug 1180555)

Comment 19

[Pulsebot]

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

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Backed out for test_interfaces.html failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=2559824&repo=b2g-inbound

Comment 21

[Pulsebot]

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

Comment 22

[Ben Tian (inactive)]

Mark feature-b2g: 2.2r+ since PBAP is required bluetooth feature.

Comment 23

[Pulsebot]

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

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out 3 changesets (131251625ee8 for bug 1180556, 5bdcc058e6d6 for bug 1180555, f7e0cd74c082 for bug 1180554) for B"G ICS Emulator opt M8 and debug M19 failures. r=backout
https://hg.mozilla.org/integration/b2g-inbound/rev/00b847293e70

Comment 25

[Pulsebot]

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

Comment 26

[Ben Tian (inactive)]

Attachment: [2.2r] Pack PBAP replies to OBEX response message and reply to remote device. (v6), r=btian

Comment 27

[Ben Tian (inactive)]

checkin-needed to land 2.2r patch in comment 26.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4dcdc90adbe5

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2r/rev/136c3d2d9a5c