1334938 - stylo: Stylo is effectively disabled even in a stylo build

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

This is because of a change in bug 1232696.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1334938 - Re-enable stylo in stylo builds.

Review commit: https://reviewboard.mozilla.org/r/108102/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/108102/

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8831575 [details]
Bug 1334938 - Re-enable stylo in stylo builds.

https://reviewboard.mozilla.org/r/108102/#review109170

Thanks for finding and fixing this :)

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c423b8c04c1d
Re-enable stylo in stylo builds. r=emilio

Comment 4

[Julian Seward [:jseward]]

Sorry about that.  Just as a note, the original formulation had

  enum class StyleBackendType : uint8_t
  {
    Gecko = 1,
    Servo
  };

but the ZEROING_NEW initialised mStyleBackendType to zero.  I believe that
compilers are allowed to assume that any enum value loaded from memory is a
legitimate value of the type, but that was not the case here -- neither has
value zero.  So it's undefined behaviour in C++.  This is also why I couldn't
make a transparent replacement initialisation -- there was no zero value for
the enum.

Comment 5

[Cameron McCormack (:heycam)]

My understanding was that any value in the range supported by the underlying storage type is valid, and the compiler shouldn't treat unnamed values as undefined.  (Otherwise the common pattern of defining individual bits with names, and then combining them into bitfields, would break, I suppose.)  Xidorn has added a name for zero to StyleBackendType now, although in my defence I sometimes like to used an unnamed value to represent "not yet initialized" so that you can do lazy value initialization like this, and also not run into compiler warnings/errors with switch statements about not handling the value representing "not yet initialized".

Comment 6

[Julian Seward [:jseward]]

(In reply to Cameron McCormack (:heycam) (away 27 Jan–1 Feb) from comment #5)
Ah, you may be right.  I discovered that UBSan flags loads of enum fields that
are not "in range", but (1) I am not sure what the _exact_ semantics of UBSan's
checks are, and (2) I simply assumed that because UBSan flags them as suspicious,
the standard says they are not allowed.  But that might not be the case.

Sorry for the noise (and breakage).

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

I got an answer from a person familiar with C++ standard, and the answer is that this is not UB.

The standard says:
For an enumeration whose underlying type is fixed, the values of the enumeration are the values of the underlying type. (7.2/8)

(It is an UB in Rust, though)

In general, I'd prefer to give every value we use an explicit name (bit flag should probably be replaced by bitfields, although there are some defect...) That would make things clearer, and allow compiler to perform better checking. Also that would make it easier for code readers to understand the state available. That's why I added that value.

I'm unsure about the name, though. I guess None is probably not the best name... Probably something like Uninitialized or Undecided might be better... but I don't like long names :)

Comment 8

[Carsten Book [:Tomcat]]

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