Fix #124167: Crash opening file after asset browser is closed

Switching to an asset browser, then back to a different editor would
cause a crash when loading a new file.

Basically the file/asset browser tried to free dangling asset and asset
library pointers when trying to free itself. That's because the asset
system gets destructed with a `AS_asset_libraries_exit()` call before
the screen and with that the asset browser were freed.

Pull Request: https://projects.blender.org/blender/blender/pulls/125084

Pull Request: https://projects.blender.org/blender/blender/pulls/127696

source/blender/asset_system/AS_asset_library.hh

@@ -127,13 +127,16 @@ class AssetLibrary {
    * \param relative_asset_path: The path of the asset relative to the asset library root. With
    *                             this the asset must be uniquely identifiable within the asset
    *                             library.
+   * \return A weak pointer to the new asset representation. The caller needs to keep some
+   *         reference stored to be able to call #remove_asset(). This would be dangling once the
+   *         asset library is destructed, so a weak pointer should be used to reference it.
    */
-  AssetRepresentation &add_external_asset(StringRef relative_asset_path,
-                                          StringRef name,
-                                          int id_type,
-                                          std::unique_ptr<AssetMetaData> metadata);
+  std::weak_ptr<AssetRepresentation> add_external_asset(StringRef relative_asset_path,
+                                                        StringRef name,
+                                                        int id_type,
+                                                        std::unique_ptr<AssetMetaData> metadata);
   /** See #AssetLibrary::add_external_asset(). */
-  AssetRepresentation &add_local_id_asset(StringRef relative_asset_path, ID &id);
+  std::weak_ptr<AssetRepresentation> add_local_id_asset(StringRef relative_asset_path, ID &id);
   /**
    * Remove an asset from the library that was added using #add_external_asset() or
    * #add_local_id_asset(). Can usually be expected to be constant time complexity (worst case may

source/blender/asset_system/intern/asset_library.cc

@@ -208,17 +208,19 @@ AssetCatalogService &AssetLibrary::catalog_service() const
 
 void AssetLibrary::refresh_catalogs() {}
 
-AssetRepresentation &AssetLibrary::add_external_asset(StringRef relative_asset_path,
-                                                      StringRef name,
-                                                      const int id_type,
-                                                      std::unique_ptr<AssetMetaData> metadata)
+std::weak_ptr<AssetRepresentation> AssetLibrary::add_external_asset(
+    StringRef relative_asset_path,
+    StringRef name,
+    const int id_type,
+    std::unique_ptr<AssetMetaData> metadata)
 {
   AssetIdentifier identifier = this->asset_identifier_from_library(relative_asset_path);
   return asset_storage_->add_external_asset(
       std::move(identifier), name, id_type, std::move(metadata), *this);
 }
 
-AssetRepresentation &AssetLibrary::add_local_id_asset(StringRef relative_asset_path, ID &id)
+std::weak_ptr<AssetRepresentation> AssetLibrary::add_local_id_asset(StringRef relative_asset_path,
+                                                                    ID &id)
 {
   AssetIdentifier identifier = this->asset_identifier_from_library(relative_asset_path);
   return asset_storage_->add_local_id_asset(std::move(identifier), id, *this);

source/blender/asset_system/intern/asset_storage.cc

@@ -17,21 +17,21 @@
 
 namespace blender::asset_system {
 
-AssetRepresentation &AssetStorage::add_local_id_asset(AssetIdentifier &&identifier,
-                                                      ID &id,
-                                                      const AssetLibrary &owner_asset_library)
+std::weak_ptr<AssetRepresentation> AssetStorage::add_local_id_asset(
+    AssetIdentifier &&identifier, ID &id, const AssetLibrary &owner_asset_library)
 {
-  return *local_id_assets_.lookup_key_or_add(
-      std::make_unique<AssetRepresentation>(std::move(identifier), id, owner_asset_library));
+  return local_id_assets_.lookup_key_or_add(
+      std::make_shared<AssetRepresentation>(std::move(identifier), id, owner_asset_library));
 }
 
-AssetRepresentation &AssetStorage::add_external_asset(AssetIdentifier &&identifier,
-                                                      StringRef name,
-                                                      const int id_type,
-                                                      std::unique_ptr<AssetMetaData> metadata,
-                                                      const AssetLibrary &owner_asset_library)
+std::weak_ptr<AssetRepresentation> AssetStorage::add_external_asset(
+    AssetIdentifier &&identifier,
+    StringRef name,
+    const int id_type,
+    std::unique_ptr<AssetMetaData> metadata,
+    const AssetLibrary &owner_asset_library)
 {
-  return *external_assets_.lookup_key_or_add(std::make_unique<AssetRepresentation>(
+  return external_assets_.lookup_key_or_add(std::make_shared<AssetRepresentation>(
       std::move(identifier), name, id_type, std::move(metadata), owner_asset_library));
 }
 

source/blender/asset_system/intern/asset_storage.hh

@@ -25,10 +25,13 @@ class IDRemapper;
 namespace blender::asset_system {
 
 class AssetIdentifier;
+class AssetLibrary;
 class AssetRepresentation;
 
 class AssetStorage {
-  using StorageT = Set<std::unique_ptr<AssetRepresentation>>;
+  /* Uses shared pointers so the UI can aquire weak pointers. It can then ensure pointers are not
+   * dangling before accessing. */
+  using StorageT = Set<std::shared_ptr<AssetRepresentation>>;
 
   StorageT external_assets_;
   /* Store local ID assets separately for efficient lookups.
@@ -38,15 +41,15 @@ class AssetStorage {
 
  public:
   /** See #AssetLibrary::add_external_asset(). */
-  AssetRepresentation &add_external_asset(AssetIdentifier &&identifier,
-                                          StringRef name,
-                                          int id_type,
-                                          std::unique_ptr<AssetMetaData> metadata,
-                                          const AssetLibrary &owner_asset_library);
+  std::weak_ptr<AssetRepresentation> add_external_asset(AssetIdentifier &&identifier,
+                                                        StringRef name,
+                                                        int id_type,
+                                                        std::unique_ptr<AssetMetaData> metadata,
+                                                        const AssetLibrary &owner_asset_library);
   /** See #AssetLibrary::add_external_asset(). */
-  AssetRepresentation &add_local_id_asset(AssetIdentifier &&identifier,
-                                          ID &id,
-                                          const AssetLibrary &owner_asset_library);
+  std::weak_ptr<AssetRepresentation> add_local_id_asset(AssetIdentifier &&identifier,
+                                                        ID &id,
+                                                        const AssetLibrary &owner_asset_library);
 
   /** See #AssetLibrary::remove_asset(). */
   bool remove_asset(AssetRepresentation &asset);

source/blender/asset_system/tests/asset_representation_test.cc

@@ -37,8 +37,9 @@ class AssetRepresentationTest : public AssetLibraryTestBase {
   AssetRepresentation &add_dummy_asset(AssetLibrary &library, StringRef relative_path)
   {
     std::unique_ptr<AssetMetaData> dummy_metadata = std::make_unique<AssetMetaData>();
-    return library.add_external_asset(
-        relative_path, "Some asset name", 0, std::move(dummy_metadata));
+    return *library
+                .add_external_asset(relative_path, "Some asset name", 0, std::move(dummy_metadata))
+                .lock();
   }
 };
 

source/blender/editors/space_file/filelist.cc

@@ -12,6 +12,7 @@
 #include <cstdlib>
 #include <cstring>
 #include <ctime>
+#include <memory>
 #include <optional>
 #include <sys/stat.h>
 
@@ -86,20 +87,20 @@ using namespace blender;
 /* ------------------FILELIST------------------------ */
 
 struct FileListInternEntry {
-  FileListInternEntry *next, *prev;
+  FileListInternEntry *next = nullptr, *prev = nullptr;
 
-  FileUID uid;
+  FileUID uid = 0;
 
-  eFileSel_File_Types typeflag;
+  eFileSel_File_Types typeflag = eFileSel_File_Types(0);
   /** ID type, in case typeflag has FILE_TYPE_BLENDERLIB set. */
-  int blentype;
+  int blentype = 0;
 
-  char *relpath;
+  char *relpath = nullptr;
   /** Optional argument for shortcuts, aliases etc. */
-  char *redirection_path;
+  char *redirection_path = nullptr;
   /** not strictly needed, but used during sorting, avoids to have to recompute it there... */
-  const char *name;
-  bool free_name;
+  const char *name = nullptr;
+  bool free_name = false;
 
   /**
    * This is data from the current main, represented by this file. It's crucial that this is
@@ -108,22 +109,45 @@ struct FileListInternEntry {
    */
   struct {
     /** When showing local IDs (FILE_MAIN, FILE_MAIN_ASSET), the ID this file entry represents. */
-    ID *id;
+    ID *id = nullptr;
 
     /* For the few file types that have the preview already in memory. For others, there's delayed
      * preview reading from disk. Non-owning pointer. */
-    PreviewImage *preview_image;
+    PreviewImage *preview_image = nullptr;
   } local_data;
 
-  /* References an asset in the asset library storage. */
-  asset_system::AssetRepresentation *asset; /* Non-owning. */
+  /**
+   * References an asset in the asset library storage.
+   * The file list inserts this asset representation into the library, and removes it again when
+   * the file list is destructed. In that sense it manages the asset but doesn't own it.
+   *
+   * Weak pointer so access is protected in case the asset library gets destructed externally.
+   */
+  std::weak_ptr<asset_system::AssetRepresentation> asset;
 
   /* See #FILE_ENTRY_BLENDERLIB_NO_PREVIEW. */
-  bool blenderlib_has_no_preview;
+  bool blenderlib_has_no_preview = false;
 
   /** Defined in BLI_fileops.h */
-  eFileAttributes attributes;
-  BLI_stat_t st;
+  eFileAttributes attributes = eFileAttributes(0);
+  BLI_stat_t st = {0};
+
+  /**
+   * Be careful not to use the returned asset pointer in a context where it might be dangling, e.g.
+   * because the file list or the asset library were destroyed.
+   */
+  asset_system::AssetRepresentation *get_asset() const
+  {
+    if (std::shared_ptr<asset_system::AssetRepresentation> asset_ptr = asset.lock()) {
+      /* Returning a raw pointer from a shared pointer and destructing the shared pointer
+       * immediately afterwards isn't entirely clean. But it's just a way to get the raw pointer
+       * from the weak pointer. Nothing should free the asset in the asset library meanwhile, so
+       * this should be fine really. */
+      BLI_assert(asset_ptr.use_count() > 1);
+      return asset_ptr.get();
+    }
+    return nullptr;
+  }
 };
 
 struct FileListIntern {
@@ -572,15 +596,17 @@ static int compare_asset_catalog(void *user_data, const void *a1, const void *a2
   const FileListInternEntry *entry1 = static_cast<const FileListInternEntry *>(a1);
   const FileListInternEntry *entry2 = static_cast<const FileListInternEntry *>(a2);
   const FileSortData *sort_data = static_cast<const FileSortData *>(user_data);
+  const asset_system::AssetRepresentation *asset1 = entry1->get_asset();
+  const asset_system::AssetRepresentation *asset2 = entry2->get_asset();
 
   /* Order non-assets. */
-  if (entry1->asset && !entry2->asset) {
+  if (asset1 && !asset2) {
     return 1;
   }
-  else if (!entry1->asset && entry2->asset) {
+  else if (!asset1 && asset2) {
     return -1;
   }
-  else if (!entry1->asset && !entry2->asset) {
+  else if (!asset1 && !asset2) {
     if (int order = compare_direntry_generic(entry1, entry2); order) {
       return compare_apply_inverted(order, sort_data);
     }
@@ -588,13 +614,13 @@ static int compare_asset_catalog(void *user_data, const void *a1, const void *a2
     return compare_apply_inverted(compare_tiebreaker(entry1, entry2), sort_data);
   }
 
-  const asset_system::AssetLibrary &asset_library1 = entry1->asset->owner_asset_library();
-  const asset_system::AssetLibrary &asset_library2 = entry2->asset->owner_asset_library();
+  const asset_system::AssetLibrary &asset_library1 = asset1->owner_asset_library();
+  const asset_system::AssetLibrary &asset_library2 = asset2->owner_asset_library();
 
   const asset_system::AssetCatalog *catalog1 = asset_library1.catalog_service().find_catalog(
-      entry1->asset->get_metadata().catalog_id);
+      asset1->get_metadata().catalog_id);
   const asset_system::AssetCatalog *catalog2 = asset_library2.catalog_service().find_catalog(
-      entry2->asset->get_metadata().catalog_id);
+      asset2->get_metadata().catalog_id);
 
   /* Order by catalog. Always keep assets without catalog last. */
   int order = 0;
@@ -806,10 +832,10 @@ static bool is_filtered_id_file_type(const FileListInternEntry *file,
  */
 static AssetMetaData *filelist_file_internal_get_asset_data(const FileListInternEntry *file)
 {
-  if (!file->asset) {
-    return nullptr;
+  if (asset_system::AssetRepresentation *asset = file->get_asset()) {
+    return &asset->get_metadata();
   }
-  return &file->asset->get_metadata();
+  return nullptr;
 }
 
 static void prepare_filter_asset_library(const FileList *filelist, FileListFilter *filter)
@@ -1456,11 +1482,9 @@ static void filelist_direntryarr_free(FileDirEntryArr *array)
 
 static void filelist_intern_entry_free(FileList *filelist, FileListInternEntry *entry)
 {
-  /* Asset system storage might be cleared already on file exit. Asset library
-   * pointers are dangling then, so don't access (#120466). */
-  if (AS_asset_libraries_available() && entry->asset) {
+  if (auto asset_ptr = entry->asset.lock()) {
     BLI_assert(filelist->asset_library);
-    filelist->asset_library->remove_asset(*entry->asset);
+    filelist->asset_library->remove_asset(*asset_ptr);
   }
 
   if (entry->relpath) {
@@ -1472,7 +1496,7 @@ static void filelist_intern_entry_free(FileList *filelist, FileListInternEntry *
   if (entry->name && entry->free_name) {
     MEM_freeN((char *)entry->name);
   }
-  MEM_freeN(entry);
+  MEM_delete(entry);
 }
 
 static void filelist_intern_free(FileList *filelist)
@@ -1985,8 +2009,8 @@ int filelist_files_num_entries(FileList *filelist)
 
 static const char *fileentry_uiname(const char *root, FileListInternEntry *entry, char *buff)
 {
-  if (entry->asset) {
-    const StringRefNull asset_name = entry->asset->get_name();
+  if (asset_system::AssetRepresentation *asset = entry->get_asset()) {
+    const StringRefNull asset_name = asset->get_name();
     return BLI_strdupn(asset_name.c_str(), asset_name.size());
   }
 
@@ -2133,7 +2157,7 @@ static FileDirEntry *filelist_file_create_entry(FileList *filelist, const int in
     ret->redirection_path = BLI_strdup(entry->redirection_path);
   }
   ret->id = entry->local_data.id;
-  ret->asset = entry->asset;
+  ret->asset = entry->get_asset();
   /* For some file types the preview is already available. */
   if (entry->local_data.preview_image &&
       BKE_previewimg_is_finished(entry->local_data.preview_image, ICON_SIZE_PREVIEW))
@@ -2270,7 +2294,7 @@ asset_system::AssetRepresentation *filelist_entry_get_asset_representation(
     const FileList *filelist, const int index)
 {
   const FileListInternEntry *intern_entry = filelist_entry_intern_get(filelist, index);
-  return intern_entry->asset;
+  return intern_entry->get_asset();
 }
 
 ID *filelist_file_get_id(const FileDirEntry *file)
@@ -3094,7 +3118,7 @@ static int filelist_readjob_list_dir(FileListReadJob *job_params,
         continue;
       }
 
-      entry = MEM_cnew<FileListInternEntry>(__func__);
+      entry = MEM_new<FileListInternEntry>(__func__);
       entry->relpath = current_relpath_append(job_params, files[i].relname);
       entry->st = files[i].s;
 
@@ -3186,7 +3210,7 @@ ENUM_OPERATORS(ListLibOptions, LIST_LIB_ADD_PARENT);
 static FileListInternEntry *filelist_readjob_list_lib_group_create(
     const FileListReadJob *job_params, const int idcode, const char *group_name)
 {
-  FileListInternEntry *entry = MEM_cnew<FileListInternEntry>(__func__);
+  FileListInternEntry *entry = MEM_new<FileListInternEntry>(__func__);
   entry->relpath = current_relpath_append(job_params, group_name);
   entry->typeflag |= FILE_TYPE_BLENDERLIB | FILE_TYPE_DIR;
   entry->blentype = idcode;
@@ -3204,7 +3228,7 @@ static void filelist_readjob_list_lib_add_datablock(FileListReadJob *job_params,
                                                     const int idcode,
                                                     const char *group_name)
 {
-  FileListInternEntry *entry = MEM_cnew<FileListInternEntry>(__func__);
+  FileListInternEntry *entry = MEM_new<FileListInternEntry>(__func__);
   if (prefix_relpath_with_group_name) {
     std::string datablock_path = StringRef(group_name) + SEP_STR + datablock_info->name;
     entry->relpath = current_relpath_append(job_params, datablock_path.c_str());
@@ -3229,7 +3253,7 @@ static void filelist_readjob_list_lib_add_datablock(FileListReadJob *job_params,
         datablock_info->asset_data = metadata.get();
         datablock_info->free_asset_data = false;
 
-        entry->asset = &job_params->load_asset_library->add_external_asset(
+        entry->asset = job_params->load_asset_library->add_external_asset(
             entry->relpath, datablock_info->name, idcode, std::move(metadata));
       }
     }
@@ -3273,7 +3297,7 @@ static void filelist_readjob_list_lib_add_from_indexer_entries(
 static FileListInternEntry *filelist_readjob_list_lib_navigate_to_parent_entry_create(
     const FileListReadJob *job_params)
 {
-  FileListInternEntry *entry = MEM_cnew<FileListInternEntry>(__func__);
+  FileListInternEntry *entry = MEM_new<FileListInternEntry>(__func__);
   entry->relpath = current_relpath_append(job_params, FILENAME_PARENT);
   entry->typeflag |= (FILE_TYPE_BLENDERLIB | FILE_TYPE_DIR);
   return entry;
@@ -3893,7 +3917,7 @@ static void filelist_readjob_main_assets_add_items(FileListReadJob *job_params,
 
     const char *id_code_name = BKE_idtype_idcode_to_name(GS(id_iter->name));
 
-    entry = MEM_cnew<FileListInternEntry>(__func__);
+    entry = MEM_new<FileListInternEntry>(__func__);
     std::string datablock_path = StringRef(id_code_name) + SEP_STR + (id_iter->name + 2);
     entry->relpath = current_relpath_append(job_params, datablock_path.c_str());
     entry->name = id_iter->name + 2;
@@ -3905,7 +3929,7 @@ static void filelist_readjob_main_assets_add_items(FileListReadJob *job_params,
                                                                              id_iter);
     entry->local_data.id = id_iter;
     if (job_params->load_asset_library) {
-      entry->asset = &job_params->load_asset_library->add_local_id_asset(entry->relpath, *id_iter);
+      entry->asset = job_params->load_asset_library->add_local_id_asset(entry->relpath, *id_iter);
     }
     entries_num++;
     BLI_addtail(&tmp_entries, entry);