1836670 - Intermittent application crashed [@ arena_dalloc] when running test_parser_diagnostics_unprintables.html on opt builds | single tracking bug

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Treeherder Bug Filer]

Filed by: ealvarez [at] mozilla.com
Parsed log: https://treeherder.mozilla.org/logviewer?job_id=418052311&repo=autoland
Full log: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/HmN7Ha1cQIKUunmqlsUUpg/runs/0/artifacts/public/logs/live_backing.log

---

The test changes from https://hg.mozilla.org/integration/autoland/rev/7c8f755bb185b66dbaf3d2f61f4702ea28a300fd unveil this crash on shippable builds only.

I believe might be rust UB.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

:emilio, since you are the author of the regressor, bug 1835066, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1836670 - Use NonNull in cssparser::CowRcStr. r=#layout,#style

This prevents the compiler from misoptimizing this code due to undefined
behavior.

Upstream PR: https://github.com/servo/rust-cssparser/pull/345

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9337424 [details]
Bug 1836670 - Use NonNull in cssparser::CowRcStr. r=#layout,#style

Beta/Release Uplift Approval Request

• User impact if declined: Crashes in some cases (with CSS error reporting enabled, which is a devtools feature, but maybe others)
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Simple refactoring.
• String changes made/needed: none
• Is Android affected?: Yes

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9337424 [details]
Bug 1836670 - Use NonNull in cssparser::CowRcStr. r=#layout,#style

Security Approval Request

• How easily could an exploit be constructed based on the patch?: not trivially, I believe. This mostly happens with error reporting enabled and it's a miscompilation / compiler abusing UB issue.
• Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: No
• Which older supported branches are affected by this flaw?: all, potentially
• If not all supported branches, which bug introduced the flaw?: None
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?: Should apply cleanly to all supported branches (maybe needing an extra cargo.toml tweak for ESR).
• How likely is this patch to cause regressions; how much testing does it need?: not likely, it's a very self-contained refactoring to a well-exercised codepath.
• Is Android affected?: Yes

Comment 5

[Mike Hommey [:glandium]]

Note this may not affect our builds of esr102, because we're not using LLVM 16 there. I suspect LLVM 16 needs to be involved to make things possibly bad (and rust 1.70, itself using LLVM 16 made things worse, triggering bug 1836885, which is, in fact, the same underlying bug). We've been using LLVM 16 since 114. We'll still want this on esr102, though, because downstreams may be using LLVM 16 (we know some are).

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1835066

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: WIP: Bug 1836670 - ESR patch.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1836670 - ESR patch.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9337823 [details]
Bug 1836670 - ESR patch.

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: see above
• User impact if declined: crashes
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Simple / well-scoped patch.

Comment 10

[Tom Ritter [:tjr]]

Comment on attachment 9337424 [details]
Bug 1836670 - Use NonNull in cssparser::CowRcStr. r=#layout,#style

Approved to land and uplift

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/951284fe622c

Comment 12

[Donal Meehan [:dmeehan]]

Comment on attachment 9337424 [details]
Bug 1836670 - Use NonNull in cssparser::CowRcStr. r=#layout,#style

Approved for 115.0b5.

Comment 13

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c7a1d5f45ad5

Comment 14

[Donal Meehan [:dmeehan]]

Comment on attachment 9337823 [details]
Bug 1836670 - ESR patch.

Approved for 102.13esr.

Comment 15

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-esr102/rev/8b7eadc7f506