HalTypes's LightMode and FlashMode enum serializers probably don't work




Hardware Abstraction Layer (HAL)
6 years ago
6 years ago


(Reporter: Justin Lebar (not reading bugmail), Assigned: shelly)



Firefox Tracking Flags

(Not tracked)


(Whiteboard: [mentor=cyu])


(1 attachment)



6 years ago
Filing a bug based entirely on code inspection is risky, so forgive me if I'm wrong.  But I don't think the LightMode enum serializer in HalTypes.cpp will work.

> enum LightMode {
>     eHalLightMode_User = 0,       // brightness is managed by user setting
>     eHalLightMode_Sensor = 1      // brightness is managed by a light sensor
> };

> template <>
> struct ParamTraits<mozilla::hal::LightMode>
>   : public EnumSerializer<mozilla::hal::LightMode,
>                           mozilla::hal::eHalLightMode_User,
>                           mozilla::hal::eHalLightMode_Sensor>

And in ipc/glue/IPCMessageUtils.h:

> template <typename E, E smallestLegal, E highBound>
> struct EnumSerializer {
>   typedef E paramType;
>   static bool IsLegalValue(const paramType &aValue) {
>     return smallestLegal <= aValue && aValue < highBound;
>   }

Notice that highBound is not a valid value.  In this case, that means that the serializer should reject eHalLightMode_Sensor.

Comment 1

6 years ago
Same for the FlashMode serializer.
Summary: HalTypes's LightMode enum serializer probably doesn't work → HalTypes's LightMode and FlashMode enum serializers probably don't work

Comment 2

6 years ago
I filed bug 779156 for the WakeLockControl enum serializer, which has the same problem.
Assignee: nobody → slin
Whiteboard: [mentor=cyu]

Comment 3

6 years ago
Fixed in bug 782460.
Last Resolved: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 782460

Comment 4

6 years ago
Created attachment 655856 [details] [diff] [review]
Add counters to enum types and test case - v1.

Added counters to the enum of LightMode and FlashMode, and added test case.
Attachment #655856 - Flags: review?(justin.lebar+bug)

Comment 6

6 years ago
Oh, I was imaginging that this test checked the functionality at a higher level than the enums themselves.  I don't think we need a testcase for this -- or, if we did want to test it, we could statically assert it, right?

That is, I think you ought to be able to do

MOZ_STATIC_ASSERT(!IPC::ParamTraits<mozilla::hal::LightMode>::IsLegalValue(mozilla::hal::eHalLightMode_Sensor), "eHalLightMode_Sensor in range");

And then the assertion would catch mistakes at compile-time, instead of test-time.

But if we wanted do add static assertions, I think a cleaner way to do it would be to restructure these enum definitions as a variadic macro, so it's impossible to make this mistake anymore.  That might be worthwhile, but should be a separate bug.


6 years ago
Attachment #655856 - Flags: review?(justin.lebar+bug) → review-

Comment 7

6 years ago
Thanks for your suggestion, but I looked it up and found out static_alert takes only constant-expression as its first parameter, so it might not work in this case. I think the fix is very good enough.

Comment 8

6 years ago
> I looked it up and found out static_alert takes only constant-expression as its first parameter

Ah, you're right, that wouldn't work.  Sorry, I should have looked closer!
You need to log in before you can comment on or make changes to this bug.