1325627 - Add sentinel to WR enums that go over IPC

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P3)

Description

[Kartikaya Gupta (email:[e-mail])]

WebRenderTypes.h has IPC serializations for enums in webrender.h. However the enums don't have sentinel values at the end, so the last "real" value is used in the IPC serialization definitions, e.g. at https://hg.mozilla.org/projects/graphics/file/7b88f8d64a95/gfx/layers/wr/WebRenderTypes.h#l89. This means that if somebody adds a new value to the end of the enum they might forget to update the IPC serialization, causing hard-to-track-down errors.

Comment 1

[Botond Ballo [:botond]]

There is a downside to adding a sentinel value, as Seth pointed out in bug 1266301: it prevents the compiler from giving accurate exhaustiveness warnings for switch statements over the enum. I don't have a great solution for this, just pointing it out so we're aware of the tradeoffs.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

(In reply to Botond Ballo [:botond] from comment #1)
> There is a downside to adding a sentinel value, as Seth pointed out in bug
> 1266301: it prevents the compiler from giving accurate exhaustiveness
> warnings for switch statements over the enum.

This can be worked around by putting the sentinel in the switch and just MOZ_RELEASE_ASSERT(false)'ing it. It's not perfect but we do it in a bunch of places already.

This bug should be fixed ASAP because we're actually doing the wrong thing now, like creating ContiguousEnumSerializer with valid values as the third template argument.

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Also WrTextureFilter is defined as an "enum class" inside a "extern C" header, which is probably not good, as the compiler may do things that make it incompatible with rust's C bindings.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

(... but making it a regular enum means "Point" is in the global namespace and it causes compilation failures in faraway places that are trying to use gfx::Point)

Comment 5

[Ryan Hunt [:rhunt] (on leave until early May)]

Hm. I don't know what the best solution is.

Would declaring it |enum WrTextureFilter : int|, force it to be compatible with C? Or maybe char because we only have two values, so I don't think the enum would be that big. Either way that is kind of messy.

I'd be fine with making it a normal enum and prefixing the enum values with 'WR_TEXFILTER_' or however we normally do it in Gecko.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Botond said that "enum class" *should* in practice be the same size as "enum", so I think it might be easier to convert them all to "enum class" (to avoid namespacing ugliness) and then add a static_assert inside an #ifdef DEBUG block to make sure the sizes wind up the same. I'm working on a patch that does that now.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=355359e000b7af6b0d14f76600ccd2eec98bf3cf

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Attachment: Add sentinel values and consistently use enum classes

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/f20aca8fdea2
Ensure all serialized Wr* enums have sentinel values and are consistently defined as |enum class|. r=gfx?

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8828924 [details] [diff] [review]
Add sentinel values and consistently use enum classes

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

I'm just going to r+ this since a lot of people have looked at and further modified the code in question since this landed, and nobody seems to be keeping an eye on review requests to :gfx.