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

fix: string comparison when deleting synced attributes #385

Merged

Conversation

aboedo
Copy link
Member

@aboedo aboedo commented Nov 9, 2020

This is kind of a fascinating one to me.
Tests were failing on 32-bit devices, on something that didn't seem arch-related to me at least in an obvious way.
The actual bug was really clear: we were doing

        if (syncingAppUserID != currentAppUserID) {

where both arguments are strings, so that comparison meant to check for value but was actually checking equality.

The interesting part, though, was that it works for 64 bit devices (we would have caught it straight away in the tests if it didn't).

Turns out, an implementation detail within NSString actually makes it work in 64-bit, but not 32-bit.

64-bit 32-bit
image image image image

I believe 64-bit devices are internally doing something akin to ruby's symbols where they reuse the same memory address for immutable strings that have the same value, but I couldn't find any docs on it.

Fortunately, the impact of the bug is small - it just means that devices might try to re-sync the same subscriber attribute if it's set to the same value after syncing.
And, although I can't confirm it, from the tests it would seem like it only affects 32-bit devices, which there aren't that many of in the wild.

@aboedo aboedo added this to the 3.7.6 milestone Nov 9, 2020
@aboedo aboedo requested a review from vegaro November 9, 2020 15:19
@aboedo aboedo self-assigned this Nov 9, 2020
Copy link
Contributor

@vegaro vegaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow good catch

@aboedo aboedo merged commit 5b2b90d into develop Nov 9, 2020
@aboedo aboedo deleted the fix/string_comparison_when_deleting_synced_attributes branch November 9, 2020 20:23
@aboedo aboedo mentioned this pull request Nov 11, 2020
@aboedo aboedo mentioned this pull request Jul 13, 2021
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.

None yet

2 participants