1612301 - Hit MOZ_CRASH(assertion failed: `(left == right)` left: `Length`, right: `Calc`) at servo/components/style/values/computed/length_percentage.rs:238 on big endian machine

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Petr Sumbera]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:72.0) Gecko/20100101 Firefox/72.0

Steps to reproduce:

I see this on Solaris SPARC (64bits) with latest Firefox sources:

Hit MOZ_CRASH(assertion failed: (left == right)
left: Length,
right: Calc) at servo/components/style/values/computed/length_percentage.rs:238

Comment 1

[Petr Sumbera]

/// Private version of new_calc() that constructs a calc() variant without
    /// checking.
    fn new_calc_unchecked(calc: Box<CalcLengthPercentage>) -> Self {
        let ptr = Box::into_raw(calc);
        let calc = Self(LengthPercentageUnion {
            calc: CalcVariant {
                #[cfg(target_pointer_width = "32")]
                tag: LengthPercentageUnion::TAG_CALC,
                ptr,
            }
        });
        debug_assert_eq!(calc.tag(), Tag::Calc);          <=== !!!
        calc
    }

This was added via Bug 1607049 .

Emilio can you please have look at this?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Hmm, so this is on a big-endian machine, right? I suspect our union trickery doesn't work for big-endian machines and we need to figure out something else...

Do you know how can I test this / emulate this? I don't have the right hardware.

Comment 3

[Petr Sumbera]

Yes, it's big-endian machine. I can just offer you to run some isolated tests on your requests (some simple Rust test code?).

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Standalone rust-test-case.

This should reproduce the issue. Haven't tried to make it work on big-endian yet.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Attachment: Potentially fixed code.

Does this work for you? I'd like to fix it in a better way (so that it only takes 64 bits rather than growing the thing), but just confirming that you can repro this and that the problem goes away like this.

Comment 6

[Petr Sumbera]

I haven't looked into your code. But the second version seems to be fine.

$: ./reproduce
thread 'main' panicked at 'assertion failed: `(left == right)`
  left: `Length`,
 right: `Calc`', reproduce.rs:149:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
$ ./fixed
$

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Potentially better fix.

Does this work?

Comment 8

[Petr Sumbera]

Yes. No error output.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Ok, I'll try to clean it up and send it for review tomorrow or sometime next week, probably.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Err, sorry, that was supposed to be for me.

Comment 11

[Petr Sumbera]

Thank you! I can also confirm that with your changes applied to Firefox tree (see below) I no longer see the assertion (though at the moment I'm hitting probably another bug - Bug 1612503).

--- a/servo/components/style/values/computed/length_percentage.rs       Wed Jan 29 15:38:48 2020 +0100
+++ b/servo/components/style/values/computed/length_percentage.rs       Fri Jan 31 10:24:54 2020 +0100
@@ -59,7 +59,7 @@
 #[doc(hidden)]
 #[derive(Copy, Clone)]
 #[repr(C)]
-#[cfg(target_pointer_width = "32")]
+#[cfg(any(target_endian = "big", target_pointer_width = "32"))]
 pub struct CalcVariant {
     tag: u32,
     ptr: *mut CalcLengthPercentage,
@@ -68,7 +68,7 @@
 #[doc(hidden)]
 #[derive(Copy, Clone)]
 #[repr(C)]
-#[cfg(target_pointer_width = "64")]
+#[cfg(all(target_endian = "little", target_pointer_width = "64"))]
 pub struct CalcVariant {
     ptr: *mut CalcLengthPercentage,
 }
@@ -127,15 +127,6 @@
     Percentage = LengthPercentageUnion::TAG_PERCENTAGE,
 }

-// All the members should be 64 bits, even in 32-bit builds.
-#[allow(unused)]
-unsafe fn static_assert() {
-    std::mem::transmute::<u64, LengthVariant>(0u64);
-    std::mem::transmute::<u64, PercentageVariant>(0u64);
-    std::mem::transmute::<u64, CalcVariant>(0u64);
-    std::mem::transmute::<u64, LengthPercentage>(0u64);
-}
-
 impl Drop for LengthPercentage {
     fn drop(&mut self) {
         if self.tag() == Tag::Calc {
@@ -230,7 +221,7 @@
         let ptr = Box::into_raw(calc);
         let calc = Self(LengthPercentageUnion {
             calc: CalcVariant {
-                #[cfg(target_pointer_width = "32")]
+                #[cfg(any(target_endian = "big", target_pointer_width = "32"))]
                 tag: LengthPercentageUnion::TAG_CALC,
                 ptr,
             }

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1612301 - Fix LengthPercentage on big-endian machines. r=jwatt,#style

Always store the pointer in little-endian order so that the tag trick works.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Can you confirm that the patch I submitted works for you?

Comment 14

[Emilio Cobos Álvarez (:emilio)]

(I wonder if we also need to store the tag in big-endian order... Otherwise you'd hit assertions I guess...)

Comment 15

[Petr Sumbera]

I can confirm that the submitted patch works. I also don't see Bug 1612503. So it must had been related (I will close it now). Thank you!

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4412f2791565
Fix LengthPercentage on big-endian machines. r=jfkthame

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1612301 - Add some comments as per jwatt's request.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b70256aaf4ba
Add some comments as per jwatt's request.

Comment 20

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/4412f2791565

Comment 21

[Narcis Beleuzu [:NarcisB]]

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