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

Status: RESOLVED DUPLICATE of bug 782460

(Core :: Hardware Abstraction Layer (HAL), defect)

Description

[Justin Lebar (not reading bugmail)]

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

[Justin Lebar (not reading bugmail)]

Same for the FlashMode serializer.

Comment 2

[Justin Lebar (not reading bugmail)]

I filed bug 779156 for the WakeLockControl enum serializer, which has the same problem.

Comment 3

[Justin Lebar (not reading bugmail)]

Fixed in bug 782460.

Comment 4

[Shelly Lin [:shelly]]

Attachment: Add counters to enum types and test case - v1.

Added counters to the enum of LightMode and FlashMode, and added test case.

Comment 5

[Shelly Lin [:shelly]]

Try : https://tbpl.mozilla.org/?tree=Try&rev=50fd1624c5a1

Comment 6

[Justin Lebar (not reading bugmail)]

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.

Comment 7

[Shelly Lin [:shelly]]

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

[Justin Lebar (not reading bugmail)]

> 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!