Issue 401 in pdfium: XFA: SEGV in CPDFXFA_Page::GetXFAPageView()

[jun_fang@foxitsoftware.com via Monorail]

Status: Accepted
Owner: thes...@chromium.org
CC: jun_f...@foxitsoftware.com,  kai_j...@foxitsoftware.com,  jinming_...@foxitsoftware.com,  tse...@chromium.org  
Labels: Type-Defect Priority-Medium

New issue 401 by jun_f...@foxitsoftware.com: XFA: SEGV in CPDFXFA_Page::GetXFAPageView()
https://bugs.chromium.org/p/pdfium/issues/detail?id=401

What steps will reproduce the problem?
1. Build chrome with XFA enabled on Ubuntu 14.04.
2. Run chrome to open the attached file.
3. Click the button "Hide Page 2".
4. Go to page 2 on chrome.
What is the expected output? What do you see instead?
Only page 1 is visible on chrome. A crasher was found.

Please use labels and text to provide additional information.
==1==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000008 (pc 0x7f38d7725fe9 bp 0x7ffc69657110 sp 0x7ffc69657100 T0)
    #0 0x7f38d7725fe8 in GetXFAPageView /home/jun/chrome/src/out/Debug/../../third_party/pdfium/fpdfsdk/include/fpdfxfa/fpdfxfa_page.h:22:44
    #1 0x7f38d7720435 in FPDFPage_HasFormFieldAtPoint /home/jun/chrome/src/out/Debug/../../third_party/pdfium/fpdfsdk/src/fpdfformfill.cpp:75:30
    #2 0x7f38d7720e00 in FPDPage_HasFormFieldAtPoint /home/jun/chrome/src/out/Debug/../../third_party/pdfium/fpdfsdk/src/fpdfformfill.cpp:132:10
    #3 0x7f38bd5f4617 in GetCharIndex /home/jun/chrome/src/out/Debug/../../pdf/pdfium/pdfium_page.cc:320:7
    #4 0x7f38bd591c3f in GetCharIndex /home/jun/chrome/src/out/Debug/../../pdf/pdfium/pdfium_engine.cc:1610:10
    #5 0x7f38bd590af2 in GetCharIndex /home/jun/chrome/src/out/Debug/../../pdf/pdfium/pdfium_engine.cc:1581:10
    #6 0x7f38bd584be3 in OnMouseMove /home/jun/chrome/src/out/Debug/../../pdf/pdfium/pdfium_engine.cc:1764:7
    #7 0x7f38bd580ec7 in HandleEvent /home/jun/chrome/src/out/Debug/../../pdf/pdfium/pdfium_engine.cc:1289:12
    #8 0x7f38bd63064b in HandleInputEvent /home/jun/chrome/src/out/Debug/../../pdf/out_of_process_instance.cc:544:7
    #9 0x7f38d72f20d5 in InputEvent_HandleEvent /home/jun/chrome/src/out/Debug/../../ppapi/cpp/module.cc:53:22
    #10 0x7f38d3ea7d89 in CallWhileUnlocked<PP_Bool, int, int, int, int> /home/jun/chrome/src/out/Debug/../../ppapi/shared_impl/proxy_lock.h:135:10
    #11 0x7f38d3ea6611 in OnMsgHandleFilteredInputEvent /home/jun/chrome/src/out/Debug/../../ppapi/proxy/ppp_input_event_proxy.cc:107:13
    #12 0x7f38d3ea9286 in DispatchToMethodImpl<ppapi::proxy::PPP_InputEvent_Proxy *, void (ppapi::proxy::PPP_InputEvent_Proxy::*)(int, const ppapi::InputEventData &, PP_Bool *), int, ppapi::InputEventData, PP_Bool, 0, 1, 0> /home/jun/chrome/src/out/Debug/../../base/tuple.h:241:3
    #13 0x7f38d3ea8ea5 in DispatchToMethod<ppapi::proxy::PPP_InputEvent_Proxy *, void (ppapi::proxy::PPP_InputEvent_Proxy::*)(int, const ppapi::InputEventData &, PP_Bool *), int, ppapi::InputEventData, PP_Bool> /home/jun/chrome/src/out/Debug/../../base/tuple.h:249:3
    #14 0x7f38d3ea7990 in Dispatch<ppapi::proxy::PPP_InputEvent_Proxy, ppapi::proxy::PPP_InputEvent_Proxy, void, void (ppapi::proxy::PPP_InputEvent_Proxy::*)(int, const ppapi::InputEventData &, PP_Bool *)> /home/jun/chrome/src/out/Debug/../../ipc/ipc_message_templates.h:169:7
    #15 0x7f38d3ea5ec1 in OnMessageReceived /home/jun/chrome/src/out/Debug/../../ppapi/proxy/ppp_input_event_proxy.cc:85:5
    #16 0x7f38d3cc7b83 in OnMessageReceived /home/jun/chrome/src/out/Debug/../../ppapi/proxy/dispatcher.cc:70:10
    #17 0x7f38d412c0ab in OnMessageReceived /home/jun/chrome/src/out/Debug/../../ppapi/proxy/plugin_dispatcher.cc:252:10
    #18 0x7f38c1e8c61d in OnDispatchMessage /home/jun/chrome/src/out/Debug/../../ipc/ipc_channel_proxy.cc:293:3
    #19 0x7f38c1e9991a in Run<const IPC::Message &> /home/jun/chrome/src/out/Debug/../../base/bind_internal.h:181:12
    #20 0x7f38c1e9979e in MakeItSo<IPC::ChannelProxy::Context *, const IPC::Message &> /home/jun/chrome/src/out/Debug/../../base/bind_internal.h:301:5
    #21 0x7f38c1e9956f in Run /home/jun/chrome/src/out/Debug/../../base/bind_internal.h:351:12
    #22 0x7f38bac80293 in Run /home/jun/chrome/src/out/Debug/../../base/callback.h:394:12
    #23 0x7f38bdb96fa0 in RunTask /home/jun/chrome/src/out/Debug/../../base/debug/task_annotator.cc:51:3
    #24 0x7f38bd83dbe1 in RunTask /home/jun/chrome/src/out/Debug/../../base/message_loop/message_loop.cc:486:3
    #25 0x7f38bd83e499 in DeferOrRunPendingTask /home/jun/chrome/src/out/Debug/../../base/message_loop/message_loop.cc:495:5
    #26 0x7f38bd83f577 in DoWork /home/jun/chrome/src/out/Debug/../../base/message_loop/message_loop.cc:607:13
    #27 0x7f38bd86c089 in Run /home/jun/chrome/src/out/Debug/../../base/message_loop/message_pump_default.cc:33:21
    #28 0x7f38bd83c972 in RunHandler /home/jun/chrome/src/out/Debug/../../base/message_loop/message_loop.cc:450:3
    #29 0x7f38bd93881b in Run /home/jun/chrome/src/out/Debug/../../base/run_loop.cc:56:3
    #30 0x7f38bd83a56b in Run /home/jun/chrome/src/out/Debug/../../base/message_loop/message_loop.cc:293:3
    #31 0x7f38d94f3dc3 in PpapiPluginMain /home/jun/chrome/src/out/Debug/../../content/ppapi_plugin/ppapi_plugin_main.cc:160:3
    #32 0x7f38bd54f260 in RunZygote /home/jun/chrome/src/out/Debug/../../content/app/content_main_runner.cc:304:14
    #33 0x7f38bd54fc4c in RunNamedProcessTypeMain /home/jun/chrome/src/out/Debug/../../content/app/content_main_runner.cc:391:12
    #34 0x7f38bd555af1 in Run /home/jun/chrome/src/out/Debug/../../content/app/content_main_runner.cc:752:12
    #35 0x7f38bd54e017 in ContentMain /home/jun/chrome/src/out/Debug/../../content/app/content_main.cc:19:15
    #36 0x7f38ba9562c0 in ChromeMain /home/jun/chrome/src/out/Debug/../../chrome/app/chrome_main.cc:67:12
    #37 0x7f38ba9560b1 in main /home/jun/chrome/src/out/Debug/../../chrome/app/chrome_exe_main_aura.cc:17:10
    #38 0x7f38ad2d1ec4 in __libc_start_main /build/eglibc-3GlaMS/eglibc-2.19/csu/libc-start.c:287:0

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV (/home/jun/chrome/src/out/Debug/chrome+0x222cafe8)
==1==ABORTING
[1:1:0218/174548:ERROR:render_process_impl.cc(73)] WebFrame LEAKED 1 TIMES
[1:1:0218/174548:ERROR:render_process_impl.cc(73)] WebFrame LEAKED 1 TIMES
[15304:15331:0218/174548:ERROR:form_field_data.cc(198)] Unknown FormFieldData pickle version 0
[1:1:0218/174548:ERROR:render_process_impl.cc(73)] WebFrame LEAKED 1 TIMES
[15304:15304:0218/174548:WARNING:pref_notifier_impl.cc(27)] pref observer found at shutdown printing.enabled
[15304:15304:0218/174549:WARNING:pref_notifier_impl.cc(27)] pref observer found at shutdown plugins.always_authorize
[15304:15304:0218/174549:WARNING:pref_notifier_impl.cc(27)] pref observer found at shutdown plugins.allow_outdated


Attachments:
	PageEvent.pdf  78.2 KB

-- 
You received this message because:
  1. The project was configured to send all issue notifications to this address

You may adjust your notification preferences at:
https://bugs.chromium.org/hosting/settings

[jun_fang@foxitsoftware.com via Monorail]

Comment #1 on issue 401 by jun_f...@foxitsoftware.com: XFA: SEGV in CPDFXFA_Page::GetXFAPageView()
https://bugs.chromium.org/p/pdfium/issues/detail?id=401#c1

Clicking "Hide page 2" made page 2 invisible. However, there was no chance for XFA to inform chromium changes on pages. Chromium thought page 2 was still there. When users tried to view page 2 on chromium, chromium sent an invalid page index to XFA. It caused a crasher. So chromium needs to implement a call-back function PDFiumEngine::Form_PageEvent like in https://codereview.chromium.org/1643943002/.

-- 
You received this message because:
  1. The project was configured to send all issue notifications to this address

You may adjust your notification preferences at:
https://bugs.chromium.org/hosting/settings

[thestig@chromium.org via Monorail]

Comment #2 on issue 401 by thes...@chromium.org: XFA: SEGV in CPDFXFA_Page::GetXFAPageView()
https://bugs.chromium.org/p/pdfium/issues/detail?id=401#c2

Questions - if FFI_PageEvent says "page 3 has been removed" then:

1) Does FPDF_ClosePage(handle_to_page_3) / FPDFText_ClosePage(handle_to_page_3) still need to be called?
2) Does FORM_OnBeforeClosePage() need to be called?
3) Is page 4 now page 3? Or does page 3 just become a blank page?
4) Do you have a sample XFA PDF that dynamically inserts a page? (Assuming that is possible)

-- 
You received this message because:
  1. The project was configured to send all issue notifications to this address

You may adjust your notification preferences at:
https://bugs.chromium.org/hosting/settings

[jun_fang@foxitsoftware.com via Monorail]

Comment #3 on issue 401 by jun_f...@foxitsoftware.com: XFA: SEGV in CPDFXFA_Page::GetXFAPageView()
https://bugs.chromium.org/p/pdfium/issues/detail?id=401#c3

1) No need to call FPDF_ClosePage/FPDFText_ClosePage. XFA pages are not like pdf pages. They are dynamically created after layout is performed. In the process of preforming layout, the existing pages will be closed by XFA SDK first and then new pages will be created based on the updated XFA content. 
2) No need for the same reason as above.
3) Assume that we have 4 pages and delete page 3. All the existing page handles, page1 to page4, will be invalid. Applications need to call open pages to reload XFA page handles, page1 to page3. The reason is that XFA removes all XFA pages and creates new pages in the process of performing layout as described in step 1).
4) Will provide a sample to show how to delete/insert a page.

-- 
You received this message because:
  1. The project was configured to send all issue notifications to this address

You may adjust your notification preferences at:
https://bugs.chromium.org/hosting/settings

[thestig@chromium.org via Monorail]

Comment #4 on issue 401 by the...@chromium.org: XFA: SEGV in CPDFXFA_Page::GetXFAPageView()
https://bugs.chromium.org/p/pdfium/issues/detail?id=401#c4

Are you sure there's no need to call FPDF_ClosePage() ? The FPDF_PAGE handles coming from CPDFXFA_Document::GetPage() are pointers to CPDFXFA_Page objects, right? Aren't those CPDFXFA_Page objects ref-counted?

-- 
You received this message because:
  1. The project was configured to send all issue notifications to this address

You may adjust your notification preferences at:
https://bugs.chromium.org/hosting/settings

[jun_fang@foxitsoftware.com via Monorail]

Comment #5 on issue 401 by jun_f...@foxitsoftware.com: XFA: SEGV in CPDFXFA_Page::GetXFAPageView()
https://bugs.chromium.org/p/pdfium/issues/detail?id=401#c5

Yes. There is no need to call FPDF_ClosePage(). Pdfium SDK will delete all page handles internally even ref-count isn't 0 when XFA performs layout. Currently, chromium doesn't need to call FPDF_ClosePage() in the process of page deletion informed by PDFiumEngine::Form_PageEvent.

We don't think it's a good solution. Now, Foxit team are doing an enhancement on the management of page handles when XFA performs layout and causes pages to be refreshed.

[jinming_wang@foxitsoftware.com via Monorail]

Comment #6 on issue 401 by jinming_...@foxitsoftware.com: XFA: SEGV in CPDFXFA_Page::GetXFAPageView()
https://bugs.chromium.org/p/pdfium/issues/detail?id=401#c6

Hello Lei, I have uploaded a CL for this issue, please help to review it.
https://codereview.chromium.org/1758553003

[jinming_wang@foxitsoftware.com via Monorail]

Comment #7 on issue 401 by jinming_...@foxitsoftware.com: XFA: SEGV in CPDFXFA_Page::GetXFAPageView()
https://bugs.chromium.org/p/pdfium/issues/detail?id=401#c7

committed at : https://pdfium.googlesource.com/pdfium/+/a1cef70c08a16e2b9d7ec14987a8b20660d83534

[jinming_wang@foxitsoftware.com via Monorail]

Comment #8 on issue 401 by jinming_...@foxitsoftware.com: XFA: SEGV in CPDFXFA_Page::GetXFAPageView()
https://bugs.chromium.org/p/pdfium/issues/detail?id=401#c8

Hi Lei, Following is the sample codes: Note, FPDF_LoadPage or FPDF_ClosePage is now required to be called by user.

void PDFiumEngine::Form_PageEvent(FPDF_FORMFILLINFO* param,
int page_count,
FPDF_DWORD event_type)
{
if (FXFA_PAGEVIEWEVENT_POSTADDED == event_type)
{
PDFiumEngine* engine = static_cast<PDFiumEngine*>(param);
int nTotalPages = engine->pages_.size();
for (int i = 0; i < page_count; i++)
{
pp::Rect rect;
PDFiumPage* pPage = new PDFiumPage(engine, nTotalPages+i, rect, false);
engine->pages_.push_back(pPage);
pPage->GetPage();
}
engine->InvalidateAllPages();
}
else
{
PDFiumEngine* engine = static_cast<PDFiumEngine*>(param);
int nTotalPages = engine->pages_.size();
for (int i = 0; i < page_count; i++)
{
PDFiumPage* pPage = engine->pages_[nTotalPages - i - 1];
if (pPage)
pPage->Unload();
}
engine->pages_.resize(engine->pages_.size() - page_count);
engine->InvalidateAllPages();

[sou… via monorail]

Comment #10 on issue 401 by sou...@chromium.org: XFA: SEGV in CPDFXFA_Page::GetXFAPageView()
https://bugs.chromium.org/p/pdfium/issues/detail?id=401#c10

Lei@ Sorry, could you confirm whether this issue has been resolved or not?

[thes… via monorail]

Comment #11 on issue 401 by the...@chromium.org: XFA: SEGV in CPDFXFA_Page::GetXFAPageView()
https://bugs.chromium.org/p/pdfium/issues/detail?id=401#c11

FFI_PageEvent isn't implemented any more, so this crash cannot happen for the moment, but at the same time, the PDF viewer can't handle XFA events that adds or deletes pages.

[hnakash… via monorail]

Updates:
Cc: rhar...@chromium.org

Comment #12 on issue 401 by hnaka...@chromium.org: XFA: SEGV in CPDFXFA_Page::GetXFAPageView()
https://bugs.chromium.org/p/pdfium/issues/detail?id=401#c12

From what I understand from this bug, we do not currently implement any mechanism of letting the PDF viewer know a page was removed.

We need a plan, are we going to implement this?

pdfium:483 is also related to deleting pages.

[thes… via monorail]

Comment #13 on issue 401 by the...@chromium.org: XFA: SEGV in CPDFXFA_Page::GetXFAPageView()
https://bugs.chromium.org/p/pdfium/issues/detail?id=401#c13

Maybe we can put on this functionality for now and just track metrics for how often this functionality is actually used?

[hnakash… via monorail]

Comment #14 on issue 401 by hnaka...@chromium.org: XFA: SEGV in CPDFXFA_Page::GetXFAPageView()
https://bugs.chromium.org/p/pdfium/issues/detail?id=401#c14

The crash does happen at the moment, as originally described.

We can disable page addition/removal in XFA. I'm not sure that's a good idea since a js script may rely on the addition or removal working. For example, for crbug.com/867135, a new page is created and a barcode is generate on it.

[hnakash… via monorail]

Updates:
Owner: hnaka...@chromium.org

Comment #15 on issue 401 by hnaka...@chromium.org: XFA: SEGV in CPDFXFA_Page::GetXFAPageView()
https://bugs.chromium.org/p/pdfium/issues/detail?id=401#c15

(No comment was entered for this change.)

[bugdro… via monorail]

Comment #17 on issue 401 by bugd...@chromium.org: XFA: SEGV in CPDFXFA_Page::GetXFAPageView()
https://bugs.chromium.org/p/pdfium/issues/detail?id=401#c17

The following revision refers to this bug:
https://chromium.googlesource.com/chromium/src.git/+/0cb6b59e39bce4769dfda830cf435b4400086ffd

commit 0cb6b59e39bce4769dfda830cf435b4400086ffd
Author: Henrique Nakashima <hnaka...@chromium.org>
Date: Mon Aug 20 18:46:15 2018

Handle FFI_PageEvent from PDFium in PDF Viewer.

Add or remove pages from the viewer in response to these events.

Bug: pdfium:401,chromium:872903,chromium:867135
Change-Id: Iddc88c5a90370213619a0f3f2587f215d0dafbc8
Reviewed-on: https://chromium-review.googlesource.com/1169691
Commit-Queue: Henrique Nakashima <hnaka...@chromium.org>
Reviewed-by: Lei Zhang <the...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#584515}
[modify] https://crrev.com/0cb6b59e39bce4769dfda830cf435b4400086ffd/pdf/pdfium/pdfium_engine.cc
[modify] https://crrev.com/0cb6b59e39bce4769dfda830cf435b4400086ffd/pdf/pdfium/pdfium_engine.h
[modify] https://crrev.com/0cb6b59e39bce4769dfda830cf435b4400086ffd/pdf/pdfium/pdfium_form_filler.cc
[modify] https://crrev.com/0cb6b59e39bce4769dfda830cf435b4400086ffd/pdf/pdfium/pdfium_form_filler.h

[hnakash… via monorail]

Updates:
Status: Fixed

Comment #18 on issue 401 by hnaka...@chromium.org: XFA: SEGV in CPDFXFA_Page::GetXFAPageView()
https://bugs.chromium.org/p/pdfium/issues/detail?id=401#c18

(No comment was entered for this change.)