Fixed crash on bookmarks loading

map/bookmark_manager.cpp

@@ -1325,12 +1325,12 @@ void BookmarkManager::CreateCategories(KMLDataCollection && dataCollection, bool
   for (auto const & data : dataCollection)
   {
     auto const & fileName = data.first;
-    auto & fileData = *data.second.get();
+    auto & fileData = *data.second;
     auto & categoryData = fileData.m_categoryData;
 
-    if ((categoryData.m_id != kml::kInvalidMarkGroupId) &&
-        (UserMarkIdStorage::Instance().IsJustCreated() || fileData.m_deviceId != GetPlatform().UniqueClientId()))
+    if (!UserMarkIdStorage::Instance().CheckIds(fileData) || HasDuplicatedIds(fileData))
     {
+      //TODO: notify subscribers(like search subsystem). This KML could have been indexed.
       ResetIds(fileData);
     }
 
@@ -1385,6 +1385,29 @@ void BookmarkManager::CreateCategories(KMLDataCollection && dataCollection, bool
   }
 }
 
+bool BookmarkManager::HasDuplicatedIds(kml::FileData const & fileData) const
+{
+  CHECK_THREAD_CHECKER(m_threadChecker, ());
+  if (fileData.m_categoryData.m_id == kml::kInvalidMarkGroupId)
+    return false;
+
+  if (m_categories.find(fileData.m_categoryData.m_id) != m_categories.cend())
+    return true;
+
+  for (auto const & b : fileData.m_bookmarksData)
+  {
+    if (m_bookmarks.count(b.m_id) > 0)
+      return true;
+  }
+
+  for (auto const & t : fileData.m_tracksData)
+  {
+    if (m_tracks.count(t.m_id) > 0)
+      return true;
+  }
+  return false;
+}
+
 std::unique_ptr<kml::FileData> BookmarkManager::CollectBmGroupKMLData(BookmarkCategory const * group) const
 {
   auto kmlData = std::make_unique<kml::FileData>();

map/bookmark_manager.hpp

@@ -443,6 +443,8 @@ private:
   bool CanConvert() const;
   void FinishConversion(ConversionHandler const & handler, bool result);
 
+  bool HasDuplicatedIds(kml::FileData const & fileData) const;
+
   ThreadChecker m_threadChecker;
 
   Callbacks m_callbacks;

map/user_mark_id_storage.cpp

@@ -13,10 +13,10 @@ std::string const kLastBookmarkCategoryId = "LastBookmarkCategoryId";
 }  // namespace
 
 UserMarkIdStorage::UserMarkIdStorage()
-  : m_isJustCreated(false)
-  , m_lastUserMarkId(0)
+  : m_lastUserMarkId(0)
 {
-  m_isJustCreated = !(HasKey(kLastBookmarkCategoryId) && HasKey(kLastBookmarkId) && HasKey(kLastTrackId));
+  m_isJustCreated = !(HasKey(kLastBookmarkCategoryId) && HasKey(kLastBookmarkId) &&
+                      HasKey(kLastTrackId));
   if (m_isJustCreated)
   {
     ResetCategoryId();
@@ -29,6 +29,10 @@ UserMarkIdStorage::UserMarkIdStorage()
     LoadLastTrackId();
     LoadLastCategoryId();
   }
+
+  m_initialLastBookmarkId = m_lastBookmarkId;
+  m_initialLastTrackId = m_lastTrackId;
+  m_initialLastCategoryId = m_lastCategoryId;
 }
 
 // static
@@ -44,11 +48,6 @@ UserMark::Type UserMarkIdStorage::GetMarkType(kml::MarkId id)
   return static_cast<UserMark::Type>(id >> (sizeof(id) * 8 - kMarkIdTypeBitsCount));
 }
 
-bool UserMarkIdStorage::IsJustCreated() const
-{
-  return m_isJustCreated;
-}
-
 void UserMarkIdStorage::ResetBookmarkId()
 {
   m_lastBookmarkId = 0;
@@ -67,6 +66,41 @@ void UserMarkIdStorage::ResetCategoryId()
   SaveLastCategoryId();
 }
 
+bool UserMarkIdStorage::CheckIds(kml::FileData const & fileData) const
+{
+  // File has no ids. Check passed.
+  if (fileData.m_categoryData.m_id == kml::kInvalidMarkGroupId)
+    return true;
+
+  // Storage is just created. Check failed.
+  if (m_isJustCreated)
+    return false;
+
+  // File was created on another device. Check failed.
+  if (fileData.m_deviceId != GetPlatform().UniqueClientId())
+    return false;
+
+  // There are ids of categories, bookmarks or tracks with values
+  // more than last stored maximums. Check failed.
+  if (fileData.m_categoryData.m_id > m_initialLastCategoryId)
+    return false;
+
+  for (auto const & b : fileData.m_bookmarksData)
+  {
+    if (b.m_id > m_initialLastBookmarkId)
+      return false;
+  }
+
+  for (auto const & t : fileData.m_tracksData)
+  {
+    if (t.m_id > m_initialLastTrackId)
+      return false;
+  }
+
+  // No one corner case. Check passed.
+  return true;
+}
+
 kml::MarkId UserMarkIdStorage::GetNextUserMarkId(UserMark::Type type)
 {
   static_assert(UserMark::Type::USER_MARK_TYPES_COUNT <= (1 << kMarkIdTypeBitsCount),

map/user_mark_id_storage.hpp

@@ -2,6 +2,8 @@
 
 #include "map/user_mark.hpp"
 
+#include "kml/types.hpp"
+
 #include <atomic>
 
 class UserMarkIdStorage
@@ -11,12 +13,12 @@ public:
 
   static UserMark::Type GetMarkType(kml::MarkId id);
 
-  bool IsJustCreated() const;
-
   kml::MarkId GetNextUserMarkId(UserMark::Type type);
   kml::TrackId GetNextTrackId();
   kml::MarkGroupId GetNextCategoryId();
 
+  bool CheckIds(kml::FileData const & fileData) const;
+
   void ResetBookmarkId();
   void ResetTrackId();
   void ResetCategoryId();
@@ -37,12 +39,17 @@ private:
 
   bool HasKey(std::string const & name);
 
-  std::atomic<bool> m_isJustCreated;
+  bool m_isJustCreated;
 
+  //TODO: do we really need atomics here?
   std::atomic<uint64_t> m_lastBookmarkId;
   std::atomic<uint64_t> m_lastTrackId;
   std::atomic<uint64_t> m_lastUserMarkId;
   std::atomic<uint64_t> m_lastCategoryId;
 
+  uint64_t m_initialLastBookmarkId;
+  uint64_t m_initialLastTrackId;
+  uint64_t m_initialLastCategoryId;
+
   bool m_testModeEnabled = false;
 };