Fix resource tracking when using FPDFPageObj_SetIsActive()
The existing resource tracking code inside cpdf_pagecontentgenerator.cpp
will delete unused resources from the resources dictionary. With the
introduction of FPDFPageObj_SetIsActive(), resources associated with
deactivated page objects get removed, since most of the codebase treats
deactivated page objects are though they do not exist. If those page
objects get reactivated, their resources are missing.
Fix this issue by adding a map in CPDF_PageObjectHolder to store the
unused resources. The resource tracking code moves unused resources into
this map instead of outright deleting them. If FPDFPageObj_SetIsActive()
reactivates an object, the resource tracking code may see resources in
use that do not exist in the resources dictionary. It then attempts to
restore the missing resources from the unused resource map.
To make this scheme work better, also change CPDF_PageContentGenerator
to check the unused resources map for names that are already in use when
naming new resources in RealizeResource().
With this fix, update the test that deliberately expected a bad result
to use the correct test expectation.
Bug: 378464305
Change-Id: Ifbf8d2e90594c9981f8b0f4284ae8046aaed3739
Reviewed-on: https://pdfium-review.googlesource.com/c/pdfium/+/126390
Reviewed-by: Thomas Sepez <tsepez@google.com>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Commit-Queue: Lei Zhang <thestig@chromium.org>
diff --git a/core/fpdfapi/edit/cpdf_pagecontentgenerator.cpp b/core/fpdfapi/edit/cpdf_pagecontentgenerator.cpp
index b248066..0590b6a 100644
--- a/core/fpdfapi/edit/cpdf_pagecontentgenerator.cpp
+++ b/core/fpdfapi/edit/cpdf_pagecontentgenerator.cpp
@@ -94,40 +94,122 @@
}
}
-void RemoveUnusedResources(RetainPtr<CPDF_Dictionary> resources_dict,
- const ResourcesMap& resources_in_use) {
- // TODO(thestig): Remove other unused resource types:
+CPDF_PageObjectHolder::RemovedResourceMap RemoveUnusedResources(
+ CPDF_Dictionary* resource_dict,
+ const std::vector<ByteString>& keys,
+ const std::set<ByteString>* resource_in_use_of_current_type) {
+ CPDF_PageObjectHolder::RemovedResourceMap removed_resource_map;
+ for (const ByteString& key : keys) {
+ if (resource_in_use_of_current_type &&
+ pdfium::Contains(*resource_in_use_of_current_type, key)) {
+ continue;
+ }
+
+ removed_resource_map[key] = resource_dict->RemoveFor(key.AsStringView());
+ }
+ return removed_resource_map;
+}
+
+void SaveUnusedResources(
+ CPDF_PageObjectHolder::RemovedResourceMap& removed_resource_map,
+ CPDF_PageObjectHolder::RemovedResourceMap& saved_resource_map) {
+ for (auto& entry : removed_resource_map) {
+ saved_resource_map[entry.first] = std::move(entry.second);
+ }
+}
+
+std::vector<ByteString> FindKeysToRestore(
+ const std::vector<ByteString>& keys,
+ const std::set<ByteString>* resource_in_use_of_current_type,
+ const CPDF_PageObjectHolder::RemovedResourceMap& saved_resource_map) {
+ if (!resource_in_use_of_current_type) {
+ return {};
+ }
+
+ std::vector<ByteString> missing_keys;
+ for (const ByteString& key : *resource_in_use_of_current_type) {
+ if (!pdfium::Contains(keys, key)) {
+ missing_keys.push_back(key);
+ }
+ }
+
+ std::vector<ByteString> keys_to_restore;
+ for (const ByteString& key : missing_keys) {
+ if (pdfium::Contains(saved_resource_map, key)) {
+ keys_to_restore.push_back(key);
+ }
+ }
+ return keys_to_restore;
+}
+
+// `keys_to_restore` entries must exist in `saved_resource_map`.
+void RestoreUsedResources(
+ CPDF_Dictionary* resource_dict,
+ const std::vector<ByteString>& keys_to_restore,
+ CPDF_PageObjectHolder::RemovedResourceMap& saved_resource_map) {
+ for (const ByteString& key : keys_to_restore) {
+ auto it = saved_resource_map.find(key);
+ CHECK(it != saved_resource_map.end());
+ resource_dict->SetFor(key, std::move(it->second));
+ saved_resource_map.erase(it);
+ }
+}
+
+void RemoveOrRestoreUnusedResources(
+ RetainPtr<CPDF_Dictionary> resources_dict,
+ const ResourcesMap& resources_in_use,
+ CPDF_PageObjectHolder::AllRemovedResourcesMap& all_removed_resources_map) {
+ // TODO(thestig): Remove/restore other unused resource types:
// - ColorSpace
// - Pattern
// - Shading
static constexpr const char* kResourceKeys[] = {"ExtGState", "Font",
"XObject"};
for (const char* resource_key : kResourceKeys) {
- RetainPtr<CPDF_Dictionary> resource_dict =
+ auto resources_in_use_it = resources_in_use.find(resource_key);
+ const std::set<ByteString>* resource_in_use_of_current_type =
+ resources_in_use_it != resources_in_use.end()
+ ? &resources_in_use_it->second
+ : nullptr;
+
+ RetainPtr<CPDF_Dictionary> current_resource_dict =
resources_dict->GetMutableDictFor(resource_key);
- if (!resource_dict) {
+ if (!current_resource_dict && !resource_in_use_of_current_type) {
continue;
}
- std::vector<ByteString> keys;
- {
- CPDF_DictionaryLocker resource_dict_locker(resource_dict);
- for (auto& it : resource_dict_locker) {
- keys.push_back(it.first);
- }
+ const std::vector<ByteString> keys = current_resource_dict->GetKeys();
+ CPDF_PageObjectHolder::RemovedResourceMap removed_resource_map =
+ RemoveUnusedResources(current_resource_dict, keys,
+ resource_in_use_of_current_type);
+ if (!removed_resource_map.empty()) {
+ // Note that this may create a new entry in `all_removed_resources_map`.
+ // Only do this if `removed_resource_map` is not empty.
+ SaveUnusedResources(removed_resource_map,
+ all_removed_resources_map[resource_key]);
}
- auto it = resources_in_use.find(resource_key);
- const std::set<ByteString>* resource_in_use_of_current_type =
- it != resources_in_use.end() ? &it->second : nullptr;
- for (const ByteString& key : keys) {
- if (resource_in_use_of_current_type &&
- pdfium::Contains(*resource_in_use_of_current_type, key)) {
- continue;
- }
-
- resource_dict->RemoveFor(key.AsStringView());
+ // Restore in-use objects if possible. To do so, first see if there is a
+ // saved resource map to restore from.
+ auto saved_resource_map_it = all_removed_resources_map.find(resource_key);
+ if (saved_resource_map_it == all_removed_resources_map.end()) {
+ continue;
}
+
+ auto& saved_resource_map = saved_resource_map_it->second;
+ std::vector<ByteString> keys_to_restore = FindKeysToRestore(
+ keys, resource_in_use_of_current_type, saved_resource_map);
+ if (keys_to_restore.empty()) {
+ continue;
+ }
+
+ // Create the dictionary if needed, so there is a place to restore to.
+ if (!current_resource_dict) {
+ current_resource_dict =
+ resources_dict->SetNewFor<CPDF_Dictionary>(resource_key);
+ }
+ RestoreUsedResources(current_resource_dict, keys_to_restore,
+ saved_resource_map);
}
}
@@ -136,7 +218,8 @@
CPDF_PageContentGenerator::CPDF_PageContentGenerator(
CPDF_PageObjectHolder* pObjHolder)
: m_pObjHolder(pObjHolder), m_pDocument(pObjHolder->GetDocument()) {
- // Copy all page objects, even if they are inactive.
+ // Copy all page objects, even if they are inactive. They are needed in
+ // GenerateModifiedStreams() below.
for (const auto& pObj : *pObjHolder) {
m_pageObjects.emplace_back(pObj.get());
}
@@ -322,7 +405,8 @@
seen_resources["ExtGState"].insert(m_DefaultGraphicsName);
}
- RemoveUnusedResources(std::move(resources), seen_resources);
+ RemoveOrRestoreUnusedResources(std::move(resources), seen_resources,
+ m_pObjHolder->all_removed_resources_map());
}
ByteString CPDF_PageContentGenerator::RealizeResource(
@@ -336,20 +420,34 @@
m_pObjHolder->GetResources()->GetObjNum());
}
- RetainPtr<CPDF_Dictionary> pResList =
+ RetainPtr<CPDF_Dictionary> resource_dict =
m_pObjHolder->GetMutableResources()->GetOrCreateDictFor(bsType);
+ const auto& all_removed_resources_map =
+ m_pObjHolder->all_removed_resources_map();
+ auto it = all_removed_resources_map.find(bsType);
+ const CPDF_PageObjectHolder::RemovedResourceMap* removed_resource_map =
+ it != all_removed_resources_map.end() ? &it->second : nullptr;
ByteString name;
int idnum = 1;
while (true) {
name = ByteString::Format("FX%c%d", bsType[0], idnum);
- if (!pResList->KeyExist(name))
- break;
+ // Avoid name collisions with existing `resource_dict` entries.
+ if (resource_dict->KeyExist(name)) {
+ idnum++;
+ continue;
+ }
- idnum++;
+ // Also avoid collisions with entries in `removed_resource_map`, since those
+ // entries may move back into `resource_dict`.
+ if (removed_resource_map && pdfium::Contains(*removed_resource_map, name)) {
+ idnum++;
+ continue;
+ }
+
+ resource_dict->SetNewFor<CPDF_Reference>(name, m_pDocument,
+ pResource->GetObjNum());
+ return name;
}
- pResList->SetNewFor<CPDF_Reference>(name, m_pDocument,
- pResource->GetObjNum());
- return name;
}
bool CPDF_PageContentGenerator::ProcessPageObjects(fxcrt::ostringstream* buf) {
diff --git a/core/fpdfapi/page/cpdf_pageobjectholder.h b/core/fpdfapi/page/cpdf_pageobjectholder.h
index 44cbbd2..8970b8b 100644
--- a/core/fpdfapi/page/cpdf_pageobjectholder.h
+++ b/core/fpdfapi/page/cpdf_pageobjectholder.h
@@ -56,6 +56,14 @@
// Value: The current transformation matrix at the end of the stream.
using CTMMap = std::map<int32_t, CFX_Matrix>;
+ // Key: The dictionary key for a given removed resource.
+ // Value: The removed object.
+ using RemovedResourceMap = std::map<ByteString, RetainPtr<CPDF_Object>>;
+
+ // Key: The resource dictionary name.
+ // Value: The entries removed from that dictionary.
+ using AllRemovedResourcesMap = std::map<ByteString, RemovedResourceMap>;
+
using iterator = std::deque<std::unique_ptr<CPDF_PageObject>>::iterator;
using const_iterator =
std::deque<std::unique_ptr<CPDF_PageObject>>::const_iterator;
@@ -129,6 +137,10 @@
// `stream` must be non-negative.
CFX_Matrix GetCTMAtEndOfStream(int32_t stream);
+ AllRemovedResourcesMap& all_removed_resources_map() {
+ return m_AllRemovedResourcesMap;
+ }
+
protected:
void LoadTransparencyInfo();
@@ -152,6 +164,10 @@
// The indexes of Content streams that are dirty and need to be regenerated.
std::set<int32_t> m_DirtyStreams;
+
+ // All the resources from `m_pResources` that are unused. Hold on to them here
+ // in case they need to be restored.
+ AllRemovedResourcesMap m_AllRemovedResourcesMap;
};
#endif // CORE_FPDFAPI_PAGE_CPDF_PAGEOBJECTHOLDER_H_
diff --git a/fpdfsdk/fpdf_editpage_embeddertest.cpp b/fpdfsdk/fpdf_editpage_embeddertest.cpp
index d24e74d..5787d74 100644
--- a/fpdfsdk/fpdf_editpage_embeddertest.cpp
+++ b/fpdfsdk/fpdf_editpage_embeddertest.cpp
@@ -736,14 +736,7 @@
ASSERT_TRUE(saved_page);
ScopedFPDFBitmap bitmap = RenderSavedPage(saved_page);
- // TODO(crbug.com/378464305): Should be `new_path_checksum`.
- const char* wrong_checksum = []() {
- if (CFX_DefaultRenderDevice::UseSkiaRenderer()) {
- return "1da7ed00781f0b3b5c0c6b34af6ed4fd";
- }
- return "b95170fe98422dbc583388f9ad48e6b8";
- }();
- CompareBitmap(bitmap.get(), page_width, page_height, wrong_checksum);
+ CompareBitmap(bitmap.get(), page_width, page_height, new_path_checksum);
EXPECT_EQ(kObjectCountWithNewPath, FPDFPage_CountObjects(saved_page));
CloseSavedPage(saved_page);
