Closed Bug 1181482 Opened 4 years ago Closed 4 years ago

Implement GATT Server read / write request

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(firefox44 fixed)

RESOLVED FIXED
FxOS-S8 (02Oct)
Tracking Status
firefox44 --- fixed

People

(Reporter: brsun, Assigned: yrliou)

References

Details

Attachments

(3 files, 8 obsolete files)

6.92 KB, patch
Details | Diff | Splinter Review
4.64 KB, patch
Details | Diff | Splinter Review
51.38 KB, patch
Details | Diff | Splinter Review
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
Blocks: 1181483
Assignee: nobody → joliu
* use uint16_t for |SendResponse| since application might want to use their own error status code
* replace int by BluetoothAttributeHandle for handles
Attachment #8656394 - Flags: review?(btian)
* implement requestread/write notifications and fire attributereadreq/attributewritereq events
* implement |sendResponse|
Attachment #8656397 - Flags: review?(btian)
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.
Attachment #8656394 - Flags: review?(btian) → review+
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)
Attachment #8656395 - Flags: review?(btian)
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.
(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
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!
> 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.
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 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.
Attachment #8656953 - Attachment is patch: true
Attachment #8656953 - Attachment mime type: application/mbox → text/plain
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.
Attachment #8656953 - Flags: review?(btian) → review+
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.
Attachment #8656397 - Flags: review?(btian) → review+
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
Attachment #8656953 - Flags: review?(bzbarsky)
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
Attachment #8658599 - Flags: review?(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
Attachment #8656953 - Flags: review?(bzbarsky) → review+
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.
Attachment #8658599 - Flags: review?(bzbarsky) → review+
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?
> 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.  :(
(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
* convert to hg format and add reviewer's name.
Attachment #8656394 - Attachment is obsolete: true
* 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
Attachment #8658599 - Attachment is obsolete: true
See Also: → 1203900
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. ;)
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
(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.
> 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.
- 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
Attachment #8659824 - Attachment is obsolete: true
(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
Depends on: 1236724
You need to log in before you can comment on or make changes to this bug.