Closed Bug 1382190 Opened 2 years ago Closed 2 years ago

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

Categories

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

56 Branch
x86
Windows 10
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla56
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 --- verified

People

(Reporter: ananuti, Assigned: manishearth)

References

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

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
Likely dupe of Bug 1382019, even though the signatures are different.
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.
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?
Flags: needinfo?(manishearth)
Summary: Stylo: multiple crashes when starting firefox → Stylo: multiple nsStyle* crashes when starting firefox on Windows 32-bit
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)
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.
Flags: needinfo?(manishearth) → needinfo?(emilio+bugs)
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)`.
Assignee: nobody → 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/#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...
Attachment #8888122 - Flags: review?(emilio+bugs)
Pushed up an older patch, my bad
(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...
Flags: needinfo?(emilio+bugs)
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.
Flags: needinfo?(emilio+bugs)
Attachment #8888122 - Flags: review?(emilio+bugs)
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 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.
Attachment #8888122 - Flags: review?(emilio+bugs) → review-
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.
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.
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.
When did you test it? This is a different try push from the one I originally posted.
(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.
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.
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 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.
Attachment #8888122 - Flags: review?(emilio+bugs) → review+
Crash Signature: [@ nsStyleImageRequest::Resolve] [@ nsStyleImageLayers::ResolveImages ] [@ nsStyleAutoArray<T>::operator[] ] → [@ nsStyleImageRequest::Resolve] [@ nsStyleImage::ResolveImage] [@ nsStyleImageLayers::ResolveImages ] [@ nsStyleAutoArray<T>::operator[] ]
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.
Crash Signature: [@ nsStyleImageRequest::Resolve] [@ nsStyleImage::ResolveImage] [@ nsStyleImageLayers::ResolveImages ] [@ nsStyleAutoArray<T>::operator[] ] → [@ nsStyleImageRequest::Resolve] [@ nsStyleImage::ResolveImage] [@ nsStyleImageLayers::ResolveImages ] [@ nsStyleImageRequest::GetImageURI ] [@ nsStyleAutoArray<T>::operator[] ]
Pushed by bholley@mozilla.com:
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
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a216d7ca2e9a
Make fields public to avoid assertions; r=bustage
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.
(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?
Perhaps you hit an unrelated crash. There shouldn't be a difference.
https://hg.mozilla.org/mozilla-central/rev/23da6b9178da
https://hg.mozilla.org/mozilla-central/rev/a216d7ca2e9a
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Can no longer reproduce those crashes.
Status: RESOLVED → VERIFIED
We're investigating the layout test failures in bug 1366050 too.
Flags: needinfo?(emilio+bugs)
You need to log in before you can comment on or make changes to this bug.