Add sentinel to WR enums that go over IPC

RESOLVED FIXED in mozilla54

Status

()

Core
Graphics: WebRender
P3
normal
RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: kats, Assigned: kats)

Tracking

(Blocks: 1 bug)

53 Branch
mozilla54
Points:
---

Firefox Tracking Flags

(firefox53 affected)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 attachment)

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.
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.
(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.
Assignee: nobody → bugmail
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.
(... 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

11 months ago
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.
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.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=355359e000b7af6b0d14f76600ccd2eec98bf3cf
Created attachment 8828924 [details] [diff] [review]
Add sentinel values and consistently use enum classes
Attachment #8828924 - Flags: review?(graphics-team)

Comment 9

11 months ago
Pushed by kgupta@mozilla.com:
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?
Status: NEW → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
(Assignee)

Updated

11 months ago
See Also: → bug 1332737
(Assignee)

Updated

10 months ago
Target Milestone: --- → mozilla54
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.
Attachment #8828924 - Flags: review?(graphics-team) → review+
You need to log in before you can comment on or make changes to this bug.