Closed Bug 1334938 Opened 3 years ago Closed 3 years ago

stylo: Stylo is effectively disabled even in a stylo build

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(1 file)

This is because of a change in bug 1232696.
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 :)
Attachment #8831575 - Flags: review?(emilio+bugs) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c423b8c04c1d
Re-enable stylo in stylo builds. r=emilio
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.
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".
(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).
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 :)
https://hg.mozilla.org/mozilla-central/rev/c423b8c04c1d
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.