[bookmarks] Fix: collections reloading bug when BookmarkManager::LoadBookmarks() is called multiple times.

[kirylkaveryn]

When BookmarkManager::LoadBookmarks() is called multiple times, collections will be appended to the m_categories set on every call.
BookmarkManager::LoadBookmarks() can be called multiple times during the batch updates triggered by iCloud sync #7641.

- (void)loadBookmarks
{
  self.bm.LoadBookmarks();
  self.bm.LoadBookmarks();
  self.bm.LoadBookmarks();
}

Bug result in ui:

Solution

ClearCategories() is moved to clean up the m_categories set before reloading in the gui task.

Tested on:

• iPhone 11pro (17.1.2)
• iPhone 6 (12.5)

[vng]

Honestly saying, I don't understand why ClearCategories doesn't work within LoadBookmarks function. Because of async loading and updating UI?

[kirylkaveryn]

Honestly saying, I don't understand why ClearCategories doesn't work within LoadBookmarks function. Because of async loading and updating UI?

Because filling the m_categories set with a new data is not synchronous and data dispatched several times between different threads before will be ready to use.
If we call load bookmarks 3 times the ClearCategories will be called 3 times, and after that the same data will be added to the set 3 times.

And another problem is that MarkGroupID which is used as a key is incremented on every call, and will be different when we emplace the same collection multiple times and produce the data duplication.

[biodranik]

What happens when a user has a lot of bookmarks (and they're loaded slowly). OM is not running. Then the user imports a track from some app (not from Files, there's a bug that import from Files doesn't work on a cold start #4800 ), so there is a cold start of OM, and bookmarks are loaded asynchronously, and import of a file has started asynchronously?

[kirylkaveryn]

What happens when a user has a lot of bookmarks (and they're loaded slowly). OM is not running. Then the user imports a track from some app (not from Files, there's a bug that import from Files doesn't work on a cold start #4800 ), so there is a cold start of OM, and bookmarks are loaded asynchronously, and import of a file has started asynchronously?

The key point is that both operations (the initial loading and an update) will be dispatched on the same GUI thread.
But the cleaning will not be run on the track import (only the full reloading). In this case, the task with the new track will be waiting for the initial loading of all files and only after that will be executed.

2024-05-11.10.48.28.mp4

[biodranik]

Is the order guaranteed by code? E.g. bookmarks loading task will always fire first, and then track import task will be queued, right?

[kirylkaveryn]

Is the order guaranteed by code? E.g. bookmarks loading task will always fire first, and then track import task will be queued, right?

Yes. The first bookmarks loading starts during the entire MapViewController class initialization (not instance) which is created during the app's Main storyboard initialization and launching. The deeplink will be handled later.

The runtime sends initialize() to each class in a program just before the class, or any class that inherits from it, is sent its first message from within the program. Superclasses receive this message before their subclasses.

- (void)initialize {
  self.listeners = [NSHashTable<id<MWMLocationModeListener>> weakObjectsHashTable];
  Framework &f = GetFramework();
  // TODO: Review and improve this code.
  f.SetPlacePageListeners([self]() { [self onMapObjectSelected]; },
                          [self](bool switchFullScreen) { [self onMapObjectDeselected:switchFullScreen]; },
                          [self]() { [self onMapObjectUpdated]; });
  // TODO: Review and improve this code.
  f.SetMyPositionModeListener([self](location::EMyPositionMode mode, bool routingActive) {
    // TODO: Two global listeners are subscribed to the same event from the core.
    // Probably it's better to subscribe only wnen needed and usubscribe in other cases.
    // May be better solution would be multiobservers support in the C++ core.
    [self processMyPositionStateModeEvent:location_helpers::mwmMyPositionMode(mode)];
  });

  self.userTouchesAction = UserTouchesActionNone;
  [[MWMBookmarksManager sharedManager] addObserver:self];
  [[MWMBookmarksManager sharedManager] loadBookmarks]; // <--- Here
  [MWMFrameworkListener addObserver:self];
}

[rtsisyk]

1. BookmarkManager::LoadBookmarks() [ui thread] calls ClearCategories()

organicmaps/map/bookmark_manager.cpp

Lines 1936 to 1940 in b4aecc4

	void BookmarkManager::LoadBookmarks()
	{
	CHECK_THREAD_CHECKER(m_threadChecker, ());
	ClearCategories();
	LoadMetadata();

2. BookmarkManager::ClearCategories() [ui thread] removes all in-memory state

organicmaps/map/bookmark_manager.cpp

Lines 1895 to 1910 in b4aecc4

	void BookmarkManager::ClearCategories()
	{
	CHECK_THREAD_CHECKER(m_threadChecker, ());
	for (auto groupId : m_unsortedBmGroupsIdList)
	{
	ClearGroup(groupId);
	m_changesTracker.OnDeleteGroup(groupId);
	}
	m_compilations.clear();
	m_categories.clear();
	m_unsortedBmGroupsIdList.clear();
	m_bookmarks.clear();
	m_tracks.clear();
	}

At this point, the Bookmarks screen is empty.

---

3. BookmarkManager::LoadBookmarks() [ui thread] schedules BookmarkManager::LoadBookmarks(std::string const & dir... ) on [file thread]

organicmaps/map/bookmark_manager.cpp

Lines 1944 to 1946 in b4aecc4

	GetPlatform().RunTask(Platform::Thread::File, [this]()
	{
	auto collection = LoadBookmarks(GetBookmarksDirectory(), kKmlExtension, KmlFileType::Text,

4. BookmarkManager::LoadBookmarks(std::string const & dir... ) [file thread] reads bookmarks from the disk into a collection and then calls BookmarkManager::NotifyAboutFinishAsyncLoading(KMLData...)

organicmaps/map/bookmark_manager.cpp

Lines 1946 to 1954 in b4aecc4

	auto collection = LoadBookmarks(GetBookmarksDirectory(), kKmlExtension, KmlFileType::Text,
	[](kml::FileData const &)
	{
	return true; // Allow to load any files from the bookmarks directory.
	});
	if (m_needTeardown)
	return;
	NotifyAboutFinishAsyncLoading(std::move(collection));

5. BookmarkManager::NotifyAboutFinishAsyncLoading(KMLData...) updates in-memory state

organicmaps/map/bookmark_manager.cpp

Lines 2043 to 2071 in b4aecc4

	GetPlatform().RunTask(Platform::Thread::Gui, [this, collection]()
	{
	if (!collection->empty())
	{
	CreateCategories(std::move(*collection), true /* autoSave */);
	}
	else if (!m_loadBookmarksFinished)
	{
	CheckAndResetLastIds();
	CheckAndCreateDefaultCategory();
	}
	m_loadBookmarksFinished = true;
	if (!m_bookmarkLoadingQueue.empty())
	{
	ASSERT(m_asyncLoadingInProgress, ());
	LoadBookmarkRoutine(m_bookmarkLoadingQueue.front().m_filename,
	m_bookmarkLoadingQueue.front().m_isTemporaryFile);
	m_bookmarkLoadingQueue.pop_front();
	}
	else
	{
	m_asyncLoadingInProgress = false;
	if (m_asyncLoadingCallbacks.m_onFinished != nullptr)
	m_asyncLoadingCallbacks.m_onFinished();
	}
	});
	}

After this point, the Bookmarks screen has reload bookmarks.

---

There is obvious race condition between steps (2) and (5). This PR should fix this race condition by moving (1)-(2) into (5), i.e. calling ClearCategories() from NotifyAboutFinishAsyncLoading().

[biodranik]

LoadBookmarks was not designed to be called more than once. The fact that it is called many times now, so bookmarks are loaded (and discarded) several times, doesn't feel right, not saying about the seemingly unnecessary overhead for large collections.

Let's investigate other side effects first, including Android, and think about alternative methods and solutions and their pros/cons.

[kirylkaveryn]

LoadBookmarks was not designed to be called more than once. The fact that it is called many times now, so bookmarks are loaded (and discarded) several times, doesn't feel right, not saying about the seemingly unnecessary overhead for large collections.

Let's investigate other side effects first, including Android, and think about alternative methods and solutions and their pros/cons.

So I'll try to write a separate method for reloading categories to avoid the reloading.

[rtsisyk]

LoadBookmarks was not designed to be called more than once. The fact that it is called many times now, so bookmarks are loaded (and discarded) several times, doesn't feel right, not saying about the seemingly unnecessary overhead for large collections.

LoadBookmarks() has reproducible race condition as described in #8133 (comment). This PR does resolve this issue and improves the code in general. The PR doesn't add any extra calls of LoadBookmarks(). Is there any reason why we shouldn't merge this PR?

[rtsisyk]

How do we proceed with this PR? If LoadBookmarks() is supposed to be called only once during the lifetime of the app, why does it have ClearCategories() in the beginning?

I can see two options here:

1. Add a CHECK() to crash LoadBookmarks() on subsequent calls and remove ClearCategories() call as not needed.
2. Merge this PR to fix the obvious race condition.

@biodranik you are blocking a PR. Let's make a decision and move forward.

[vng]

I'd like to review this PR in context with files updating from the Cloud.

What function do we call when some file was updated?

• Generic: LoadBookmarks()
• Or just for one: LoadBookmarks(filePath)

[kirylkaveryn]

I'd like to review this PR in context with files updating from the Cloud.

What function do we call when some file was updated?

• Generic: LoadBookmarks()
• Or just for one: LoadBookmarks(filePath)

In the latest icloud implementation the LoadBookmarks isn't used.

Only the LoadBookmarks(filePath) Is used for the every file that was changed.

LoadBookmarks() and ClearCategories() are called once during the whole app lifecycle. So ClearCategories actually do nothing because there is nothing to clear.