Skip to content

Commit

Permalink
fix a possible leak when editing a material with matdbg
Browse files Browse the repository at this point in the history
A MaterialParser could be leaked if several edits happened before they
were latched -- this was because the MaterialParser was stored as
a raw pointer instead of a unique_ptr<>, this was done as an attempt
to avoid to use a lock around accessing mPendingEdits.
  • Loading branch information
pixelflinger committed Apr 4, 2024
1 parent 5d27240 commit de3c4b6
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 8 deletions.
7 changes: 2 additions & 5 deletions filament/src/details/Material.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -815,10 +815,7 @@ void FMaterial::applyPendingEdits() noexcept {
const char* name = mName.c_str();
slog.d << "Applying edits to " << (name ? name : "(untitled)") << io::endl;
destroyPrograms(mEngine); // FIXME: this will not destroy the shared variants

std::unique_ptr<MaterialParser> pending{ mPendingEdits };
std::swap(pending, mMaterialParser);
mPendingEdits = nullptr;
latchPendingEdits();
}

/**
Expand All @@ -837,7 +834,7 @@ void FMaterial::onEditCallback(void* userdata, const utils::CString&, const void
// and swapping out the MaterialParser until the next getProgram call.
std::unique_ptr<MaterialParser> pending = createParser(
engine.getBackend(), engine.getShaderLanguage(), packageData, packageSize);
material->mPendingEdits = pending.release();
material->setPendingEdits(std::move(pending));
}

void FMaterial::onQueryCallback(void* userdata, VariantList* pVariants) {
Expand Down
19 changes: 16 additions & 3 deletions filament/src/details/Material.h
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ class FMaterial : public Material {
static void onQueryCallback(void* userdata, VariantList* pActiveVariants);

void checkProgramEdits() noexcept {
if (UTILS_UNLIKELY(mPendingEdits.load())) {
if (UTILS_UNLIKELY(hasPendingEdits())) {
applyPendingEdits();
}
}
Expand Down Expand Up @@ -302,8 +302,21 @@ class FMaterial : public Material {
matdbg::MaterialKey mDebuggerId;
mutable utils::Mutex mActiveProgramsLock;
mutable VariantList mActivePrograms;
// FIXME: multi-threaded access to mPendingEdits is very broken
std::atomic<MaterialParser*> mPendingEdits = {};
mutable utils::Mutex mPendingEditsLock;
std::unique_ptr<MaterialParser> mPendingEdits;
void setPendingEdits(std::unique_ptr<MaterialParser> pendingEdits) noexcept {
std::lock_guard lock(mPendingEditsLock);
std::swap(pendingEdits, mPendingEdits);
}
bool hasPendingEdits() noexcept {
std::lock_guard lock(mPendingEditsLock);
return (bool)mPendingEdits;
}
void latchPendingEdits() noexcept {
std::lock_guard lock(mPendingEditsLock);
std::swap(mPendingEdits, mMaterialParser);
}

#endif

utils::CString mName;
Expand Down

0 comments on commit de3c4b6

Please sign in to comment.