Skip to content

Fix priority queue assertion which enforce the item to have strict order #14496

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

Merged
merged 2 commits into from
Mar 18, 2025

Conversation

cherylEnkidu
Copy link
Contributor

@cherylEnkidu cherylEnkidu commented Feb 25, 2025

The crash was triggered by recent changes in standard library. In C++ priority queue library, it forces the item inside the queue to have strict order, which means when A > B, B < A is true.

Reference standard library code:

template <class _Tp, class _Up>
  _LIBCPP_CONSTEXPR_SINCE_CXX14 _LIBCPP_HIDE_FROM_ABI bool operator()(_Tp& __x, _Up& __y) {
    bool __r = __comp_(__x, __y);
    if (__r)
      __do_compare_assert(0, __y, __x);
    return __r;
  }

However, in customized compare function for FieldIndex, it only compares sequence_number and collection_group, which leads to two Field Index could potentially evaluate to equal.

Hence this PR add a unique identifier to the field index to make sure they will never evaluate to equal.

The fix can be verified by running Firestore_Tests_iOS in Xcode. It should fail in main branch and success with this change.

Copy link
Contributor

@wu-hui wu-hui left a comment

Choose a reason for hiding this comment

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

Is it possible to create a unit test for this change?

@cherylEnkidu cherylEnkidu removed their assignment Mar 18, 2025
@wu-hui
Copy link
Contributor

wu-hui commented Mar 18, 2025

Is it possible to create a unit test for this change?

Scratch that, yeah, there is already a test for this.

@cherylEnkidu cherylEnkidu merged commit f504fae into main Mar 18, 2025
35 checks passed
@cherylEnkidu cherylEnkidu deleted the cheryllin/fixPriorityQueue branch March 18, 2025 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants