Invert DCHECK during PDF incremental loading
Fixes a DCHECK in PDFiumEngine::OnPendingRequestComplete() that
incorrectly asserts that the document should be finished loading, when
it actually should assert the opposite.
Updates PDFiumEngineTest::TryLoadIncrementally() to trigger coverage of
this code path. This code path is executed only when pages are in the
pending state when OnPendingRequestComplete() is called, which only
happens when pages become visible outside of a loading operation.
Fixed: 1051548
Change-Id: I035d95c7e626c294a0b3189fcbc6c5ab33944786
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2067743
Reviewed-by: Daniel Hosseinian <dhoss@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: K. Moon <kmoon@chromium.org>
Cr-Commit-Position: refs/heads/master@{#798979}
diff --git a/pdf/pdfium/pdfium_engine.cc b/pdf/pdfium/pdfium_engine.cc
index ee2485875..5e697d6 100644
--- a/pdf/pdfium/pdfium_engine.cc
+++ b/pdf/pdfium/pdfium_engine.cc
@@ -726,7 +726,7 @@
}
pending_pages_.swap(still_pending);
if (update_pages) {
- DCHECK(document_loaded_);
+ DCHECK(!document_loaded_);
LoadPageInfo();
}
}
diff --git a/pdf/pdfium/pdfium_engine_unittest.cc b/pdf/pdfium/pdfium_engine_unittest.cc
index 5675c8c..053974e 100644
--- a/pdf/pdfium/pdfium_engine_unittest.cc
+++ b/pdf/pdfium/pdfium_engine_unittest.cc
@@ -4,6 +4,8 @@
#include "pdf/pdfium/pdfium_engine.h"
+#include <stdint.h>
+
#include "base/hash/md5.h"
#include "base/strings/utf_string_conversions.h"
#include "base/test/scoped_feature_list.h"
@@ -70,7 +72,9 @@
CompareRect(expected_rect, page->rect());
}
- // Tries to load a PDF incrementally, returning `true` on success.
+ // Tries to load a PDF incrementally, returning `true` if the PDF actually was
+ // loaded incrementally. Note that this function will return `false` if
+ // incremental loading fails, but also if incremental loading is disabled.
bool TryLoadIncrementally() {
NiceMock<MockTestClient> client;
InitializeEngineResult initialize_result = InitializeEngineWithoutLoading(
@@ -81,14 +85,43 @@
}
PDFiumEngine* engine = initialize_result.engine.get();
- // Note: Plugin size chosen so all pages of the document are visible. The
- // engine only updates availability incrementally for visible pages.
- engine->PluginSizeUpdated({1024, 4096});
+ // Load enough for the document to become partially available.
initialize_result.document_loader->SimulateLoadData(8192);
- PDFiumPage* page0 = GetPDFiumPageForTest(engine, 0);
- PDFiumPage* page1 = GetPDFiumPageForTest(engine, 1);
- return page0 && page0->available() && page1 && !page1->available();
+ bool loaded_incrementally;
+ if (engine->GetNumberOfPages() == 0) {
+ // This is not necessarily a test failure; it just indicates incremental
+ // loading is not occurring.
+ loaded_incrementally = false;
+ } else {
+ // Note: Plugin size chosen so all pages of the document are visible. The
+ // engine only updates availability incrementally for visible pages.
+ EXPECT_EQ(0, CountAvailablePages(engine));
+ engine->PluginSizeUpdated({1024, 4096});
+ int available_pages = CountAvailablePages(engine);
+ loaded_incrementally =
+ 0 < available_pages && available_pages < engine->GetNumberOfPages();
+ }
+
+ // Verify that loading can finish.
+ while (initialize_result.document_loader->SimulateLoadData(UINT32_MAX))
+ continue;
+
+ EXPECT_EQ(engine->GetNumberOfPages(), CountAvailablePages(engine));
+
+ return loaded_incrementally;
+ }
+
+ private:
+ // Counts the number of available pages. Returns `int` instead of `size_t` for
+ // consistency with `PDFiumEngine::GetNumberOfPages()`.
+ int CountAvailablePages(PDFiumEngine* engine) {
+ int available_pages = 0;
+ for (int i = 0; i < engine->GetNumberOfPages(); ++i) {
+ if (GetPDFiumPageForTest(engine, i)->available())
+ ++available_pages;
+ }
+ return available_pages;
}
};
