Closed Bug 1547682 Opened 1 year ago Closed 1 year ago

Cranelift: properly handle unknown ref types instead of crashing

Categories

(Core :: Javascript: WebAssembly, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed

People

(Reporter: bbouvier, Assigned: bbouvier)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Once this is done, I think we can re-enable fuzzing for Cranelift, because it will properly error out instead of panicking when it sees an unknown ref type. It's also a better UX than crashing Firefox, so I'll classify this as an enhancement.

This requires https://github.com/CraneStation/cranelift/pull/754 to be merged and Spidermonkey to be bumped before we can merge this.

Attachment #9061330 - Attachment description: Bug 1547682: Cranelift: properly handle unknown type errors → Bug 1547682: Cranelift: properly handle unknown type errors; r?lth
Attachment #9061342 - Attachment description: Bug 1547682: Reenable wasm fuzzing for cranelift; → Bug 1547682: Reenable wasm fuzzing for cranelift; r?decoder
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9fd0c4622f00
Bump Cranelift to be8a83132df0a277da8fa3e6a9c5d03c4a05d57e; r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/7bfcf5af011d
Cranelift: properly handle unknown type errors; r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/815455c1634e
Reenable wasm fuzzing for cranelift; r=decoder

That's unfortunate: these new failures result from the fact that the failure crate is now built with the std feature, as recently enabled by the cranelift-wasm crate. The failure crate with std support references an old backtraces crate that doesn't compile on CI; it is probably the reason why std support for this crate has been disabled by other uses of the failure crate in tree.

We can:

  • disable failure/std support in cranelift-wasm, which would imply the WasmError::User() field to not get a failure::Error (only defined with failure/std support). It can probably be replaced with a Box<std::error::Error>, or some simple error wrapper, and it wouldn't be too bad.
  • go the long way: upgrade backtraces in failure, take care of the other link errors etc.

I'll try the former first.

Attachment #9061330 - Attachment description: Bug 1547682: Cranelift: properly handle unknown type errors; r?lth → Bug 1547682: Cranelift: properly handle unknown type errors; r=lth
Attachment #9061342 - Attachment description: Bug 1547682: Reenable wasm fuzzing for cranelift; r?decoder → Bug 1547682: Reenable wasm fuzzing for cranelift; r=decoder

I've updated the patches 2 and 3 to take into account the Cranelift PR that would need to be accepted first. If somebody wanted to land this before I come back from PTO, then they'd need to bump the Cranelift commit hash too and re-run mach vendor rust to regenerate the first patch. I'll keep the needinfo, in case it's not done when I get back.

Attachment #9061620 - Attachment description: Bug 1547682: Bump Cranelift to be8a83132df0a277da8fa3e6a9c5d03c4a05d57e; r?lth → Bug 1547682: Bump Cranelift to cc216b46b35a797d03c0f3e8b16a2096f1c6db61; r=lth
Pushed by bbouvier@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc152a886c75
Bump Cranelift to cc216b46b35a797d03c0f3e8b16a2096f1c6db61; r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3e245822b9f
Cranelift: properly handle unknown type errors; r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/037e2534c8bd
Reenable wasm fuzzing for cranelift; r=decoder
Flags: needinfo?(bbouvier)
Regressions: 1553049
Regressions: 1555894
You need to log in before you can comment on or make changes to this bug.