Fix crash on Results.freeze after owning collection removal #6445

kiburtse · 2023-03-28T10:10:57Z

What, How & Why?

☑️ ToDos

📝 Changelog update
🚦 Tests (or not relevant)
C-API, if public C++ API changed.

src/realm/object-store/results.cpp

test/object-store/frozen_objects.cpp

nicola-cab

This looks good to me, at least it mimics the current behaviour (although really weird), but I would not merge it without some review either from @finnschiermer or @jedelbo. But it also breaks a lot of tests, so something is not correct.

nielsenko

I can confirm the fix works now

finnschiermer · 2023-03-31T10:30:06Z

@kiburtse I have approved, but I think the changelog line should reflect that freeze() will no longer crash but throw.

kiburtse · 2023-03-31T11:05:28Z

Well, as i've noted on the issue i believe this pr doesn't fully resolve the problem yet. Added tests here don't yet test the change in is_valid method itself, only missing 'validate_read()' call in freeze. I think tests need to be expanded at least.

nicola-cab

LGTM.

test/object-store/frozen_objects.cpp

…tions + throw if results is constructed using a deleted object. (#6448) * fix Results::is_valid() * cope with stale accessor while freezing realm

…erent check order for collection attached. Test also Results on a table.

nicola-cab

LGTM

test/object-store/frozen_objects.cpp

kiburtse · 2023-05-05T10:53:09Z

So apparently ThreadSanitizer is not happy about something i have no idea about. Something is fishy with teardown code i guess:

3: 0.000 s: sorted
3: 0.000 s: Result constructor - Primitive list
3: 0.009 s: Freeze Results
3: ==================
3: WARNING: ThreadSanitizer: data race (pid=5961)
3:   Write of size 8 at 0x7b6c000f1100 by main thread:
3:     #0 operator delete(void*) <null> (realm-object-store-tests+0x25677e) (BuildId: 2c3c371d7b02b69bfd72c85a783219d655890311)
3:     #1 __gnu_cxx::new_allocator<Catch::Detail::AssertionOrBenchmarkResult>::deallocate(Catch::Detail::AssertionOrBenchmarkResult*, unsigned long) /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/ext/new_allocator.h:145:2 (realm-object-store-tests+0x191e610) (BuildId: 2c3c371d7b02b69bfd72c85a783219d655890311)
3:     #2 std::allocator_traits<std::allocator<Catch::Detail::AssertionOrBenchmarkResult> >::deallocate(std::allocator<Catch::Detail::AssertionOrBenchmarkResult>&, Catch::Detail::AssertionOrBenchmarkResult*, unsigned long) /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/alloc_traits.h:496:13 (realm-object-store-tests+0x191e610)
3:     #3 std::_Vector_base<Catch::Detail::AssertionOrBenchmarkResult, std::allocator<Catch::Detail::AssertionOrBenchmarkResult> >::_M_deallocate(Catch::Detail::AssertionOrBenchmarkResult*, unsigned long) /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_vector.h:354:4 (realm-object-store-tests+0x191e610)
3:     #4 void std::vector<Catch::Detail::AssertionOrBenchmarkResult, std::allocator<Catch::Detail::AssertionOrBenchmarkResult> >::_M_realloc_insert<Catch::AssertionStats const&>(__gnu_cxx::__normal_iterator<Catch::Detail::AssertionOrBenchmarkResult*, std::vector<Catch::Detail::AssertionOrBenchmarkResult, std::allocator<Catch::Detail::AssertionOrBenchmarkResult> > >, Catch::AssertionStats const&) /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/vector.tcc:500:7 (realm-object-store-tests+0x191e610)
3:     #5 Catch::TestCaseHandle::invoke() const /mnt/jenkins/workspace/realm_realm-core_PR-6445@2/build-dir/./src/catch2/../catch2/catch_test_case_info.hpp:114:24 (realm-object-store-tests+0x193e776) (BuildId: 2c3c371d7b02b69bfd72c85a783219d655890311)
3:     #6 Catch::RunContext::invokeActiveTestCase() /mnt/jenkins/workspace/realm_realm-core_PR-6445@2/build-dir/./src/catch2/internal/catch_run_context.cpp:503:27 (realm-object-store-tests+0x193e776)
3:     #7 __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16 (libc.so.6+0x29d8f) (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d)
3: 
3:   Previous write of size 8 at 0x7b6c000f1100 by thread T1223:
3:     #0 operator new(unsigned long) <null> (realm-object-store-tests+0x2560e6) (BuildId: 2c3c371d7b02b69bfd72c85a783219d655890311)
3:     #1 __gnu_cxx::new_allocator<Catch::Detail::AssertionOrBenchmarkResult>::allocate(unsigned long, void const*) /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/ext/new_allocator.h:127:27 (realm-object-store-tests+0x191e529) (BuildId: 2c3c371d7b02b69bfd72c85a783219d655890311)
3:     #2 std::allocator_traits<std::allocator<Catch::Detail::AssertionOrBenchmarkResult> >::allocate(std::allocator<Catch::Detail::AssertionOrBenchmarkResult>&, unsigned long) /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/alloc_traits.h:464:20 (realm-object-store-tests+0x191e529)
3:     #3 std::_Vector_base<Catch::Detail::AssertionOrBenchmarkResult, std::allocator<Catch::Detail::AssertionOrBenchmarkResult> >::_M_allocate(unsigned long) /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/stl_vector.h:346:20 (realm-object-store-tests+0x191e529)
3:     #4 void std::vector<Catch::Detail::AssertionOrBenchmarkResult, std::allocator<Catch::Detail::AssertionOrBenchmarkResult> >::_M_realloc_insert<Catch::AssertionStats const&>(__gnu_cxx::__normal_iterator<Catch::Detail::AssertionOrBenchmarkResult*, std::vector<Catch::Detail::AssertionOrBenchmarkResult, std::allocator<Catch::Detail::AssertionOrBenchmarkResult> > >, Catch::AssertionStats const&) /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/vector.tcc:440:33 (realm-object-store-tests+0x191e529)
3:     #5 <null> <null> (libstdc++.so.6+0xdc2b2) (BuildId: f57e02bfadacc0c923c82457d5e18e1830b5faea)
3: 
3:   Thread T1223 (tid=7195, finished) created by main thread at:
3:     #0 pthread_create <null> (realm-object-store-tests+0x1d612d) (BuildId: 2c3c371d7b02b69bfd72c85a783219d655890311)
3:     #1 std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State> >, void (*)()) <null> (libstdc++.so.6+0xdc388) (BuildId: f57e02bfadacc0c923c82457d5e18e1830b5faea)
3:     #2 Catch::TestCaseHandle::invoke() const /mnt/jenkins/workspace/realm_realm-core_PR-6445@2/build-dir/./src/catch2/../catch2/catch_test_case_info.hpp:114:24 (realm-object-store-tests+0x193e776) (BuildId: 2c3c371d7b02b69bfd72c85a783219d655890311)
3:     #3 Catch::RunContext::invokeActiveTestCase() /mnt/jenkins/workspace/realm_realm-core_PR-6445@2/build-dir/./src/catch2/internal/catch_run_context.cpp:503:27 (realm-object-store-tests+0x193e776)
3:     #4 __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16 (libc.so.6+0x29d8f) (BuildId: 69389d485a9793dbe873f0ea2c93e02efaa9aa3d)
3: 
3: SUMMARY: ThreadSanitizer: data race (/mnt/jenkins/workspace/realm_realm-core_PR-6445@2/build-dir/test/object-store/realm-object-store-tests+0x25677e) (BuildId: 2c3c371d7b02b69bfd72c85a783219d655890311) in operator delete(void*)
3: ==================
3: 0.667 s: Result constructor - Dictionary

This reverts commit 4311f04.

ironage · 2023-05-05T23:01:45Z

It is possible there is a race in catch2 itself. See for example: catchorg/Catch2#1833
We should look at updating our catch2 version soon to see if that helps.

This reverts commit efa87ac.

This reverts commit 26dc59f.

This reverts commit c496270.

kiburtse · 2023-05-11T17:19:09Z

test/object-store/frozen_objects.cpp

@@ -269,10 +269,12 @@ TEST_CASE("Freeze Results", "[freeze_results]") {
            REQUIRE(frozen_res.get<Int>(0) == 2);
        });

+        /* FIXME causes ThreadSanitizer error in Catch2?


Interesting... This is what triggered ThreadSanitizer error it seems. And it was only reported in Jenkins TSAN builder (linux x64), and not on evergreen TSAN builder (clang x64). I've tried failing configuration locally, and it runs fine for me. Weird. I'll merge it for now with this thing disabled, and investigate it later. Also, probably with updating Catch2. The same codepath is essentially tested below (on added specific test), and it doesn't fail. Should be fine this way.

cla-bot bot added the cla: yes label Mar 28, 2023

github-actions bot assigned kiburtse Mar 28, 2023

kiburtse requested a review from nicola-cab March 28, 2023 10:11

nicola-cab reviewed Mar 28, 2023

View reviewed changes

src/realm/object-store/results.cpp Outdated Show resolved Hide resolved

nicola-cab reviewed Mar 28, 2023

View reviewed changes

test/object-store/frozen_objects.cpp Outdated Show resolved Hide resolved

nicola-cab reviewed Mar 28, 2023

View reviewed changes

nielsenko added a commit to realm/realm-dart that referenced this pull request Mar 29, 2023

Update realm-core for realm/realm-core#6445

aa73ebb

nielsenko added a commit to realm/realm-dart that referenced this pull request Mar 29, 2023

Update realm-core for realm/realm-core#6445

ef26acd

nielsenko mentioned this pull request Mar 29, 2023

Crash on Results.freeze after owning object remove #6401

Closed

nielsenko approved these changes Mar 29, 2023

View reviewed changes

nielsenko added a commit to realm/realm-dart that referenced this pull request Mar 30, 2023

Update realm-core for realm/realm-core#6445

6c84bf3

finnschiermer approved these changes Mar 31, 2023

View reviewed changes

nicola-cab self-requested a review May 2, 2023 08:42

nicola-cab approved these changes May 2, 2023

View reviewed changes

kiburtse force-pushed the kb/crash-on-results-freeze-after-object-remove branch from 06d872f to 35a3897 Compare May 3, 2023 19:42

kiburtse marked this pull request as ready for review May 3, 2023 19:43

kiburtse commented May 3, 2023

View reviewed changes

test/object-store/frozen_objects.cpp Show resolved Hide resolved

test/object-store/frozen_objects.cpp Show resolved Hide resolved

test/object-store/frozen_objects.cpp Show resolved Hide resolved

kiburtse requested review from nicola-cab and finnschiermer May 4, 2023 12:27

kiburtse force-pushed the kb/crash-on-results-freeze-after-object-remove branch from 35a3897 to 32ea6a3 Compare May 4, 2023 18:49

kiburtse and others added 7 commits May 5, 2023 12:12

Fix crash on Results.freeze after owning collection removal

766d9a5

Fix Results::is_valid() for invalid results bound to deleted collec…

0e6dae3

…tions + throw if results is constructed using a deleted object. (#6448) * fix Results::is_valid() * cope with stale accessor while freezing realm

Fix test, reuse common exception logic

43e6dc5

Use own Results object for every subtest

26dc59f

Add more thorough tests. Reproduce original crash with query for diff…

0aaf91a

…erent check order for collection attached. Test also Results on a table.

Update changelog to be more precise

09c813f

Disable test for table remove for Results

efa87ac

kiburtse force-pushed the kb/crash-on-results-freeze-after-object-remove branch from 32ea6a3 to efa87ac Compare May 5, 2023 10:14

nicola-cab approved these changes May 5, 2023

View reviewed changes

test/object-store/frozen_objects.cpp Show resolved Hide resolved

test/object-store/frozen_objects.cpp Show resolved Hide resolved

nicola-cab added 2 commits May 5, 2023 17:38

avoid to convert unique_ptr into shared_ptr

4311f04

Revert "avoid to convert unique_ptr into shared_ptr"

7e780cb

This reverts commit 4311f04.

kiburtse added 4 commits May 10, 2023 19:33

Revert "Disable test for table remove for Results"

7c26304

This reverts commit efa87ac.

Revert "Use own Results object for every subtest"

c496270

This reverts commit 26dc59f.

Revert "Revert "Use own Results object for every subtest""

a6c68a2

This reverts commit c496270.

Try disabling some added testcases

9791dad

kiburtse commented May 11, 2023

View reviewed changes

kiburtse merged commit 6e127d3 into master May 11, 2023
2 checks passed

kiburtse deleted the kb/crash-on-results-freeze-after-object-remove branch May 11, 2023 18:08

This was referenced May 12, 2023

Update Catch2 submodule #6623

Closed

Update catch2 v3.3.2 #6605

Merged

ironage mentioned this pull request May 15, 2023

fix entries that went to the wrong change version #6632

Merged

github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix crash on Results.freeze after owning collection removal #6445

Fix crash on Results.freeze after owning collection removal #6445

kiburtse commented Mar 28, 2023 •

edited

Loading

nicola-cab left a comment •

edited

Loading

nielsenko left a comment

finnschiermer commented Mar 31, 2023

kiburtse commented Mar 31, 2023

nicola-cab left a comment

nicola-cab left a comment

kiburtse commented May 5, 2023 •

edited

Loading

ironage commented May 5, 2023

kiburtse May 11, 2023

Fix crash on Results.freeze after owning collection removal #6445

Fix crash on Results.freeze after owning collection removal #6445

Conversation

kiburtse commented Mar 28, 2023 • edited Loading

What, How & Why?

☑️ ToDos

nicola-cab left a comment • edited Loading

Choose a reason for hiding this comment

nielsenko left a comment

Choose a reason for hiding this comment

finnschiermer commented Mar 31, 2023

kiburtse commented Mar 31, 2023

nicola-cab left a comment

Choose a reason for hiding this comment

nicola-cab left a comment

Choose a reason for hiding this comment

kiburtse commented May 5, 2023 • edited Loading

ironage commented May 5, 2023

kiburtse May 11, 2023

Choose a reason for hiding this comment

kiburtse commented Mar 28, 2023 •

edited

Loading

nicola-cab left a comment •

edited

Loading

kiburtse commented May 5, 2023 •

edited

Loading