1403074 - stylo: crashed [@ core::sync::atomic::atomic_add<usize>] on Android/arm debug build

Status: RESOLVED WORKSFORME

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

Description

[Makoto Kato [:m_kato]]

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.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

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

Comment 2

[Makoto Kato [:m_kato]]

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

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 4

[Hiroyuki Ikezoe (:hiro)]

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

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 7

[Bobby Holley (:bholley)]

(Marking as not blocking 57)

Thanks for chasing this down Makoto!

Comment 8

[Manish Goregaokar [:manishearth]]

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)

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 11

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Makoto-san ^

Comment 13

[Makoto Kato [:m_kato]]

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.