1152702 - [bluetooth2] Fix upper bound in ContiguousEnumSerializers for BluetoothStatus and BluetoothSspVariant

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

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

Currently we pass the last valid value in our enums into ContiguousEnumSerializers as the upper bound, which is incorrect.
For example, we will hit |MOZ_ASSERT(EnumValidator::IsLegalValue(aValue))| if we try to send STATUS_RMT_DEV_DOWN through IPC.

This bug is to add a NUM_* in these enums as the upper bound.

Comment 1

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

Attachment: Bug 1152702 - Fix upper bounds of ContiguousEnumSerializer for BluetoothStatus and BluetoothSspVariant.

Hi Thomas,

Could you help review my patch?

Thanks,
Jocelyn

Comment 2

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8590163 [details] [diff] [review]
Bug 1152702 - Fix upper bounds of ContiguousEnumSerializer for BluetoothStatus and BluetoothSspVariant.

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

::: dom/bluetooth2/BluetoothCommon.h
@@ +238,5 @@
>    STATUS_PARM_INVALID,
>    STATUS_UNHANDLED,
>    STATUS_AUTH_FAILURE,
> +  STATUS_RMT_DEV_DOWN,
> +  NUM_STATUS

I don't know the plural of 'status'.

@@ +281,5 @@
>    SSP_VARIANT_PASSKEY_CONFIRMATION,
>    SSP_VARIANT_PASSKEY_ENTRY,
>    SSP_VARIANT_CONSENT,
> +  SSP_VARIANT_PASSKEY_NOTIFICATION,
> +  NUM_SSP_VARIANT

Maybe call it 'NUM_SSP_VARIANTS'

@@ +449,5 @@
>  enum BluetoothObjectType {
>    TYPE_MANAGER = 0,
>    TYPE_ADAPTER = 1,
>    TYPE_DEVICE = 2,
> +  NUM_TYPE

and 'NUM_TYPES'.

Comment 3

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

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #2)
> Comment on attachment 8590163 [details] [diff] [review]
> Bug 1152702 - Fix upper bounds of ContiguousEnumSerializer for
> BluetoothStatus and BluetoothSspVariant.
> 
> Review of attachment 8590163 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/bluetooth2/BluetoothCommon.h
> @@ +238,5 @@
> >    STATUS_PARM_INVALID,
> >    STATUS_UNHANDLED,
> >    STATUS_AUTH_FAILURE,
> > +  STATUS_RMT_DEV_DOWN,
> > +  NUM_STATUS
> 
> I don't know the plural of 'status'.
> 
> @@ +281,5 @@
> >    SSP_VARIANT_PASSKEY_CONFIRMATION,
> >    SSP_VARIANT_PASSKEY_ENTRY,
> >    SSP_VARIANT_CONSENT,
> > +  SSP_VARIANT_PASSKEY_NOTIFICATION,
> > +  NUM_SSP_VARIANT
> 
> Maybe call it 'NUM_SSP_VARIANTS'
> 
> @@ +449,5 @@
> >  enum BluetoothObjectType {
> >    TYPE_MANAGER = 0,
> >    TYPE_ADAPTER = 1,
> >    TYPE_DEVICE = 2,
> > +  NUM_TYPE
> 
> and 'NUM_TYPES'.

Hi Thomas,

I'm not sure that we should use plural nouns here.
According to the comment of ContiguousEnumSerializer[1], it looks like we shouldn't use plural nouns?

[1] https://dxr.mozilla.org/mozilla-central/source/ipc/glue/IPCMessageUtils.h#164

Thanks,
Jocelyn

Comment 4

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Oh, OK. No problem.

Comment 5

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

Attachment: [Final] Bug 1152702 - Fix upper bounds of ContiguousEnumSerializer for BluetoothStatus and BluetoothSspVariant. r=tzimmermann

Thanks!

Comment 6

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

No try server result since dom/bluetooth/bluetooth2 won't be built.

Comment 7

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

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

Comment 8

[Ryan VanderMeulen (PTO, back July 27)]

https://hg.mozilla.org/mozilla-central/rev/013338161940