Font is used after release in CPDF_TextStateData::~CPDF_TextStateData

Font is used after release in CPDF_TextStateData::~CPDF_TextStateData

BUG=400996
R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/2d55db1

[jun_fang, 2014-08-16 08:26:17]

Hi Tom,

Please start to review this fix. Thanks!

[Tom Sepez, 2014-08-18 18:19:31]

https://codereview.chromium.org/477323002/diff/1/core/include/fpdfapi/fpdf_pa...
File core/include/fpdfapi/fpdf_pageobj.h (right):

https://codereview.chromium.org/477323002/diff/1/core/include/fpdfapi/fpdf_pa...
core/include/fpdfapi/fpdf_pageobj.h:223: CPDF_Document*		m_pDocument;
Are we sure that this object doesn't ever out-live the CPDF_Document, in which
case we wind up with a stale pointer here?

[jun_fang, 2014-08-18 20:33:54]

https://codereview.chromium.org/477323002/diff/1/core/include/fpdfapi/fpdf_pa...
File core/include/fpdfapi/fpdf_pageobj.h (right):

https://codereview.chromium.org/477323002/diff/1/core/include/fpdfapi/fpdf_pa...
core/include/fpdfapi/fpdf_pageobj.h:223: CPDF_Document*		m_pDocument;
On 2014/08/18 18:19:31, Tom Sepez wrote:
> Are we sure that this object doesn't ever out-live the CPDF_Document, in which
> case we wind up with a stale pointer here?

Sorry. Can you explain your question more? I don't think that I understand it.

[Tom Sepez, 2014-08-18 20:40:45]

> Sorry. Can you explain your question more? I don't think that I understand it.

Sure.  If the CPDF_TextStateData now caches a pointer to a CPDF_Document, how do
we know that when the CPDF_Document is destroyed that there aren't still any
CPDF_TestStateData objects around that refer to it?

[Tom Sepez, 2014-08-18 20:58:56]

In other words, the problem was that we couldn't use the m_pFont to get to the
document because the m_pFont was destroyed, but how do we know that the document
isn't similarly destroyed, thus kicking the problem down the road?

[jun_fang, 2014-08-18 21:54:03]

On 2014/08/18 20:58:56, Tom Sepez wrote:
> In other words, the problem was that we couldn't use the m_pFont to get to the
> document because the m_pFont was destroyed, but how do we know that the
document
> isn't similarly destroyed, thus kicking the problem down the road?

Good question. In pdfium, the objects of CPDF_Document are basic objects for
others.
They are always created before like CPDF_Font and CPDF_TextStateData objects and

destroyed after them. That's why we can keep a reference to document in these
objects.

[Tom Sepez, 2014-08-18 22:09:22]

Ok, very good.  Just some quick code comments.  Thanks.

https://codereview.chromium.org/477323002/diff/1/core/src/fpdfapi/fpdf_page/f...
File core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp (right):

https://codereview.chromium.org/477323002/diff/1/core/src/fpdfapi/fpdf_page/f...
core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp:149: m_bForceClear = TRUE;
Let's move this down to Clear() itself. The first line of Clear() could be:

  if (bRelease)
      m_bForceClear = TRUE;

that way, should this get called elsewhere someday, its still ok.

https://codereview.chromium.org/477323002/diff/1/core/src/fpdfapi/fpdf_page/f...
core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp:156: {
nit:  these braces don't do anything and can be removed (several places) while
we're modifying Clear().

https://codereview.chromium.org/477323002/diff/1/core/src/fpdfapi/fpdf_page/f...
File core/src/fpdfapi/fpdf_page/fpdf_page_graph_state.cpp (right):

https://codereview.chromium.org/477323002/diff/1/core/src/fpdfapi/fpdf_page/f...
core/src/fpdfapi/fpdf_page/fpdf_page_graph_state.cpp:310: if (m_pDocument) {
nit: this test could be (m_pDocument && m_pFont) since we test m_pFont below
after doing some work that could be avoided if we don't have one.

https://codereview.chromium.org/477323002/diff/1/core/src/fpdfapi/fpdf_page/f...
core/src/fpdfapi/fpdf_page/fpdf_page_graph_state.cpp:320: CPDF_Document* pDoc =
pStateData ? pStateData->m_pDocument : NULL;
nit: can move this inside the if (pStateData) test on the next line and avoid
the ?-operator.

[Tom Sepez, 2014-08-18 22:16:01]

https://codereview.chromium.org/477323002/diff/1/core/src/fpdfapi/fpdf_page/f...
File core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp (right):

https://codereview.chromium.org/477323002/diff/1/core/src/fpdfapi/fpdf_page/f...
core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp:131: , m_bForceClear(FALSE)
this should move down til after the m_FontFileMap initializer, so that these get
initialized in the same order they're declared in the .h file.

[Tom Sepez, 2014-08-18 22:34:16]

https://codereview.chromium.org/477323002/diff/1/core/src/fpdfapi/fpdf_page/f...
File core/src/fpdfapi/fpdf_page/fpdf_page_graph_state.cpp (right):

https://codereview.chromium.org/477323002/diff/1/core/src/fpdfapi/fpdf_page/f...
core/src/fpdfapi/fpdf_page/fpdf_page_graph_state.cpp:304: if (m_pDocument) {
Are we sure that removing the m_pFont test is OK?  You removed one m_pFont->
frome the next line, but there's still a m_pFont->GetFontDict() call buried in
the arguments ...

[Tom Sepez, 2014-08-18 22:38:59]

https://codereview.chromium.org/477323002/diff/1/core/src/fpdfapi/fpdf_page/p...
File core/src/fpdfapi/fpdf_page/pageint.h (right):

https://codereview.chromium.org/477323002/diff/1/core/src/fpdfapi/fpdf_page/p...
core/src/fpdfapi/fpdf_page/pageint.h:431: 
nit: the m_ members should be protected: given the accessors above.  I quick
check shows this compiles with this change on my box.

[jun_fang, 2014-08-18 22:59:31]

Hi Tom, please review it again.

https://codereview.chromium.org/477323002/diff/1/core/src/fpdfapi/fpdf_page/f...
File core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp (right):

https://codereview.chromium.org/477323002/diff/1/core/src/fpdfapi/fpdf_page/f...
core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp:131: , m_bForceClear(FALSE)
On 2014/08/18 22:16:01, Tom Sepez wrote:
> this should move down til after the m_FontFileMap initializer, so that these
get
> initialized in the same order they're declared in the .h file.

Acknowledge.

https://codereview.chromium.org/477323002/diff/1/core/src/fpdfapi/fpdf_page/f...
core/src/fpdfapi/fpdf_page/fpdf_page_doc.cpp:149: m_bForceClear = TRUE;
On 2014/08/18 22:09:22, Tom Sepez wrote:
> Let's move this down to Clear() itself. The first line of Clear() could be:
> 
>   if (bRelease)
>       m_bForceClear = TRUE;
> 
> that way, should this get called elsewhere someday, its still ok.

Good.

https://codereview.chromium.org/477323002/diff/1/core/src/fpdfapi/fpdf_page/f...
File core/src/fpdfapi/fpdf_page/fpdf_page_graph_state.cpp (right):

https://codereview.chromium.org/477323002/diff/1/core/src/fpdfapi/fpdf_page/f...
core/src/fpdfapi/fpdf_page/fpdf_page_graph_state.cpp:304: if (m_pDocument) {
On 2014/08/18 22:34:16, Tom Sepez wrote:
> Are we sure that removing the m_pFont test is OK?  You removed one m_pFont->
> frome the next line, but there's still a m_pFont->GetFontDict() call buried in
> the arguments ...

At line 304, I will change it back as below:
if (m_pDocument && m_pFont) {

[Tom Sepez, 2014-08-18 23:15:16]

LGTM. Thanks.

[jun_fang, 2014-08-18 23:40:09]

Committed patchset #2 manually as 2d55db1 (presubmit successful).