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?

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.

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 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...

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...

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 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.

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.

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.

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

Can no longer reproduce those crashes.

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