Lock page in LoadFXAnnot to prevent unintended page closing

Lock page in LoadFXAnnot to prevent unintended page closing

BUG=410326
R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/facd0157ce975158da1659fb58a16c1308bd553b

[Bo Xu, 2014-09-23 23:27:31]

bo_xu@foxitsoftware.com changed reviewers:
+ jun_fang@foxitsoftware.com, tsepez@chromium.org

[Bo Xu, 2014-09-23 23:27:32]

Hi Tom, please review the fix, thanks.

[Tom Sepez, 2014-09-23 23:38:55]

I think this works, but having an ownership model where sometimes the page is
owned by one object, and sometimes its owned by another is an anti-pattern to be
avoided.  If not possible, then OK, but is there a another way?

https://codereview.chromium.org/594403003/diff/1/fpdfsdk/include/fsdk_mgr.h
File fpdfsdk/include/fsdk_mgr.h (right):

https://codereview.chromium.org/594403003/diff/1/fpdfsdk/include/fsdk_mgr.h#n...
fpdfsdk/include/fsdk_mgr.h:533: void SetValid(FX_BOOL bValid) {m_Flag |=
PAGEVIEW_FLAG_VALID;}
nit: we should probably clean up the redundant public: private: public:
declarations by grouping them together appropriately, unless there's some reason
to avoid doing this

https://codereview.chromium.org/594403003/diff/1/fpdfsdk/include/fsdk_mgr.h#n...
fpdfsdk/include/fsdk_mgr.h:540: FX_BOOL m_Flag;
How many of these objects are we going to have allocated at one time?  I might
be simpler just to have m_bValid, m_bLocked, and m_bTakeOverPage.

Then you can have 
  bool IsLocked();
  void SetLocked(bool locked);

instead of Set/Clear methods.

https://codereview.chromium.org/594403003/diff/1/fpdfsdk/src/fsdk_mgr.cpp
File fpdfsdk/src/fsdk_mgr.cpp (right):

https://codereview.chromium.org/594403003/diff/1/fpdfsdk/src/fsdk_mgr.cpp#new...
fpdfsdk/src/fsdk_mgr.cpp:340: delete pPageView;
don't we need to remove the item from the page map?  Will the subsequent
removeAll() result in a double free of these?

[Tom Sepez, 2014-09-23 23:43:14]

On 2014/09/23 23:38:55, Tom Sepez wrote:
> I think this works, but having an ownership model where sometimes the page is
> owned by one object, and sometimes its owned by another is an anti-pattern to
be
> avoided.  If not possible, then OK, but is there a another way?
> 
In other words, it almost feels like these should be reference counted.  Let me
know how your devs would feel about such a change.

[Bo Xu, 2014-09-23 23:53:04]

On 2014/09/23 23:43:14, Tom Sepez wrote:
> On 2014/09/23 23:38:55, Tom Sepez wrote:
> > I think this works, but having an ownership model where sometimes the page
is
> > owned by one object, and sometimes its owned by another is an anti-pattern
to
> be
> > avoided.  If not possible, then OK, but is there a another way?
> > 
> In other words, it almost feels like these should be reference counted.  Let
me
> know how your devs would feel about such a change.

Sure, I will check the dev who owns this code to answer your questions.

[Bo Xu, 2014-09-24 00:04:23]

On 2014/09/23 23:43:14, Tom Sepez wrote:
> On 2014/09/23 23:38:55, Tom Sepez wrote:
> > I think this works, but having an ownership model where sometimes the page
is
> > owned by one object, and sometimes its owned by another is an anti-pattern
to
> be
> > avoided.  If not possible, then OK, but is there a another way?
> > 
> In other words, it almost feels like these should be reference counted.  Let
me
> know how your devs would feel about such a change.

Actually in XFA, we have changed CPDF_page to CPDFXFA_Page, where reference
count is added their. There is a function AddRef (you can see in XFA patch).

If XFA lands, we can use that as an clearer fix, but now we may have to do in
this way. 

We can keep this as a to-do, though.

[Bo Xu, 2014-09-24 00:04:30]

https://codereview.chromium.org/594403003/diff/1/fpdfsdk/src/fsdk_mgr.cpp
File fpdfsdk/src/fsdk_mgr.cpp (right):

https://codereview.chromium.org/594403003/diff/1/fpdfsdk/src/fsdk_mgr.cpp#new...
fpdfsdk/src/fsdk_mgr.cpp:340: delete pPageView;
On 2014/09/23 23:38:54, Tom Sepez wrote:
> don't we need to remove the item from the page map?  Will the subsequent
> removeAll() result in a double free of these?

delete pPageView is removing the value of the hashmap, and removeAll is removing
the key of the hashamp. So this shouldn't be an issue.

[Bo Xu, 2014-09-24 01:01:13]

https://codereview.chromium.org/594403003/diff/1/fpdfsdk/include/fsdk_mgr.h
File fpdfsdk/include/fsdk_mgr.h (right):

https://codereview.chromium.org/594403003/diff/1/fpdfsdk/include/fsdk_mgr.h#n...
fpdfsdk/include/fsdk_mgr.h:533: void SetValid(FX_BOOL bValid) {m_Flag |=
PAGEVIEW_FLAG_VALID;}
On 2014/09/23 23:38:54, Tom Sepez wrote:
> nit: we should probably clean up the redundant public: private: public:
> declarations by grouping them together appropriately, unless there's some
reason
> to avoid doing this

Done.

https://codereview.chromium.org/594403003/diff/1/fpdfsdk/include/fsdk_mgr.h#n...
fpdfsdk/include/fsdk_mgr.h:540: FX_BOOL m_Flag;
On 2014/09/23 23:38:54, Tom Sepez wrote:
> How many of these objects are we going to have allocated at one time?  I might
> be simpler just to have m_bValid, m_bLocked, and m_bTakeOverPage.
> 
> Then you can have 
>   bool IsLocked();
>   void SetLocked(bool locked);
> 
> instead of Set/Clear methods.

Done.

[Tom Sepez, 2014-09-24 17:02:11]

https://codereview.chromium.org/594403003/diff/20001/fpdfsdk/src/fsdk_mgr.cpp
File fpdfsdk/src/fsdk_mgr.cpp (right):

https://codereview.chromium.org/594403003/diff/20001/fpdfsdk/src/fsdk_mgr.cpp...
fpdfsdk/src/fsdk_mgr.cpp:986: m_pAnnotList = new CPDF_AnnotList(m_page);
I saw this function getting re-entered during some tracing.  If this happens,
m_pAnnotList gets overwritten with a new one, and strange things may happen
below.  It looks like this is still possible to be re-entered.  Could you add
some asserts here (e.g. static int depth = 0; if (depth) { /* re-entered */ }
depth++; .... existing code ... depth--;) and see if that is still true when you
run the testcase in the bug?

Even if we don't have a memory safety issue, we'll leak m_pAnnotList if
re-entered.

https://codereview.chromium.org/594403003/diff/20001/fpdfsdk/src/fsdk_mgr.cpp...
fpdfsdk/src/fsdk_mgr.cpp:990: for(int i=0; i<nCount; i++)
if re-entered, nCount is from the old m_pAnnotList ...

https://codereview.chromium.org/594403003/diff/20001/fpdfsdk/src/fsdk_mgr.cpp...
fpdfsdk/src/fsdk_mgr.cpp:992: CPDF_Annot* pPDFAnnot = m_pAnnotList->GetAt(i);
... but this is retrieved from the new m_pAnnotList().

https://codereview.chromium.org/594403003/diff/20001/fpdfsdk/src/fsdk_mgr.cpp...
fpdfsdk/src/fsdk_mgr.cpp:1007: pAnnotHandlerMgr->Annot_OnLoad(pAnnot);
(re-entrancy obseved above ...OnLoad() in this loop.

[Tom Sepez, 2014-09-24 17:07:05]

https://codereview.chromium.org/594403003/diff/20001/fpdfsdk/include/fsdk_mgr.h
File fpdfsdk/include/fsdk_mgr.h (right):

https://codereview.chromium.org/594403003/diff/20001/fpdfsdk/include/fsdk_mgr...
fpdfsdk/include/fsdk_mgr.h:513: 
Super-nit: line contains only whitespace (git cl patch complains about this).

[Tom Sepez, 2014-09-24 17:26:21]

On 2014/09/24 17:02:11, Tom Sepez wrote:
> https://codereview.chromium.org/594403003/diff/20001/fpdfsdk/src/fsdk_mgr.cpp
> File fpdfsdk/src/fsdk_mgr.cpp (right):
> 
>
https://codereview.chromium.org/594403003/diff/20001/fpdfsdk/src/fsdk_mgr.cpp...
> fpdfsdk/src/fsdk_mgr.cpp:986: m_pAnnotList = new CPDF_AnnotList(m_page);
> I saw this function getting re-entered during some tracing.  If this happens,
> m_pAnnotList gets overwritten with a new one, and strange things may happen
> below.  It looks like this is still possible to be re-entered.  Could you add
> some asserts here (e.g. static int depth = 0; if (depth) { /* re-entered */ }
> depth++; .... existing code ... depth--;) and see if that is still true when
you
> run the testcase in the bug?
> 
> Even if we don't have a memory safety issue, we'll leak m_pAnnotList if
> re-entered.
> 
>
https://codereview.chromium.org/594403003/diff/20001/fpdfsdk/src/fsdk_mgr.cpp...
> fpdfsdk/src/fsdk_mgr.cpp:990: for(int i=0; i<nCount; i++)
> if re-entered, nCount is from the old m_pAnnotList ...
> 
>
https://codereview.chromium.org/594403003/diff/20001/fpdfsdk/src/fsdk_mgr.cpp...
> fpdfsdk/src/fsdk_mgr.cpp:992: CPDF_Annot* pPDFAnnot = m_pAnnotList->GetAt(i);
> ... but this is retrieved from the new m_pAnnotList().
> 
>
https://codereview.chromium.org/594403003/diff/20001/fpdfsdk/src/fsdk_mgr.cpp...
> fpdfsdk/src/fsdk_mgr.cpp:1007: pAnnotHandlerMgr->Annot_OnLoad(pAnnot);
> (re-entrancy obseved above ...OnLoad() in this loop.

Nevermind, I just tried this under your patch and the re-entrancy is avoided. 
LGTM.

[Bo Xu, 2014-09-24 17:47:59]

On 2014/09/24 17:26:21, Tom Sepez wrote:
> On 2014/09/24 17:02:11, Tom Sepez wrote:
> >
https://codereview.chromium.org/594403003/diff/20001/fpdfsdk/src/fsdk_mgr.cpp
> > File fpdfsdk/src/fsdk_mgr.cpp (right):
> > 
> >
>
https://codereview.chromium.org/594403003/diff/20001/fpdfsdk/src/fsdk_mgr.cpp...
> > fpdfsdk/src/fsdk_mgr.cpp:986: m_pAnnotList = new CPDF_AnnotList(m_page);
> > I saw this function getting re-entered during some tracing.  If this
happens,
> > m_pAnnotList gets overwritten with a new one, and strange things may happen
> > below.  It looks like this is still possible to be re-entered.  Could you
add
> > some asserts here (e.g. static int depth = 0; if (depth) { /* re-entered */
}
> > depth++; .... existing code ... depth--;) and see if that is still true when
> you
> > run the testcase in the bug?
> > 
> > Even if we don't have a memory safety issue, we'll leak m_pAnnotList if
> > re-entered.
> > 
> >
>
https://codereview.chromium.org/594403003/diff/20001/fpdfsdk/src/fsdk_mgr.cpp...
> > fpdfsdk/src/fsdk_mgr.cpp:990: for(int i=0; i<nCount; i++)
> > if re-entered, nCount is from the old m_pAnnotList ...
> > 
> >
>
https://codereview.chromium.org/594403003/diff/20001/fpdfsdk/src/fsdk_mgr.cpp...
> > fpdfsdk/src/fsdk_mgr.cpp:992: CPDF_Annot* pPDFAnnot =
m_pAnnotList->GetAt(i);
> > ... but this is retrieved from the new m_pAnnotList().
> > 
> >
>
https://codereview.chromium.org/594403003/diff/20001/fpdfsdk/src/fsdk_mgr.cpp...
> > fpdfsdk/src/fsdk_mgr.cpp:1007: pAnnotHandlerMgr->Annot_OnLoad(pAnnot);
> > (re-entrancy obseved above ...OnLoad() in this loop.
> 
> Nevermind, I just tried this under your patch and the re-entrancy is avoided. 
> LGTM.

Thanks Tom, I am going to remove the whitespace and land.

[Bo Xu, 2014-09-24 17:51:25]

Committed patchset #3 (id:40001) manually as facd015 (presubmit successful).