Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always serialize and deserialize ids the same way #5182

Merged
merged 1 commit into from
Feb 1, 2024

Conversation

nical
Copy link
Contributor

@nical nical commented Feb 1, 2024

Description

RawId's serialization/deserialization currently depends on what cargo features are enabled. If trace is enabled it is serialized via the SerialId type and if replay is enabled it is deserialized from SerialId.

This means that if an application serializes and deserializes Ids (like gecko does for some of the remoting), then having only one of trace and replay features enabled is broken. Firefox only uses the trace feature.

For example:

let id = wgc::id::AdapterId::zip(0, 1, wgt::Backend::Vulkan);
let mut tmp = Vec::new();
bincode::serialize_into(&mut tmp, &id).unwrap();
let de = bincode::deserialize::<wgc::id::AdapterId>(tmp.as_slice()).unwrap(); // Boom.

It's unclear why it used to work and only started breaking at the latest update.

With this patch PR, ids are always serialized via SerialId. It's probably not what we want long term, but we need the fix rather urgently so it will have to do for now.

Checklist

  • Run cargo fmt.
  • Run cargo clippy.
  • Run cargo xtask test to run tests.

@nical nical requested a review from a team as a code owner February 1, 2024 11:19
@teoxoy
Copy link
Member

teoxoy commented Feb 1, 2024

It's unclear why it used to work and only started breaking at the latest update.

#5149 changed some of the features but I don't see anything off that could have caused this.

It's worth noting that the serial-pass feature was replaced by serde but if either replay or trace are enabled it should also get enabled.

Firefox only uses the trace feature.

I see we also use the replay feature.

This change looks fine to me either way.

@nical
Copy link
Contributor Author

nical commented Feb 1, 2024

I see we also use the replay feature.

This change looks fine to me either way.

Ah, that explains it then. I'm continuing Jim's work-in-progress wgpu update, I'm on top of https://phabricator.services.mozilla.com/D200266 where trace is enabled but not replay.

@nical
Copy link
Contributor Author

nical commented Feb 1, 2024

Let's get rid of the footgun anyway.

@nical nical merged commit 617050e into gfx-rs:trunk Feb 1, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants