1181482 - Implement GATT Server read / write request

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Bruce Sun [:brsun]]

GATT Server read / write request:
 - Read / write request for characteristic
 - Read / write response for characteristic
 - Read / write request for descriptor
 - Read / write response for descriptor

Comment 1

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1181482 - Patch1: Refine some data types in gecko backend for GATT server read/write request APIs.

* use uint16_t for |SendResponse| since application might want to use their own error status code
* replace int by BluetoothAttributeHandle for handles

Comment 2

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1181482 - Patch2: Revise read/write characteristic/descriptor value to cover both GATT client and GATT server role.

Comment 3

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1181482 - Patch3: Implement |sendResponse| and BluetoothGattAttributeEvent for GATT server read/write requests.

* implement requestread/write notifications and fire attributereadreq/attributewritereq events
* implement |sendResponse|

Comment 4

[Ben Tian (inactive)]

Comment on attachment 8656394 [details] [diff] [review]
Bug 1181482 - Patch1: Refine some data types in gecko backend for GATT server read/write request APIs.

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

LGTM.

Comment 5

[Ben Tian (inactive)]

Comment on attachment 8656395 [details] [diff] [review]
Bug 1181482 - Patch2: Revise read/write characteristic/descriptor value to cover both GATT client and GATT server role.

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

Please see my question below.

::: dom/bluetooth/common/webapi/BluetoothGattCharacteristic.cpp
@@ +406,5 @@
>    NS_ENSURE_TRUE(!aRv.Failed(), nullptr);
>  
> +  if (mAttRole == ATT_SERVER_ROLE) {
> +    DispatchReplySuccess(new ReadValueTask(this, promise),
> +                         BluetoothValue(mValue));

Can we convert |mValue| to JS value as [1] and resolve here in content thread directly?

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/common/webapi/BluetoothGattDescriptor.cpp?offset=1900#140

  // Convert |mValue| to JS value
  ...
  promise->MaybeResolve(value);

@@ +442,5 @@
> +
> +  if (mAttRole == ATT_SERVER_ROLE) {
> +    mValue.Clear();
> +    mValue.AppendElements(aValue.Data(), aValue.Length());
> +    DispatchReplySuccess(new BluetoothVoidReplyRunnable(nullptr, promise));

promise->MaybeResolve(JS::UndefinedHandleValue)

Comment 6

[Jocelyn Liu [:jocelyn] [:joliu]]

Hi Ben,

Thanks for your time, please see my replies below.

Thanks,
Jocelyn
(In reply to Ben Tian [:btian] from comment #5)
> Comment on attachment 8656395 [details] [diff] [review]
> Bug 1181482 - Patch2: Revise read/write characteristic/descriptor value to
> cover both GATT client and GATT server role.
> 
> Review of attachment 8656395 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Please see my question below.
> 
> ::: dom/bluetooth/common/webapi/BluetoothGattCharacteristic.cpp
> @@ +406,5 @@
> >    NS_ENSURE_TRUE(!aRv.Failed(), nullptr);
> >  
> > +  if (mAttRole == ATT_SERVER_ROLE) {
> > +    DispatchReplySuccess(new ReadValueTask(this, promise),
> > +                         BluetoothValue(mValue));
> 
> Can we convert |mValue| to JS value as [1] and resolve here in content
> thread directly?
> 
> [1]
> https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/common/webapi/
> BluetoothGattDescriptor.cpp?offset=1900#140
> 
>   // Convert |mValue| to JS value
>   ...
>   promise->MaybeResolve(value);
> 
It's doable but I chose to still dispatch a task to main thread because
1) To be more consistent with all other APIs' behavior.
2) Save some duplicate codes since ReadValueTask and ReplyRunnable has codes for handling JS value stuff.

Comment 7

[Ben Tian (inactive)]

(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #6)
> It's doable but I chose to still dispatch a task to main thread because
> 1) To be more consistent with all other APIs' behavior.

Disagree. [1] also resolves/rejects before calling into parent thread. Or do you suggest to revise [1] with DispatchReplySuccess/Error?

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/common/webapi/BluetoothAdapter.cpp#627

> 2) Save some duplicate codes since ReadValueTask and ReplyRunnable has codes
> for handling JS value stuff.

Agree on ReadValueTask. But for VoidReplyRunnable it does merely the same promise resolving [2][3].

[2] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/common/BluetoothReplyRunnable.cpp#58
[3] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/common/BluetoothReplyRunnable.h#86

Comment 8

[Jocelyn Liu [:jocelyn] [:joliu]]

Had an offline discussion with Ben and Bruce, our conclusion is:
Resolve/Reject directly without dispatching tasks if no ipc or backend involved.
I will revise patch2 to follow this design, thanks!

Comment 9

[Bruce Sun [:brsun]]

> Resolve/Reject directly without dispatching tasks if no ipc or backend
> involved.

Because tones of tasks run on the main thread, it would be good to separate our tasks into small pieces and give chances to others to preempt the resource. Original thought to dispatch a runnable for resolving the promise is based on this. But since we don't have any CPU/IO intense tasks running in the content side currently regarding to the Bluetooth function, probably we could take it easy and simply resolve the promise directly.

Maybe we should re-exam our design one day in the future if we have some heavy loading codes in the content side.

Comment 10

[Bruce Sun [:brsun]]

Well...it seems that I am too naive and a little over-anxious. Resolving or rejecting a promise would trigger further task dispatching. So information on comment 9 might be misleading and worthless. Please kindly ignore comment 9.

Comment 11

[Jocelyn Liu [:jocelyn] [:joliu]]

Comment on attachment 8656397 [details] [diff] [review]
Bug 1181482 - Patch3: Implement |sendResponse| and BluetoothGattAttributeEvent for GATT server read/write requests.

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

::: dom/bluetooth/common/webapi/BluetoothGattServer.cpp
@@ +149,5 @@
>  void
> +BluetoothGattServer::HandleReadWriteRequest(const BluetoothValue& aValue,
> +                                            const nsAString& aEventName)
> +{
> +  MOZ_ASSERT(v.type() == BluetoothValue::TArrayOfBluetoothNamedValue);

it should be aValue, will fix it in the next version.

Comment 12

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1181482 - Patch2(v2): Revise read/write characteristic/descriptor value to cover both GATT client and GATT server role.

* resolve or reject directly

Comment 13

[Ben Tian (inactive)]

Comment on attachment 8656953 [details] [diff] [review]
Bug 1181482 - Patch2(v2): Revise read/write characteristic/descriptor value to cover both GATT client and GATT server role.

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

LGTM.

Comment 14

[Ben Tian (inactive)]

Comment on attachment 8656397 [details] [diff] [review]
Bug 1181482 - Patch3: Implement |sendResponse| and BluetoothGattAttributeEvent for GATT server read/write requests.

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

r=me with comment addressed.

::: dom/bluetooth/bluedroid/BluetoothGattManager.cpp
@@ +330,5 @@
>                int aConnId) const
>    {
>      return aClient->mConnId == aConnId;
>    }
> +  bool Equals(const nsRefPtr<BluetoothGattServer>& aServer,

nit: newline after }

@@ +2253,5 @@
> +  }
> +
> +  void SendResponse() override
> +  {
> +    if (!mServer->mSendResponseRunnable) {

Conform with |OnError|. Make them both or neither guardian clauses .

@@ +2305,5 @@
> +    DispatchReplyError(aRunnable, STATUS_NOT_READY);
> +    return;
> +  }
> +
> +  if (server->mSendResponseRunnable) {

Check |server->mSendResponseRunnable| before |!connId| to save hash search effort if |server->mSendResponseRunnable| is nullptr.

@@ +3463,5 @@
> +  nsRefPtr<BluetoothGattServer> server = (*sServers)[index];
> +
> +  // Send an error response for unsupported requests
> +  if (aIsPrepareWrite || aOffset > 0) {
> +    BT_LOGR("Unsupported prepare write or long attribute read/write requests");

I think it's "write requests" only since this is |RequestWriteNotification|.

::: dom/bluetooth/common/webapi/BluetoothGattAttributeEvent.cpp
@@ +13,5 @@
> +#include "mozilla/dom/Nullable.h"
> +#include "mozilla/dom/PrimitiveConversions.h"
> +#include "mozilla/dom/TypedArray.h"
> +#include "mozilla/dom/bluetooth/BluetoothGattCharacteristic.h"
> +#include "mozilla/dom/bluetooth/BluetoothGattDescriptor.h"

nit: alphabetical order.

@@ +102,5 @@
> +  ErrorResult& aRv)
> +{
> +  nsCOMPtr<EventTarget> owner = do_QueryInterface(aGlobal.GetAsSupports());
> +
> +  nsRefPtr<BluetoothGattAttributeEvent> e =

Check whether we can simplify here by leveraging the other constructor to assign common member variables. Revise accordingly if it's doable.

@@ +110,5 @@
> +  e->mAddress = aEventInitDict.mAddress;
> +  e->mRequestId = aEventInitDict.mRequestId;
> +  e->mCharacteristic = aEventInitDict.mCharacteristic;
> +  e->mDescriptor = aEventInitDict.mDescriptor;
> +

Assign |e->mNeedResponse|.

@@ +158,5 @@
> +  JSContext* cx, JS::MutableHandle<JSObject*> aValue, ErrorResult& aRv)
> +{
> +  if (!mValue) {
> +    mValue = ArrayBuffer::Create(cx, mRawValue.Length(), mRawValue.Elements());
> +

nit: remove this newline.

::: dom/bluetooth/common/webapi/BluetoothGattAttributeEvent.h
@@ +11,5 @@
> +#include "mozilla/ErrorResult.h"
> +#include "mozilla/dom/BindingUtils.h"
> +#include "mozilla/dom/BluetoothGattAttributeEventBinding.h"
> +#include "mozilla/dom/Event.h"
> +#include "mozilla/dom/bluetooth/BluetoothCommon.h"

nit: alphabetical order.

::: dom/bluetooth/common/webapi/BluetoothGattServer.cpp
@@ +168,5 @@
> +  bool needResponse = arr[3].value().get_bool();
> +  nsTArray<uint8_t> value;
> +  value = arr[4].value().get_ArrayOfuint8_t();
> +
> +

nit: remove extra newline.

@@ +171,5 @@
> +
> +
> +  // Find the target characteristic or descriptor from the given handle
> +  nsRefPtr<BluetoothGattCharacteristic> characteristic;
> +  nsRefPtr<BluetoothGattDescriptor> descriptor;

Assign nullptr as initial value.

@@ +173,5 @@
> +  // Find the target characteristic or descriptor from the given handle
> +  nsRefPtr<BluetoothGattCharacteristic> characteristic;
> +  nsRefPtr<BluetoothGattDescriptor> descriptor;
> +  for (uint32_t i = 0; i < mServices.Length(); i++) {
> +    for (uint32_t j = 0; j < mServices[i]->mCharacteristics.Length(); j++) {

Remember |mServices[i]->mCharacteristics| with a local variable inside outer loop. And simplify following code with it.

@@ +275,4 @@
>  {
>    mValid = false;
>    mServices.Clear();
>    mPendingService = nullptr;

nit: switch these two lines to group similar action.

  mValid = false;
  mPendingService = nullptr;
  mServices.Clear();
  mRequestMap.Clear();

@@ +761,5 @@
> +  } else {
> +    MOZ_ASSERT_UNREACHABLE(
> +      "There should be at least one characteristic or descriptor in the "
> +      "request data.");
> +    return nullptr;

Reject promise here instead of return nullptr only.

Comment 15

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1181482 - Patch3(v2): Implement |sendResponse| and BluetoothGattAttributeEvent for GATT server read/write requests. r=btian

address Comment 14.

Comment 16

[Jocelyn Liu [:jocelyn] [:joliu]]

Comment on attachment 8656953 [details] [diff] [review]
Bug 1181482 - Patch2(v2): Revise read/write characteristic/descriptor value to cover both GATT client and GATT server role.

Hi Boris,

Could you help to review my patch from a dom peer point of view?
Though only comments are added under dom/webidl, API behaviors are actually revised to cover the GATT server case.

Thanks,
Jocelyn

Comment 17

[Jocelyn Liu [:jocelyn] [:joliu]]

Comment on attachment 8658599 [details] [diff] [review]
Bug 1181482 - Patch3(v2): Implement |sendResponse| and BluetoothGattAttributeEvent for GATT server read/write requests. r=btian

Hi Boris,

This patch added and implemented |sendResponse| method and BluetoothGattAttributeEvent for handling reading/writing characteristic/descriptor requests in GATT server API.

The flow is as below:
 1) gecko receives notification of read/write request issued by remote GATT client from bluetooth daemon
 2) gecko notifies gaia the request by firing a BluetoothGattAttributeEvent.
 3) gaia handles the request and calls |sendResponse| to reply remote GATT client

This patch has been reviewed by one of our bluetooth peer, could you also help to review my patch from a dom peer point of view?

Thanks,
Jocelyn

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8656953 [details] [diff] [review]
Bug 1181482 - Patch2(v2): Revise read/write characteristic/descriptor value to cover both GATT client and GATT server role.

r=me

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8658599 [details] [diff] [review]
Bug 1181482 - Patch3(v2): Implement |sendResponse| and BluetoothGattAttributeEvent for GATT server read/write requests. r=btian

A few cycle collection issues here; we should add tests that would have failed (leaked) due to them (e.g. an event whose mValue holds a reference to that event):

1)  You need to mozilla::DropJSObjects(this) in unlink for the event.
2)  You need NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS in your traverse.  Otherwise your trace won't even be called.

You don't need to null out mValue in the destructor; you're going away anyway at that point.

The IDL should probably document that the passed-in ArrayBuffer will be copied by the event constructor, not stored by reference, right?

Is it expected that the typed array is created in the caller compartment in the mRawValue case, not the compartment of the event object in question?  That seems a little odd to me.

The GetValue function is a bit odd.  It makes a copy, then the caller makes another copy.  Would it not be better if GetValue returned a const reference to the array instead of making a copy?

r=me with those issues fixed.

Comment 20

[Jocelyn Liu [:jocelyn] [:joliu]]

Hi Boris,

Thanks a lot for your time, please see my questions inline below.

(In reply to Boris Zbarsky [:bz] from comment #19)
> Comment on attachment 8658599 [details] [diff] [review]
> Bug 1181482 - Patch3(v2): Implement |sendResponse| and
> BluetoothGattAttributeEvent for GATT server read/write requests. r=btian
> 
> A few cycle collection issues here; we should add tests that would have
> failed (leaked) due to them (e.g. an event whose mValue holds a reference to
> that event):
> 
> 1)  You need to mozilla::DropJSObjects(this) in unlink for the event.
> 2)  You need NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS in your
> traverse.  Otherwise your trace won't even be called.
> 

Got it, thanks for pointing this out.
I suppose this also applies to other places in our code base, should we open a follow-up bug for them or help to modify them in this patch?
[1] https://dxr.mozilla.org/mozilla-central/source/dom/media/eme/MediaKeyMessageEvent.cpp?from=MediaKeyMessageEvent.cpp&offset=0#34
[2] https://dxr.mozilla.org/mozilla-central/source/dom/media/eme/MediaEncryptedEvent.cpp#30
[3] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/common/webapi/BluetoothLeDeviceEvent.cpp?from=BluetoothLeDeviceEvent.h#35

> Is it expected that the typed array is created in the caller compartment in
> the mRawValue case, not the compartment of the event object in question? 
> That seems a little odd to me.
> 

I don't quite understand this.
I originally thought that our caller, the event target, will be in the same compartment as the constructed event object.
Could you explain that why they're in different compartments?
I'm not familiar with this part, please bear with my naive question.

And I saw all the ArrayBuffer in the event use cases (MediaKeyMessageEvent, MediaEncryptedEvent, BluetoothLeDeviceEvent) all created the typed array in the caller, should these use cases also be revised or there're differences between Media case and Bluetooth case?

Comment 21

[Boris Zbarsky [:bzbarsky]]

> I suppose this also applies to other places in our code base

Yes.  Separate bugs are probably better for the other classes.

> I originally thought that our caller, the event target, will be in the same compartment
> as the constructed event object.

No, the caller is not the event target.  The caller is whatever the caller is, probably an event listener or code it calls.

Let me illustrate with an example.  Say a BluetoothGattAttributeEvent with an mRawValue is dispatched to some event target in a web page.  There are two event listeners on the event target that get the event.  The first listener is a chrome function; the second listener is a function from the same web page as the event target.

If the chrome function does .value, the GetValue function will be called while in the chrome compartment (because the call is happening over an Xray wrapper to the event).  This will create the ArrayBuffer in the chrome compartment.  The second listener will then not be able to access any properties on the ArrayBuffer, since it will get an opaque security wrapper.

There are ways to get some similar effects without Xrays being involved, but they're a bit more complicated and less likely to arise in practice.

I just looked at MediaKeyMessageEvent and it has exactly the same problem, you're right.  That needs to get fixed.  Same for MediaEncryptedEvent and BluetoothLeDeviceEvent...  For the media events, looks like that bug was introduced in http://hg.mozilla.org/mozilla-central/rev/f505272bac50 (which I even reviewed, sigh).  Before that, the code correctly passed in a scope object as the second argument of Create() to ensure that the creation happened in the right compartment...

Again, separate bugs there would be good, and my apologies for missing it in that review.  :(

Comment 22

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Boris Zbarsky [:bz] from comment #21)
> > I suppose this also applies to other places in our code base
> 
> Yes.  Separate bugs are probably better for the other classes.
> 
> > I originally thought that our caller, the event target, will be in the same compartment
> > as the constructed event object.
> 
> No, the caller is not the event target.  The caller is whatever the caller
> is, probably an event listener or code it calls.
> 
> Let me illustrate with an example.  Say a BluetoothGattAttributeEvent with
> an mRawValue is dispatched to some event target in a web page.  There are
> two event listeners on the event target that get the event.  The first
> listener is a chrome function; the second listener is a function from the
> same web page as the event target.
> 
> If the chrome function does .value, the GetValue function will be called
> while in the chrome compartment (because the call is happening over an Xray
> wrapper to the event).  This will create the ArrayBuffer in the chrome
> compartment.  The second listener will then not be able to access any
> properties on the ArrayBuffer, since it will get an opaque security wrapper.
> 
> There are ways to get some similar effects without Xrays being involved, but
> they're a bit more complicated and less likely to arise in practice.
> 
> I just looked at MediaKeyMessageEvent and it has exactly the same problem,
> you're right.  That needs to get fixed.  Same for MediaEncryptedEvent and
> BluetoothLeDeviceEvent...  For the media events, looks like that bug was
> introduced in http://hg.mozilla.org/mozilla-central/rev/f505272bac50 (which
> I even reviewed, sigh).  Before that, the code correctly passed in a scope
> object as the second argument of Create() to ensure that the creation
> happened in the right compartment...
> 
> Again, separate bugs there would be good, and my apologies for missing it in
> that review.  :(

Got it, thanks a lot for your detailed explanation.
I will open follow-up bugs for calling |mozilla::DropJSObjects(this)| in event unlink and the compartment part.

Another question here, 
"We should add tests that would have failed (leaked) due to them (e.g. an event whose mValue holds a reference to that event)."
I'm not sure what kind of tests you were referring to, is it like tests under dom/event/tests?
Sadly I have no idea how to write this test, could you please point me a direction, probably?
BTW, can this be done in a follow-up bug?

Thanks,
Jocelyn

Comment 23

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: [Final] Bug 1181482 - Patch1: Refine some data types in gecko backend for GATT server read/write request APIs. r=btian

* convert to hg format and add reviewer's name.

Comment 24

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: [Final] Bug 1181482 - Patch2: Revise read/write characteristic/descriptor value to cover both GATT client and GATT server role. r=btian, r=bz

* convert to hg format and add reviewer's name

Comment 25

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: Bug 1181482 - Patch3(v3): Implement |sendResponse| and BluetoothGattAttributeEvent for GATT server read/write requests. r=btian, r=bz

* add NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS in GattAttrEvt's traverse
* add mozilla::DropJSObjects(this); in GattAttrEvt's unlink
* remove unnecessary null-out in GattAttrEvt's destructor
* pass |this| when creating ArrayBuffer in |BluetoothGattAttributeEvent::GetValue|.
* Revise |BluetoothGattCharacteristic::GetValue| and |BluetoothGattDescriptor::Value| to return const reference.  
* convert to hg format and add reviewer's name

Comment 26

[Jocelyn Liu [:jocelyn] [:joliu]]

I'm afraid you might missed my question in Comment 22 since I uploaded three patches after that.
Use ni just in case you missed it. ;)

Comment 27

[Boris Zbarsky [:bzbarsky]]

> is it like tests under dom/event/tests

Yes.  Or wherever the tests for this functionality live.

> could you please point me a direction, probably?

Sure.  Given a BluetoothGattAttributeEvent "ev", do this:

  ev.value.foo = ev;

in the test and verify that this leaks with the version of the patch that does not have the NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS bit and doesn't leak with the version that does have it.

> BTW, can this be done in a follow-up bug?

I think the test should land as part of this bug.  That should not be a big burden, and it will let us determine whether the leak is in fact fixed.

Comment 28

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Boris Zbarsky [:bz] from comment #27)
> > is it like tests under dom/event/tests
> 
> Yes.  Or wherever the tests for this functionality live.
> 
> > could you please point me a direction, probably?
> 
> Sure.  Given a BluetoothGattAttributeEvent "ev", do this:
> 
>   ev.value.foo = ev;
> 
> in the test and verify that this leaks with the version of the patch that
> does not have the NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS bit and
> doesn't leak with the version that does have it.
> 

Another super naive question here, may I ask how to determine if it leaks or not in the test?

> > BTW, can this be done in a follow-up bug?
> 
> I think the test should land as part of this bug.  That should not be a big
> burden, and it will let us determine whether the leak is in fact fixed.

Understood, will do, thanks.

Comment 29

[Boris Zbarsky [:bzbarsky]]

> may I ask how to determine if it leaks or not in the test?

Run the test in a debug build, and see whether the harness says it failed for leaks.

Comment 30

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: [Final] Bug 1181482 - Patch1: Refine some data types in gecko backend for GATT server read/write request APIs. r=btian

- rebase

Comment 31

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: [Final] Bug 1181482 - Patch2: Revise read/write characteristic/descriptor value to cover both GATT client and GATT server role. r=btian, r=bz

- rebase

Comment 32

[Jocelyn Liu [:jocelyn] [:joliu]]

Attachment: [Final] Bug 1181482 - Patch3: Implement |sendResponse| and BluetoothGattAttributeEvent for GATT server read/write requests. r=btian, r=bz

- rebase
- fix memcpy condition
- remove NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS per Bug 1203900 Comment 2
- no test is added since there's no leak, see Bug 1203900 Comment 4

Comment 33

[Jocelyn Liu [:jocelyn] [:joliu]]

(In reply to Jocelyn Liu [:jocelyn] [:joliu] from comment #32)
> Created attachment 8664170 [details] [diff] [review]
> [Final] Bug 1181482 - Patch3: Implement |sendResponse| and
> BluetoothGattAttributeEvent for GATT server read/write requests. r=btian,
> r=bz
> 
> - rebase
> - fix memcpy condition
should be memcmp, sorry
> - remove NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS per Bug 1203900
> Comment 2
> - no test is added since there's no leak, see Bug 1203900 Comment 4

Comment 34

[Jocelyn Liu [:jocelyn] [:joliu]]

try looks OK: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aaf5d1bda3d8

Comment 35

[Pulsebot]

https://hg.mozilla.org/integration/b2g-inbound/rev/432eb9253252
https://hg.mozilla.org/integration/b2g-inbound/rev/9ed155901d76
https://hg.mozilla.org/integration/b2g-inbound/rev/a10d1b694b6b

Comment 36

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

https://hg.mozilla.org/mozilla-central/rev/432eb9253252
https://hg.mozilla.org/mozilla-central/rev/9ed155901d76
https://hg.mozilla.org/mozilla-central/rev/a10d1b694b6b