From bfa2a97ca9f5026ceb8e8c2ce23cfc3c3e8b38a1 Mon Sep 17 00:00:00 2001 From: Tom Sepez Date: Mon, 24 Jul 2017 11:38:31 -0700 Subject: Observe Annot destruction in CPDFSDK_PageView::DeleteAnnot Run test XFA-Only, since that's where the issue occurs, and the textual output is different for non-xfa. Fix a few lifetime issue in pdfium_test.cc unearthed by this test (doc must outlive pages now held in formfill info). Bug: 735912 Change-Id: Icc9e6a967c32ece67d897117896c973bb16a1515 Reviewed-on: https://pdfium-review.googlesource.com/8510 Commit-Queue: Tom Sepez Reviewed-by: dsinclair --- fpdfsdk/cpdfsdk_pageview.cpp | 14 ++- samples/pdfium_test.cc | 59 +++++------ testing/SUPPRESSIONS | 22 ++-- testing/resources/javascript/bug_735912.in | 115 +++++++++++++++++++++ .../resources/javascript/bug_735912_expected.txt | 2 + 5 files changed, 169 insertions(+), 43 deletions(-) create mode 100644 testing/resources/javascript/bug_735912.in create mode 100644 testing/resources/javascript/bug_735912_expected.txt diff --git a/fpdfsdk/cpdfsdk_pageview.cpp b/fpdfsdk/cpdfsdk_pageview.cpp index 4780d87780..06761a5806 100644 --- a/fpdfsdk/cpdfsdk_pageview.cpp +++ b/fpdfsdk/cpdfsdk_pageview.cpp @@ -175,17 +175,23 @@ CPDFSDK_Annot* CPDFSDK_PageView::AddAnnot(CXFA_FFWidget* pPDFAnnot) { bool CPDFSDK_PageView::DeleteAnnot(CPDFSDK_Annot* pAnnot) { if (!pAnnot) return false; + CPDFXFA_Page* pPage = pAnnot->GetPDFXFAPage(); if (!pPage || (pPage->GetContext()->GetDocType() != XFA_DocType::Static && pPage->GetContext()->GetDocType() != XFA_DocType::Dynamic)) { return false; } + CPDFSDK_Annot::ObservedPtr pObserved(pAnnot); if (GetFocusAnnot() == pAnnot) - m_pFormFillEnv->KillFocusAnnot(0); - CPDFSDK_AnnotHandlerMgr* pAnnotHandler = m_pFormFillEnv->GetAnnotHandlerMgr(); - if (pAnnotHandler) - pAnnotHandler->ReleaseAnnot(pAnnot); + m_pFormFillEnv->KillFocusAnnot(0); // May invoke JS, invalidating pAnnot. + + if (pObserved) { + CPDFSDK_AnnotHandlerMgr* pAnnotHandler = + m_pFormFillEnv->GetAnnotHandlerMgr(); + if (pAnnotHandler) + pAnnotHandler->ReleaseAnnot(pObserved.Get()); + } auto it = std::find(m_SDKAnnotArray.begin(), m_SDKAnnotArray.end(), pAnnot); if (it != m_SDKAnnotArray.end()) diff --git a/samples/pdfium_test.cc b/samples/pdfium_test.cc index dab734e530..bb969b1c8d 100644 --- a/samples/pdfium_test.cc +++ b/samples/pdfium_test.cc @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -118,7 +119,7 @@ struct Options { struct FPDF_FORMFILLINFO_PDFiumTest : public FPDF_FORMFILLINFO { // Hold a map of the currently loaded pages in order to avoid them // to get loaded twice. - std::map loaded_pages; + std::map> loaded_pages; // Hold a pointer of FPDF_FORMHANDLE so that PDFium app hooks can // make use of it. @@ -966,7 +967,7 @@ FPDF_PAGE GetPageForIndex(FPDF_FORMFILLINFO* param, auto& loaded_pages = form_fill_info->loaded_pages; auto iter = loaded_pages.find(index); if (iter != loaded_pages.end()) - return iter->second; + return iter->second.get(); FPDF_PAGE page = FPDF_LoadPage(doc, index); if (!page) @@ -975,7 +976,7 @@ FPDF_PAGE GetPageForIndex(FPDF_FORMFILLINFO* param, FPDF_FORMHANDLE& form_handle = form_fill_info->form_handle; FORM_OnAfterLoadPage(page, form_handle); FORM_DoPageAAction(page, form_handle, FPDFPAGE_AACTION_OPEN); - loaded_pages[index] = page; + loaded_pages[index].reset(page); return page; } @@ -1130,27 +1131,25 @@ bool RenderPage(const std::string& name, const int page_index, const Options& options, const std::string& events) { - std::unique_ptr page( - GetPageForIndex(form_fill_info, doc, page_index)); - if (!page.get()) + FPDF_PAGE page = GetPageForIndex(form_fill_info, doc, page_index); + if (!page) return false; if (options.send_events) - SendPageEvents(form, page.get(), events); + SendPageEvents(form, page, events); if (options.output_format == OUTPUT_STRUCTURE) { - DumpPageStructure(page.get(), page_index); + DumpPageStructure(page, page_index); return true; } - std::unique_ptr text_page( - FPDFText_LoadPage(page.get())); + std::unique_ptr text_page(FPDFText_LoadPage(page)); double scale = 1.0; if (!options.scale_factor_as_string.empty()) std::stringstream(options.scale_factor_as_string) >> scale; - int width = static_cast(FPDF_GetPageWidth(page.get()) * scale); - int height = static_cast(FPDF_GetPageHeight(page.get()) * scale); - int alpha = FPDFPage_HasTransparency(page.get()) ? 1 : 0; + int width = static_cast(FPDF_GetPageWidth(page) * scale); + int height = static_cast(FPDF_GetPageHeight(page) * scale); + int alpha = FPDFPage_HasTransparency(page) ? 1 : 0; std::unique_ptr bitmap( FPDFBitmap_Create(width, height, alpha)); @@ -1162,24 +1161,23 @@ bool RenderPage(const std::string& name, // Note, client programs probably want to use this method instead of the // progressive calls. The progressive calls are if you need to pause the // rendering to update the UI, the PDF renderer will break when possible. - FPDF_RenderPageBitmap(bitmap.get(), page.get(), 0, 0, width, height, 0, + FPDF_RenderPageBitmap(bitmap.get(), page, 0, 0, width, height, 0, FPDF_ANNOT); } else { IFSDK_PAUSE pause; pause.version = 1; pause.NeedToPauseNow = &NeedToPauseNow; - int rv = FPDF_RenderPageBitmap_Start( - bitmap.get(), page.get(), 0, 0, width, height, 0, FPDF_ANNOT, &pause); + int rv = FPDF_RenderPageBitmap_Start(bitmap.get(), page, 0, 0, width, + height, 0, FPDF_ANNOT, &pause); while (rv == FPDF_RENDER_TOBECOUNTINUED) - rv = FPDF_RenderPage_Continue(page.get(), &pause); + rv = FPDF_RenderPage_Continue(page, &pause); } - FPDF_FFLDraw(form, bitmap.get(), page.get(), 0, 0, width, height, 0, - FPDF_ANNOT); + FPDF_FFLDraw(form, bitmap.get(), page, 0, 0, width, height, 0, FPDF_ANNOT); if (!options.render_oneshot) - FPDF_RenderPage_Close(page.get()); + FPDF_RenderPage_Close(page); int stride = FPDFBitmap_GetStride(bitmap.get()); const char* buffer = @@ -1194,20 +1192,20 @@ bool RenderPage(const std::string& name, break; case OUTPUT_EMF: - WriteEmf(page.get(), name.c_str(), page_index); + WriteEmf(page, name.c_str(), page_index); break; case OUTPUT_PS2: case OUTPUT_PS3: - WritePS(page.get(), name.c_str(), page_index); + WritePS(page, name.c_str(), page_index); break; #endif case OUTPUT_TEXT: - WriteText(page.get(), name.c_str(), page_index); + WriteText(page, name.c_str(), page_index); break; case OUTPUT_ANNOT: - WriteAnnot(page.get(), name.c_str(), page_index); + WriteAnnot(page, name.c_str(), page_index); break; case OUTPUT_PNG: @@ -1224,9 +1222,8 @@ bool RenderPage(const std::string& name, case OUTPUT_SKP: { std::unique_ptr recorder( reinterpret_cast( - FPDF_RenderPageSkp(page.get(), width, height))); - FPDF_FFLRecord(form, recorder.get(), page.get(), 0, 0, width, height, 0, - 0); + FPDF_RenderPageSkp(page, width, height))); + FPDF_FFLRecord(form, recorder.get(), page, 0, 0, width, height, 0, 0); image_file_name = WriteSkp(name.c_str(), page_index, recorder.get()); } break; #endif @@ -1242,9 +1239,8 @@ bool RenderPage(const std::string& name, fprintf(stderr, "Page was too large to be rendered.\n"); } - form_fill_info->loaded_pages.erase(page_index); - FORM_DoPageAAction(page.get(), form, FPDFPAGE_AACTION_CLOSE); - FORM_OnBeforeClosePage(page.get(), form); + FORM_DoPageAAction(page, form, FPDFPAGE_AACTION_CLOSE); + FORM_OnBeforeClosePage(page, form); return !!bitmap; } @@ -1260,6 +1256,8 @@ void RenderPdf(const std::string& name, platform_callbacks.Doc_gotoPage = ExampleDocGotoPage; platform_callbacks.Doc_mail = ExampleDocMail; + // The document must outlive |form_callbacks.loaded_pages|. + std::unique_ptr doc; FPDF_FORMFILLINFO_PDFiumTest form_callbacks = {}; #ifdef PDF_ENABLE_XFA form_callbacks.version = 2; @@ -1285,7 +1283,6 @@ void RenderPdf(const std::string& name, int nRet = PDF_DATA_NOTAVAIL; bool bIsLinearized = false; - std::unique_ptr doc; std::unique_ptr pdf_avail( FPDFAvail_Create(&file_avail, &file_access)); diff --git a/testing/SUPPRESSIONS b/testing/SUPPRESSIONS index 8389289589..feb862e3b1 100644 --- a/testing/SUPPRESSIONS +++ b/testing/SUPPRESSIONS @@ -286,13 +286,8 @@ zh_file1.pdf mac * * zh_function_list.pdf mac * * zh_shared_document.pdf mac * * -bug_492.in * nov8 * -bug_679643.in * * noxfa -bug_679642.in * * noxfa - -# # TODO(hnakashima): These might never have been run. Go over them and fix. -# + FRC_8.5_E&X.pdf * * * FRC_8.5_O&PO_GoToE.pdf * * * FRC_8.5_OpenAction&O_URI.pdf * * * @@ -300,9 +295,8 @@ FRC_8.5_PC&C_GoToE_T_T.pdf * * * FRC_8.5_PC_GoToE_T_R&P&A.pdf * * * FRC_8.5_PO_GoToE_T_R&N.pdf * * * -# # xfa_specific -# + Choose.pdf * * * data_binding.pdf * * * Date_FormCale.pdf * * * @@ -315,3 +309,15 @@ TimeField.pdf win,linux * * Test_CheckBox.pdf * * * Test_DateField_locale_zh_HK.pdf mac,win * * +# +# JavaScript tests +# +bug_679642.in * * noxfa +bug_679643.in * * noxfa +bug_735912.in * * noxfa + +# +# Pixel tests +# +bug_492.in * nov8 * + diff --git a/testing/resources/javascript/bug_735912.in b/testing/resources/javascript/bug_735912.in new file mode 100644 index 0000000000..7f42d1b7c2 --- /dev/null +++ b/testing/resources/javascript/bug_735912.in @@ -0,0 +1,115 @@ +{{header}} +{{object 1 0}} << + /Type /Catalog + /Pages 2 0 R + /AcroForm 4 0 R + /OpenAction 30 0 R +>> +endobj +{{object 2 0}} << + /Type /Pages + /Count 1 + /Kids [ + 3 0 R + ] +>> +endobj +% Page number 0. +{{object 3 0}} << + /Type /Page + /Parent 2 0 R + /Annots [ + 8 0 R + ] + /AA << /C 36 0 R >> +>> +endobj +% Forms +{{object 4 0}} << + /XFA [ + (xdp:xdp) 43 0 R + (form) 49 0 R + () 50 0 R + ] + /Fields [ + 8 0 R + ] +>> +endobj +% Fields +{{object 8 0}} << + /FT /Tx + /T (MyField) + /Type /Annot + /Subtype /Widget + /Rect [100 300 500 400] + /AA << /Bl 34 0 R >> +>> +endobj +% JS Action +{{object 30 0}} << + /Type /Action + /S /JavaScript + /JS 31 0 R +>> +endobj +% JS program to exexute +{{object 31 0}} << +>> +stream +this.getField("MyField").setFocus(); +app.alert("focused"); +endstream +endobj +% Lost Focus AAction MyField +{{object 34 0}} << + /Type /Action + /S /JavaScript + /JS 35 0 R +>> +endobj +% JS program to exexute +{{object 35 0}} << +>> +stream +this.removeField("MyField"); +app.alert("removed"); +endstream +endobj +% Closing Page 0 +{{object 36 0}} << + /Type /Action + /S /JavaScript + /JS 37 0 R +>> +endobj +% JS program to exexute +{{object 37 0}} << +>> +stream +this.removeField("MyField"); +endstream +endobj +{{object 43 0}} << +>>stream + + +endstream +endobj +{{object 49 0}} << +>>stream + + +endstream +endobj +{{object 50 0}} << +>>stream + +endstream +endobj +{{xref}} +trailer << + /Root 1 0 R +>> +{{startxref}} +%%EOF diff --git a/testing/resources/javascript/bug_735912_expected.txt b/testing/resources/javascript/bug_735912_expected.txt new file mode 100644 index 0000000000..7d0f42f517 --- /dev/null +++ b/testing/resources/javascript/bug_735912_expected.txt @@ -0,0 +1,2 @@ +Alert: removed +Alert: focused -- cgit v1.2.3