Makoto Kato [:m_kato] Reporter
	Description • 7 years ago

Crash log is here.

https://treeherder.mozilla.org/logviewer.html#?job_id=132923044&repo=try&lineNumber=1808

This might be rust compiler bug.  nsStyleContext has 3 values (mPseudoTag, mBits and mFrameRefCnt) on debug build, and mBits (2nd value) is uint64_t.  So on arm, offset of it may be 8 bytes, not 4 bytes.  But rust compiler uses that offset is 4 bytes.  So this crash occurs.

We need consider alignment of nsStyleContext.

	Hiroyuki Ikezoe (:hiro)
	Comment 1 • 7 years ago

I have no idea why nsStyleContext is involved on stylo. CCing people who had investigated about stylo for linux 32bit (IIRC).

	Makoto Kato [:m_kato] Reporter
	Comment 2 • 7 years ago

Simple workaround is that we change order of each value (1st is mBits, 2nd is mPseudoTag)

	Xidorn Quan [:xidorn] UTC+11
	Comment 3 • 7 years ago

(In reply to Hiroyuki Ikezoe (:hiro) PTO on 28 Sep. from comment #1)
> I have no idea why nsStyleContext is involved on stylo.

IIUC, ComputedValues in Rust side is ServoStyleContext in C++ side (somehow), and ServoStyleContext inherits nsStyleContext.

	Hiroyuki Ikezoe (:hiro)
	Comment 4 • 7 years ago

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #3)
> (In reply to Hiroyuki Ikezoe (:hiro) PTO on 28 Sep. from comment #1)
> > I have no idea why nsStyleContext is involved on stylo.
> 
> IIUC, ComputedValues in Rust side is ServoStyleContext in C++ side
> (somehow), and ServoStyleContext inherits nsStyleContext.

Yeah, Cameron told me that on IRC. I had totally forgotten about the great work done by Manish.

	Xidorn Quan [:xidorn] UTC+11
	Comment 5 • 7 years ago

And... I don't think Rust should be aligning u64 to 4 bytes, given that Rust has a test since long ago checking that u64 is aligned to 8 bytes on android arm: https://github.com/rust-lang/rust/blob/master/src/test/run-pass/rec-align-u64.rs

	Xidorn Quan [:xidorn] UTC+11
	Comment 6 • 7 years ago

Oh, well, but Rust doesn't run tests on Android... so maybe that just fails silently :/

	Bobby Holley (:bholley)
	Comment 7 • 7 years ago

(Marking as not blocking 57)

Thanks for chasing this down Makoto!

	Manish Goregaokar [:manishearth]
	Comment 8 • 7 years ago

Yeah this is likely a bindgen bug on ARM (or a mistake in the manual alignment guesses made there, though that should show up in the ServoStyleContext fields, not the shared nsStyleContext ones. What we have works on the platforms we've run layout tests on, but may be broken on android)

	Xidorn Quan [:xidorn] UTC+11
	Comment 9 • 7 years ago

I don't think that's a bindgen bug. The code bindgen generates seems reasonable.

	Xidorn Quan [:xidorn] UTC+11
	Comment 10 • 7 years ago

FWIW, you can always download the target.stylo-bindings.zip file for any stylo build on treeherder to inspect the code bindgen generates. For this case, it should be in https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d5b01ce823c5515bd45050176e1ec2bdf6dbbb5&selectedJob=132922134

	Hiroyuki Ikezoe (:hiro)
	Comment 11 • 7 years ago

Does this still persist?  I can't see the crash on local build.  Also the failure log in comment 0 is no longer visible so I have no idea what's going on there and what solved this crash.

	Hiroyuki Ikezoe (:hiro)
	Comment 12 • 7 years ago

Makoto-san ^

	Makoto Kato [:m_kato] Reporter
	Comment 13 • 7 years ago

This is fixed by another manish's landing (https://github.com/servo/servo/pull/18667).  Maybe, root cause isn't fixed, but I cannot reproduce this by simple rust code yet.  So this isn't blocker of stylo now.