1382190 - Stylo: multiple nsStyle* crashes when starting firefox on Windows 32-bit

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Ekanan Ketunuti]

This bug was filed from the Socorro interface and is 
report bp-7ead63df-1d99-414b-9867-7678c0170719.
=============================================================


I get crash when start firefox with stylo enabled. If it doesn't crash immediately, type something in urlbar.

There were multiple crash signatures. 
bp-0fd2eb09-96cd-418e-a282-146cc0170719
bp-d3b739a8-b32b-4ccf-8118-4b8ea0170719
bp-adab7a65-569e-4fd6-955c-1b7f50170719

Comment 1

[Marcia Knous [:marcia]]

Likely dupe of Bug 1382019, even though the signatures are different.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think this is worse in today's build than yesterday's -- these crashes also look like they might be 32-bit only?  Basically on 32-bit Windows today's nightly basically can't load any page with stylo enabled.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

The appears to a separate crash from bug 1382019, and it only seems to appear on Windows 32-bit.

I tested the latest Nightly, which includes the fix for bug 1382019 (that's the last commit used to build it).

Manish, can you investigate here?

Comment 4

[Manish Goregaokar [:manishearth]]

I suspect ServoFontComputationData is wrong on 32 bit.

We've picked the size and alignment manually so that it matches Rust (the bindgen tests keep this robust). But I chose these on 64 bit and the sizes may differ on 32 bit.

If the size/alignment is off by 4, that would make each style struct become a different one (but still have within-bounds pointers), leading to segfaults within methods on the style structs (but not on accessing the structs themselves)

Comment 5

[Manish Goregaokar [:manishearth]]

My hunch was correct. I have a bunch of failing layout tests. (Also some failing sizeof tests, but that's ok, we hardcode those numbers for 64 bit).

The important failure is

---- bindings::root::mozilla::bindgen_test_layout_ServoStyleContext stdout ----
	thread 'bindings::root::mozilla::bindgen_test_layout_ServoStyleContext' panicked at 'assertion failed: `(left == right)` (left: `156`, right: `152`): Size of: ServoStyleContext', /home/manishearth/mozilla/servo/target/i686-unknown-linux-gnu/debug/build/style-57dc208bd78e0740/out/gecko/structs_debug.rs:7676


If I remove the whole size check, I get 


---- bindings::root::bindgen_test_layout_ServoComputedValues stdout ----
        thread 'bindings::root::bindgen_test_layout_ServoComputedValues' panicked at 'assertion failed: `(left == right)` (left: `112`, right: `108`): Alignment of field: ServoComputedValues::rules', /home/manishearth/mozilla/servo/target/i686-unknown-linux-gnu/debug/build/style-57dc208bd78e0740/out/gecko/structs_debug.rs:21131

which means that the FontComputationData field (right before the rules field) is 4 bytes larger than it should be.


There are two ways to fix this. One is to hardcode FontComputationData as smaller on 32 bit. This is an easy fix. The other is to replace it with a (bool, KeywordSize, f32), or perhaps use an enum that is KeywordSize with a None variant. This will make the Rust code uglier since we can't nicely use match anymore, but it should eliminate the brittleness of guessing the size and layout of an Option<(enum, f32)> and relying on layout tests to catch issues.



----

All of the layout failures can be found in https://gist.github.com/Manishearth/1794ba7268d7f23cd3dc09bb42109a5f. I suspect ServoStyleContext is the only thing that actually gets used on the Servo side, so the rest haven't caused crashes so far. Emilio, mind looking into these? The rest are probably bindgen bugs since they don't sound like something we meddle with as we do with ServoComputedValues.

Comment 6

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1382190 - servo: Move FontComputationData to the end of ServoComputedValues to make size check easier, make it NonZero ;

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

Comment 7

[Manish Goregaokar [:manishearth]]

FWIW we don't seem to use the int value of KeywordSize (we convert from, not to), so I set to start at 1. This way `Option<KeywordSize>` flattens out because of NonZero and becomes `(enum, f32)`.

Comment 8

[Manish Goregaokar [:manishearth]]

Comment on attachment 8888122 [details]
Bug 1382190 - servo: Move FontComputationData to the end of ServoComputedValues to make size check easier, make it NonZero ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159012/diff/1-2/

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8888122 [details]
Bug 1382190 - servo: Move FontComputationData to the end of ServoComputedValues to make size check easier, make it NonZero ;

https://reviewboard.mozilla.org/r/159012/#review164422

::: commit-message-62b19:2
(Diff revision 2)
> +Bug 1382190 - servo: Move FontComputationData to the end of ServoComputedValues to make size check easier, make it NonZero ; r?emilio
> +

Please describe in the commit message the rationale for this.

::: layout/style/ServoTypes.h:140
(Diff revision 2)
>  struct ServoWritingMode {
>    uint8_t mBits;
>  };
>  
>  struct ServoFontComputationData {
> -  // 8 bytes, but is done as 4+4 for alignment
> +  uintptr_t mBits;

Wouldn't this fail the alignment test in 64bit?

::: layout/style/ServoTypes.h:217
(Diff revision 2)
>    /// relevant link for this element. A element's "relevant link" is the
>    /// element being matched if it is a link or the nearest ancestor link.
>    mozilla::ServoVisitedStyle visited_style;
> -  mozilla::ServoComputedValueFlags flags;
>  
> +  mozilla::ServoFontComputationData font_computation_data;

Add a comment about this being the last member, etc...

Comment 10

[Manish Goregaokar [:manishearth]]

Comment on attachment 8888122 [details]
Bug 1382190 - servo: Move FontComputationData to the end of ServoComputedValues to make size check easier, make it NonZero ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159012/diff/2-3/

Comment 11

[Manish Goregaokar [:manishearth]]

Pushed up an older patch, my bad

Comment 12

[Manish Goregaokar [:manishearth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=18853d7998cf4255babcc8befff5fbf7e559aa0f

Comment 13

[Emilio Cobos Álvarez (:emilio)]

(In reply to Manish Goregaokar [:manishearth] from comment #11)
> Pushed up an older patch, my bad

You seem to have pushed the same patch again? It's totally unchanged...

Comment 14

[Manish Goregaokar [:manishearth]]

Comment on attachment 8888122 [details]
Bug 1382190 - servo: Move FontComputationData to the end of ServoComputedValues to make size check easier, make it NonZero ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159012/diff/3-4/

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8888122 [details]
Bug 1382190 - servo: Move FontComputationData to the end of ServoComputedValues to make size check easier, make it NonZero ;

Cancelling review for now since it's the same patch (or maybe mozreview broke?).

I'm putting back the NI? request to see if there's some fix in the bindgen side that can be taken for those.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8888122 [details]
Bug 1382190 - servo: Move FontComputationData to the end of ServoComputedValues to make size check easier, make it NonZero ;

https://reviewboard.mozilla.org/r/159012/#review164434

::: servo/components/style/properties/longhand/font.mako.rs:624
(Diff revision 4)
> -        XXSmall = 0,
> -        XSmall = 1,
> -        Small = 2,
> -        Medium = 3,
> -        Large = 4,
> -        XLarge = 5,
> -        XXLarge = 6,
> +        XXSmall = 1, // This is to enable the NonZero optimization
> +                     // which simplifies the representation of Option<KeywordSize>
> +                     // in bindgen
> +        XSmall,
> +        Small,
> +        Medium,
> +        Large,
> +        XLarge,
> +        XXLarge,
>          // This is not a real font keyword and will not parse
>          // HTML font-size 7 corresponds to this value
> -        XXXLarge = 7,
> +        XXXLarge,

Does anything depend on <font size=3>, etc., mapping to these by their integer values?

Comment 17

[Manish Goregaokar [:manishearth]]

Comment on attachment 8888122 [details]
Bug 1382190 - servo: Move FontComputationData to the end of ServoComputedValues to make size check easier, make it NonZero ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159012/diff/4-5/

Comment 18

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8888122 [details]
Bug 1382190 - servo: Move FontComputationData to the end of ServoComputedValues to make size check easier, make it NonZero ;

https://reviewboard.mozilla.org/r/159012/#review164462

::: layout/style/ServoTypes.h:139
(Diff revision 5)
>  
>  struct ServoWritingMode {
>    uint8_t mBits;
>  };
>  
> +enum ServoKeywordSize {

This is wrong, afaict. The default enum layout in C++ doesn't need (and in fact doesn't match the one of rust).

This just happens to work because the float after it forces the struct to have padding, so the checks match... Still seems quite fishy...

::: servo/components/style/properties/longhand/font.mako.rs:635
(Diff revision 5)
> -        XXLarge = 6,
> +        Large,
> +        XLarge,
> +        XXLarge,
>          // This is not a real font keyword and will not parse
>          // HTML font-size 7 corresponds to this value
> -        XXXLarge = 7,
> +        XXXLarge,

Also, we index on an array of length 8 with this, so not only is this altering behavior, but would be a panic when this value is used from pres hints.

And we have a `from_html_size` that matches on integers to this enum, and at the very minimmum the comments there should be updated.

Comment 19

[Manish Goregaokar [:manishearth]]

Comment on attachment 8888122 [details]
Bug 1382190 - servo: Move FontComputationData to the end of ServoComputedValues to make size check easier, make it NonZero ;

https://reviewboard.mozilla.org/r/159012/#review164462

> This is wrong, afaict. The default enum layout in C++ doesn't need (and in fact doesn't match the one of rust).
> 
> This just happens to work because the float after it forces the struct to have padding, so the checks match... Still seems quite fishy...

We are using this as an opaque type, I'm matching the enums so that it's less brittle.

Layout tests are supposed to ensure this works. Will leave some comments.

> Also, we index on an array of length 8 with this, so not only is this altering behavior, but would be a panic when this value is used from pres hints.
> 
> And we have a `from_html_size` that matches on integers to this enum, and at the very minimmum the comments there should be updated.

I looked for casts by removing the integer values, turns out you can always treat C-like enums as integers. I'll fix that.

Comment 20

[Manish Goregaokar [:manishearth]]

Comment on attachment 8888122 [details]
Bug 1382190 - servo: Move FontComputationData to the end of ServoComputedValues to make size check easier, make it NonZero ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159012/diff/5-6/

Comment 21

[Manish Goregaokar [:manishearth]]

Comment on attachment 8888122 [details]
Bug 1382190 - servo: Move FontComputationData to the end of ServoComputedValues to make size check easier, make it NonZero ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159012/diff/6-7/

Comment 22

[Manish Goregaokar [:manishearth]]

Tested on 32 bit, works. I can't verify that it doesn't crash, but layout tests for ServoStyleContext are passing again

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

Would be great if someone could test that build on an actual win32 machine.

Comment 23

[Ekanan Ketunuti]

I had tested that and can still repro the crash. But without crash symbol, I don't know for sure that this fixes this crash or trigger another crash.

Comment 24

[Manish Goregaokar [:manishearth]]

When did you test it? This is a different try push from the one I originally posted.

Comment 25

[Ekanan Ketunuti]

(In reply to Manish Goregaokar [:manishearth] from comment #24)
> When did you test it? This is a different try push from the one I originally
> posted.

comment 22 one.

Comment 26

[Manish Goregaokar [:manishearth]]

Hm. That's interesting. The layout tests pass now so it is probably a different crash, but I'm not sure.


Someone who has 32 bit builds set up really should be looking into this. I've been unsuccessful in getting cross builds working so far.

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://reviewboard.mozilla.org/r/159012/diff/7 makes it possible for me to load pages again, with stylo enabled, in a Win32 debug build.

Comment 28

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8888122 [details]
Bug 1382190 - servo: Move FontComputationData to the end of ServoComputedValues to make size check easier, make it NonZero ;

https://reviewboard.mozilla.org/r/159012/#review164554

::: layout/style/ServoTypes.h:159
(Diff revision 7)
> +// but the entire struct should
> +// have the same size and alignment as the Rust version.
> +// Ensure layout tests get run if touching either side.
>  struct ServoFontComputationData {
> -  // 8 bytes, but is done as 4+4 for alignment
> -  uint32_t mFour;
> +  ServoKeywordSize mKeyword;
> +  float/*32_t*/ mRatio;

Please either really use float32_t or static_assert the sizeof(float).

::: servo/components/style/properties/longhand/font.mako.rs:626
(Diff revision 7)
>      #[derive(Debug, Copy, Clone, PartialEq)]
>      #[cfg_attr(feature = "servo", derive(HeapSizeOf))]
>      pub enum KeywordSize {
> -        XXSmall = 0,
> -        XSmall = 1,
> -        Small = 2,
> +        XXSmall = 1, // This is to enable the NonZero optimization
> +                     // which simplifies the representation of Option<KeywordSize>
> +                     // in bindgen

I still feel this is quite an abuse...

::: servo/components/style/properties/longhand/font.mako.rs:653
(Diff revision 7)
>                  "x-large" => Ok(XLarge),
>                  "xx-large" => Ok(XXLarge),
>              }
>          }
> +
> +        pub fn html_size(&self) -> u8 {

This is a footgun... Please document it.

Also, this can just become `*self as u8 - 1`.

::: servo/components/style/properties/longhand/font.mako.rs:750
(Diff revision 7)
>                  let base_size = unsafe { Atom::with(gecko_font.mLanguage.raw::<nsIAtom>(), |atom| {
>                      cx.font_metrics_provider.get_size(atom, gecko_font.mGenericID).0
>                  }) };
>  
>                  let base_size_px = au_to_int_px(base_size as f32);
> -                let html_size = *self as usize;
> +                // The enum is one-indexed, but we 

This comment is truncated, please finish it.

Comment 29

[Manish Goregaokar [:manishearth]]

Comment on attachment 8888122 [details]
Bug 1382190 - servo: Move FontComputationData to the end of ServoComputedValues to make size check easier, make it NonZero ;

https://reviewboard.mozilla.org/r/159012/#review164554

> I still feel this is quite an abuse...

We're not trying to get the exact repr, just that FontCOmputationData should overall have the same size/alignment/padding in the struct. The layout tests get us the rest of the way there.

> This is a footgun... Please document it.
> 
> Also, this can just become `*self as u8 - 1`.

I checked, it compiles down to a subtraction.

Comment 30

[Manish Goregaokar [:manishearth]]

Comment on attachment 8888122 [details]
Bug 1382190 - servo: Move FontComputationData to the end of ServoComputedValues to make size check easier, make it NonZero ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159012/diff/7-8/

Comment 31

[Manish Goregaokar [:manishearth]]

Comment on attachment 8888122 [details]
Bug 1382190 - servo: Move FontComputationData to the end of ServoComputedValues to make size check easier, make it NonZero ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159012/diff/8-9/

Comment 32

[Manish Goregaokar [:manishearth]]

https://github.com/servo/servo/pull/17803

Comment 33

[Manish Goregaokar [:manishearth]]

Comment on attachment 8888122 [details]
Bug 1382190 - servo: Move FontComputationData to the end of ServoComputedValues to make size check easier, make it NonZero ;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159012/diff/9-10/

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/23da6b9178da
servo: Move FontComputationData to the end of ServoComputedValues to make size check easier, make it NonZero. r=emilio

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a216d7ca2e9a
Make fields public to avoid assertions; r=bustage

Comment 36

[Manish Goregaokar [:manishearth]]

Some builds are failing because the fields are private and unused, I'm just going to not mark them private for now; the comment should suffice. I could also add a dummy method that uses them.

Looks like C++ doesn't have anything like Rust's usage of underscores to silence warnings.

Comment 37

[Ekanan Ketunuti]

(In reply to Ekanan Ketunuti from comment #23)
> I had tested that and can still repro the crash. But without crash symbol, I
> don't know for sure that this fixes this crash or trigger another crash.

Hmmm, the autoland build doesn't seem to crash for me. strange. what's difference between try and autoland?

Comment 38

[Manish Goregaokar [:manishearth]]

Perhaps you hit an unrelated crash. There shouldn't be a difference.

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/23da6b9178da
https://hg.mozilla.org/mozilla-central/rev/a216d7ca2e9a

Comment 40

[Ekanan Ketunuti]

Can no longer reproduce those crashes.

Comment 41

[Emilio Cobos Álvarez (:emilio)]

We're investigating the layout test failures in bug 1366050 too.