Pdfium: Fix fonts leaking on ClosePage.

Fix memory leaking on ClosePage.
CFX_FontCache refactoring:
  after this CL: Only one global CFX_FontCache used. Any cached items from it, are released, when its are not used.

BUG=79367, 48791
The fonts was not cleared after unloading pages.

Test pdf:

http://www.nasa.gov/pdf/750614main_NASA_FY_2014_Budget_Estimates-508.pdf

For this file, we have ~5 fonts per page, which equal ~1 Mb per page.
In this PDF we have 670 pages, as result after slow scrolling(reading) full document we have ~600 Mb fonts data in memory.

memory usage of PDF Plugin:
  before this CL: ~660 Mb
  after this CL: ~100 Mb

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

[snake, 2016-07-18 17:16:22]

Description was changed from

==========
Fix fonts leaking on ClosePage.

BUG=79367
==========

to

==========
Fix fonts leaking on ClosePage.

BUG=79367

Test pdf:

http://www.nasa.gov/pdf/750614main_NASA_FY_2014_Budget_Estimates-508.pdf

The fonts was not cleared after unloading pages.
==========

[snake, 2016-07-18 17:16:22]

art-snake@yandex-team.ru changed reviewers:
+ brucedawson@chromium.org, jam@chromium.org

[snake, 2016-07-18 17:23:31]

Description was changed from

==========
Fix fonts leaking on ClosePage.

BUG=79367

Test pdf:

http://www.nasa.gov/pdf/750614main_NASA_FY_2014_Budget_Estimates-508.pdf

The fonts was not cleared after unloading pages.
==========

to

==========
Fix fonts leaking on ClosePage.

BUG=79367

The fonts was not cleared after unloading pages.

Test pdf:

http://www.nasa.gov/pdf/750614main_NASA_FY_2014_Budget_Estimates-508.pdf

For this file, we have ~5 fonts per page, which equal ~1 Mb per page.
In this PDF we have 670 pages, as result after slow scrolling(reading) full
document we have ~600 Mb fonts data in memory.
==========

[jam, 2016-07-18 17:49:09]

jam@chromium.org changed reviewers:
+ tsepez@chromium.org
- jam@chromium.org

[jam, 2016-07-18 17:49:09]

redirecting to tsepez

[Tom Sepez, 2016-07-18 17:51:04]

tsepez@chromium.org changed reviewers:
+ dsinclair@chromium.org
- brucedawson@chromium.org, tsepez@chromium.org

[Tom Sepez, 2016-07-18 17:51:15]

tsepez@chromium.org changed reviewers:
+ tsepez@chromium.org

[Tom Sepez, 2016-07-18 17:51:16]

Passing the buck.

[dsinclair, 2016-07-18 17:56:13]

Can you update the description with the memory usage after this CL is applied?

For the various callers of GetFont() have we verified that they're all correctly
calling ReleaseFont() so they don't leak a ref as well?

[snake, 2016-07-18 18:05:03]

On 2016/07/18 17:56:13, dsinclair wrote:
> Can you update the description with the memory usage after this CL is applied?
> 
> For the various callers of GetFont() have we verified that they're all
correctly
> calling ReleaseFont() so they don't leak a ref as well?

The first ref is leaking:
https://cs.chromium.org/chromium/src/third_party/pdfium/core/fpdfapi/fpdf_pag...
 fontData = new CPDF_CountedFont(pFont); // first ref (ref_counter == 1)
After calling 
 return fontData->AddRef(); // we have ref_counter == 2.

and After call ReleaseFont() we have ref_counter == 1, not zero.

[brucedawson, 2016-07-18 18:08:47]

brucedawson@chromium.org changed reviewers:
+ brucedawson@chromium.org

[brucedawson, 2016-07-18 18:08:47]

I verified the huge increase in memory when scrolling this PDF and verified the
memory savings after applying this fix.

[dsinclair, 2016-07-18 18:23:14]

On 2016/07/18 18:05:03, snake wrote:
> On 2016/07/18 17:56:13, dsinclair wrote:
> > Can you update the description with the memory usage after this CL is
applied?
> > 
> > For the various callers of GetFont() have we verified that they're all
> correctly
> > calling ReleaseFont() so they don't leak a ref as well?
> 
> The first ref is leaking:
>
https://cs.chromium.org/chromium/src/third_party/pdfium/core/fpdfapi/fpdf_pag...
>  fontData = new CPDF_CountedFont(pFont); // first ref (ref_counter == 1)
> After calling 
>  return fontData->AddRef(); // we have ref_counter == 2.
> 
> and After call ReleaseFont() we have ref_counter == 1, not zero.


Right, but have we checked that the callers of GetFont are also correctly
calling ReleaseFont? Otherwise, this would fix the first leaked font, but not
the subsequent ones, yea?

[snake, 2016-07-18 18:25:59]

On 2016/07/18 18:05:03, snake wrote:
> On 2016/07/18 17:56:13, dsinclair wrote:
> > Can you update the description with the memory usage after this CL is
applied?
> > 
> > For the various callers of GetFont() have we verified that they're all
> correctly
> > calling ReleaseFont() so they don't leak a ref as well?
> 
> The first ref is leaking:
>
https://cs.chromium.org/chromium/src/third_party/pdfium/core/fpdfapi/fpdf_pag...
>  fontData = new CPDF_CountedFont(pFont); // first ref (ref_counter == 1)
> After calling 
>  return fontData->AddRef(); // we have ref_counter == 2.
> 
> and After call ReleaseFont() we have ref_counter == 1, not zero.

Hmm...
Also i see same leaks for 
Images (stored in m_ImageMap)
pColorSpace (stored in m_ColorSpaceMap)
pPattern  (stored in m_PatternMap)
IccProfile (stored in m_IccProfileMap)
FontFiles (stored in m_FontFileMap)

Should i fix this too?

[snake, 2016-07-18 18:29:51]

On 2016/07/18 18:23:14, dsinclair wrote:
> On 2016/07/18 18:05:03, snake wrote:
> > On 2016/07/18 17:56:13, dsinclair wrote:
> > > Can you update the description with the memory usage after this CL is
> applied?
> > > 
> > > For the various callers of GetFont() have we verified that they're all
> > correctly
> > > calling ReleaseFont() so they don't leak a ref as well?
> > 
> > The first ref is leaking:
> >
>
https://cs.chromium.org/chromium/src/third_party/pdfium/core/fpdfapi/fpdf_pag...
> >  fontData = new CPDF_CountedFont(pFont); // first ref (ref_counter == 1)
> > After calling 
> >  return fontData->AddRef(); // we have ref_counter == 2.
> > 
> > and After call ReleaseFont() we have ref_counter == 1, not zero.
> 
> 
> Right, but have we checked that the callers of GetFont are also correctly
> calling ReleaseFont? Otherwise, this would fix the first leaked font, but not
> the subsequent ones, yea?

I did check this manualy.
All callers using CPDF_TextState or CPDF_TextStateData, which is like
intrusive_ptr for Fonts.

[snake, 2016-07-18 18:54:16]

Description was changed from

==========
Fix fonts leaking on ClosePage.

BUG=79367

The fonts was not cleared after unloading pages.

Test pdf:

http://www.nasa.gov/pdf/750614main_NASA_FY_2014_Budget_Estimates-508.pdf

For this file, we have ~5 fonts per page, which equal ~1 Mb per page.
In this PDF we have 670 pages, as result after slow scrolling(reading) full
document we have ~600 Mb fonts data in memory.
==========

to

==========
Fix fonts leaking on ClosePage.

BUG=79367

The fonts was not cleared after unloading pages.

Test pdf:

http://www.nasa.gov/pdf/750614main_NASA_FY_2014_Budget_Estimates-508.pdf

For this file, we have ~5 fonts per page, which equal ~1 Mb per page.
In this PDF we have 670 pages, as result after slow scrolling(reading) full
document we have ~600 Mb fonts data in memory.

UPD: Also fix other leaks like this.
==========

[dsinclair, 2016-07-18 19:08:03]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2016-07-18 19:08:03]

lgtm

[commit-bot: I haz the power, 2016-07-18 19:08:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-18 19:14:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-18 19:14:22]

Try jobs failed on following builders:
  linux_xfa on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/linux_xfa/build...)

[snake, 2016-07-18 19:29:24]

On 2016/07/18 19:14:22, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_xfa on master.tryserver.client.pdfium (JOB_FAILED,
>
https://build.chromium.org/p/tryserver.client.pdfium/builders/linux_xfa/build...)

plz rereview this CL.
I did commit some changes for fixing other leaks like font leak.

[dsinclair, 2016-07-18 20:13:48]

https://codereview.chromium.org/2158023002/diff/40001/core/fpdfapi/fpdf_page/...
File core/fpdfapi/fpdf_page/fpdf_page_doc.cpp (right):

https://codereview.chromium.org/2158023002/diff/40001/core/fpdfapi/fpdf_page/...
core/fpdfapi/fpdf_page/fpdf_page_doc.cpp:456: if (it_copied_stream !=
m_IccProfileMap.end())
Should we be removing the entry from IccProfileMap at the same time we remove it
from it_copied_stream instead of doing this?

[snake, 2016-07-19 18:55:14]

https://codereview.chromium.org/2158023002/diff/40001/core/fpdfapi/fpdf_page/...
File core/fpdfapi/fpdf_page/fpdf_page_doc.cpp (right):

https://codereview.chromium.org/2158023002/diff/40001/core/fpdfapi/fpdf_page/...
core/fpdfapi/fpdf_page/fpdf_page_doc.cpp:456: if (it_copied_stream !=
m_IccProfileMap.end())
On 2016/07/18 20:13:48, dsinclair wrote:
> Should we be removing the entry from IccProfileMap at the same time we remove
it
> from it_copied_stream instead of doing this?

No, To remove it, we should walk within full m_IccProfileMap, to find entry. And
this will be on each document unload. It is slow operation.
But the entry of IccProfileMap is tiny, and it will be reused on next document
load, and not necessary to remove it.

[dsinclair, 2016-07-19 18:56:38]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2016-07-19 18:56:39]

lgtm

[commit-bot: I haz the power, 2016-07-19 18:56:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-19 19:03:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-19 19:03:03]

Try jobs failed on following builders:
  mac_no_v8 on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/mac_no_v8/build...)
  win_xfa on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/win_xfa/builds/...)

[dsinclair, 2016-08-17 18:33:11]

Is this still valid?

[snake, 2016-09-07 17:39:43]

The CQ bit was checked by art-snake@yandex-team.ru

[snake, 2016-09-07 17:39:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
dsinclair@chromium.org
Link to the patchset: https://codereview.chromium.org/2158023002/#ps60001
(title: " ")

[snake, 2016-09-07 17:39:44]

The CQ bit was unchecked by art-snake@yandex-team.ru

[snake, 2016-09-07 17:39:51]

The CQ bit was checked by art-snake@yandex-team.ru

[commit-bot: I haz the power, 2016-09-07 17:39:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-07 18:39:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-07 18:39:47]

Try jobs failed on following builders:
  linux_xfa on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/linux_xfa/build...)

[snake, 2016-09-09 16:28:17]

On 2016/09/07 18:39:47, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   linux_xfa on master.tryserver.client.pdfium (JOB_FAILED,
>
https://build.chromium.org/p/tryserver.client.pdfium/builders/linux_xfa/build...)

I see deps error in TryBots result:
  gclient(669) ParseDepsFile:Found recursedeps "{'build_internal/scripts/slave':
{'deps_file': '.DEPS.git'}}".
But I did not add/change/remove any deps in my changes.

Also I see failed sorpus tests on TryBots.
  But, When I run its localy without my changes, I see failed result like on
TryBots, but it change, from run to run.
  Are sorpus tests flaking?

What next step should I do to commit this patch?

[brucedawson, 2016-09-09 18:16:11]

The CQ bit was checked by brucedawson@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 18:16:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 18:21:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-09 18:21:06]

Dry run: Try jobs failed on following builders:
  win on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/win/builds/2009)

[brucedawson, 2016-09-09 18:40:57]

On 2016/09/09 16:28:17, snake wrote:
> On 2016/09/07 18:39:47, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   linux_xfa on master.tryserver.client.pdfium (JOB_FAILED,
> >
>
https://build.chromium.org/p/tryserver.client.pdfium/builders/linux_xfa/build...)
> 
> I see deps error in TryBots result:
>   gclient(669) ParseDepsFile:Found recursedeps
"{'build_internal/scripts/slave':
> {'deps_file': '.DEPS.git'}}".
> But I did not add/change/remove any deps in my changes.
> 
> Also I see failed sorpus tests on TryBots.
>   But, When I run its localy without my changes, I see failed result like on
> TryBots, but it change, from run to run.
>   Are sorpus tests flaking?
> 
> What next step should I do to commit this patch?

I patched your changed in locally and did a "git cl try" and got similar
failures - you can see them above.

I also created a white-space only change and ran the try bots on that. It
initially looks like it is more successful, but I'm not convinced because the
'successful' results for mac_no_v8 show a lot of apparent failures:

https://build.chromium.org/p/tryserver.client.pdfium/builders/mac_no_v8/build...

Failed to open:
/b/build/slave/pdfium/build/pdfium/out/Debug/gen/pdfium/testing/corpus/example_027.evt
diff: 4.29% failed
FAILURE: example_024.pdf; Command
'['/b/build/slave/pdfium/build/pdfium/out/Debug/pdfium_diff',
'/b/build/slave/pdfium/build/pdfium/testing/corpus/third_party/tcpdf/example_024_expected.pdf.0.png',
'/b/build/slave/pdfium/build/pdfium/out/Debug/gen/pdfium/testing/corpus/example_024.pdf.0.png']'
returned non-zero exit status 1
Rendering PDF file
/b/build/slave/pdfium/build/pdfium/out/Debug/gen/pdfium/testing/corpus/example_028.pdf.

If those are expected failures then the output should indicate that. If they are
unexpected failures then the test harness is buggy.

I don't work on pdfium enough to know what is going on.

[snake, 2016-09-13 15:29:48]

On 2016/09/09 18:40:57, brucedawson wrote:
> On 2016/09/09 16:28:17, snake wrote:
> > On 2016/09/07 18:39:47, commit-bot: I haz the power wrote:
> > > Try jobs failed on following builders:
> > >   linux_xfa on master.tryserver.client.pdfium (JOB_FAILED,
> > >
> >
>
https://build.chromium.org/p/tryserver.client.pdfium/builders/linux_xfa/build...)
> > 
> > I see deps error in TryBots result:
> >   gclient(669) ParseDepsFile:Found recursedeps
> "{'build_internal/scripts/slave':
> > {'deps_file': '.DEPS.git'}}".
> > But I did not add/change/remove any deps in my changes.
> > 
> > Also I see failed sorpus tests on TryBots.
> >   But, When I run its localy without my changes, I see failed result like on
> > TryBots, but it change, from run to run.
> >   Are sorpus tests flaking?
> > 
> > What next step should I do to commit this patch?
> 
> I patched your changed in locally and did a "git cl try" and got similar
> failures - you can see them above.
> 
> I also created a white-space only change and ran the try bots on that. It
> initially looks like it is more successful, but I'm not convinced because the
> 'successful' results for mac_no_v8 show a lot of apparent failures:
> 
>
https://build.chromium.org/p/tryserver.client.pdfium/builders/mac_no_v8/build...
> 
> Failed to open:
>
/b/build/slave/pdfium/build/pdfium/out/Debug/gen/pdfium/testing/corpus/example_027.evt
> diff: 4.29% failed
> FAILURE: example_024.pdf; Command
> '['/b/build/slave/pdfium/build/pdfium/out/Debug/pdfium_diff',
>
'/b/build/slave/pdfium/build/pdfium/testing/corpus/third_party/tcpdf/example_024_expected.pdf.0.png',
>
'/b/build/slave/pdfium/build/pdfium/out/Debug/gen/pdfium/testing/corpus/example_024.pdf.0.png']'
> returned non-zero exit status 1
> Rendering PDF file
>
/b/build/slave/pdfium/build/pdfium/out/Debug/gen/pdfium/testing/corpus/example_028.pdf.
> 
> If those are expected failures then the output should indicate that. If they
are
> unexpected failures then the test harness is buggy.
> 
> I don't work on pdfium enough to know what is going on.

The problem is in CFX_FontCache::GetCachedFace (
https://cs.chromium.org/chromium/src/third_party/pdfium/core/fxge/ge/cfx_font...
).

When Fonts are unloaded, CFX_FontCache still contains the old fonts data. When I
disable this cache, all tests are finished with success also with my changes.

But I have not idea, how synchronize the CFX_FontCache and cache in
CPDF_DocPageData.

[dsinclair, 2016-09-13 15:47:44]

dsinclair@chromium.org changed reviewers:
+ npm@chromium.org

[npm_g, 2016-09-13 17:21:14]

On 2016/09/13 15:29:48, snake wrote:
> On 2016/09/09 18:40:57, brucedawson wrote:
> > On 2016/09/09 16:28:17, snake wrote:
> > > On 2016/09/07 18:39:47, commit-bot: I haz the power wrote:
> > > > Try jobs failed on following builders:
> > > >   linux_xfa on master.tryserver.client.pdfium (JOB_FAILED,
> > > >
> > >
> >
>
https://build.chromium.org/p/tryserver.client.pdfium/builders/linux_xfa/build...)
> > > 
> > > I see deps error in TryBots result:
> > >   gclient(669) ParseDepsFile:Found recursedeps
> > "{'build_internal/scripts/slave':
> > > {'deps_file': '.DEPS.git'}}".
> > > But I did not add/change/remove any deps in my changes.
> > > 
> > > Also I see failed sorpus tests on TryBots.
> > >   But, When I run its localy without my changes, I see failed result like
on
> > > TryBots, but it change, from run to run.
> > >   Are sorpus tests flaking?
> > > 
> > > What next step should I do to commit this patch?
> > 
> > I patched your changed in locally and did a "git cl try" and got similar
> > failures - you can see them above.
> > 
> > I also created a white-space only change and ran the try bots on that. It
> > initially looks like it is more successful, but I'm not convinced because
the
> > 'successful' results for mac_no_v8 show a lot of apparent failures:
> > 
> >
>
https://build.chromium.org/p/tryserver.client.pdfium/builders/mac_no_v8/build...
> > 
> > Failed to open:
> >
>
/b/build/slave/pdfium/build/pdfium/out/Debug/gen/pdfium/testing/corpus/example_027.evt
> > diff: 4.29% failed
> > FAILURE: example_024.pdf; Command
> > '['/b/build/slave/pdfium/build/pdfium/out/Debug/pdfium_diff',
> >
>
'/b/build/slave/pdfium/build/pdfium/testing/corpus/third_party/tcpdf/example_024_expected.pdf.0.png',
> >
>
'/b/build/slave/pdfium/build/pdfium/out/Debug/gen/pdfium/testing/corpus/example_024.pdf.0.png']'
> > returned non-zero exit status 1
> > Rendering PDF file
> >
>
/b/build/slave/pdfium/build/pdfium/out/Debug/gen/pdfium/testing/corpus/example_028.pdf.
> > 
> > If those are expected failures then the output should indicate that. If they
> are
> > unexpected failures then the test harness is buggy.
> > 
> > I don't work on pdfium enough to know what is going on.
> 
> The problem is in CFX_FontCache::GetCachedFace (
>
https://cs.chromium.org/chromium/src/third_party/pdfium/core/fxge/ge/cfx_font...
> ).
> 
> When Fonts are unloaded, CFX_FontCache still contains the old fonts data. When
I
> disable this cache, all tests are finished with success also with my changes.
> 
> But I have not idea, how synchronize the CFX_FontCache and cache in
> CPDF_DocPageData.

I don't think that the CPDF_CountedObject is counting the maps, which is why the
changes in this CL are not correct (the refs are being cleared while being used
elsewhere). The problem with this pdf is that it seems to define different fonts
for every page! It has a lot of Arial and Times New Roman (probably all
duplicates). By the time you load the last page, all of these duplicates will be
in cache. But clearing font cache after loading a page is not reasonable,
because most pdfs will not have separate font definitions in each page.

[snake, 2016-09-13 17:44:40]

On 2016/09/13 17:21:14, npm_g wrote:
> On 2016/09/13 15:29:48, snake wrote:
> > On 2016/09/09 18:40:57, brucedawson wrote:
> > > On 2016/09/09 16:28:17, snake wrote:
> > > > On 2016/09/07 18:39:47, commit-bot: I haz the power wrote:
> > > > > Try jobs failed on following builders:
> > > > >   linux_xfa on master.tryserver.client.pdfium (JOB_FAILED,
> > > > >
> > > >
> > >
> >
>
https://build.chromium.org/p/tryserver.client.pdfium/builders/linux_xfa/build...)
> > > > 
> > > > I see deps error in TryBots result:
> > > >   gclient(669) ParseDepsFile:Found recursedeps
> > > "{'build_internal/scripts/slave':
> > > > {'deps_file': '.DEPS.git'}}".
> > > > But I did not add/change/remove any deps in my changes.
> > > > 
> > > > Also I see failed sorpus tests on TryBots.
> > > >   But, When I run its localy without my changes, I see failed result
like
> on
> > > > TryBots, but it change, from run to run.
> > > >   Are sorpus tests flaking?
> > > > 
> > > > What next step should I do to commit this patch?
> > > 
> > > I patched your changed in locally and did a "git cl try" and got similar
> > > failures - you can see them above.
> > > 
> > > I also created a white-space only change and ran the try bots on that. It
> > > initially looks like it is more successful, but I'm not convinced because
> the
> > > 'successful' results for mac_no_v8 show a lot of apparent failures:
> > > 
> > >
> >
>
https://build.chromium.org/p/tryserver.client.pdfium/builders/mac_no_v8/build...
> > > 
> > > Failed to open:
> > >
> >
>
/b/build/slave/pdfium/build/pdfium/out/Debug/gen/pdfium/testing/corpus/example_027.evt
> > > diff: 4.29% failed
> > > FAILURE: example_024.pdf; Command
> > > '['/b/build/slave/pdfium/build/pdfium/out/Debug/pdfium_diff',
> > >
> >
>
'/b/build/slave/pdfium/build/pdfium/testing/corpus/third_party/tcpdf/example_024_expected.pdf.0.png',
> > >
> >
>
'/b/build/slave/pdfium/build/pdfium/out/Debug/gen/pdfium/testing/corpus/example_024.pdf.0.png']'
> > > returned non-zero exit status 1
> > > Rendering PDF file
> > >
> >
>
/b/build/slave/pdfium/build/pdfium/out/Debug/gen/pdfium/testing/corpus/example_028.pdf.
> > > 
> > > If those are expected failures then the output should indicate that. If
they
> > are
> > > unexpected failures then the test harness is buggy.
> > > 
> > > I don't work on pdfium enough to know what is going on.
> > 
> > The problem is in CFX_FontCache::GetCachedFace (
> >
>
https://cs.chromium.org/chromium/src/third_party/pdfium/core/fxge/ge/cfx_font...
> > ).
> > 
> > When Fonts are unloaded, CFX_FontCache still contains the old fonts data.
When
> I
> > disable this cache, all tests are finished with success also with my
changes.
> > 
> > But I have not idea, how synchronize the CFX_FontCache and cache in
> > CPDF_DocPageData.
> 
> I don't think that the CPDF_CountedObject is counting the maps, which is why
the
> changes in this CL are not correct (the refs are being cleared while being
used
> elsewhere). The problem with this pdf is that it seems to define different
fonts
> for every page! It has a lot of Arial and Times New Roman (probably all
> duplicates). By the time you load the last page, all of these duplicates will
be
> in cache. But clearing font cache after loading a page is not reasonable,
> because most pdfs will not have separate font definitions in each page.

But after loading the last page, all other pages are unloaded, and not refs are
used for unloaded pages, but cache is not cleared!!

And without my changes, clean code will be never called (but should be called on
PageUnload, as I see in code.).

Why we need this, if we do not clear cache?
   pFontData->clear();
   pPattern->clear();
   ....

Most pdfs will have font in each page. Each font have size ~500kb. For any pdfs
we allocate ~page_count*500Kb memory.
For example: 
For 100 pages pdf we should allocate min 50 Mb.
For 500 pages pdf we should allocate min 250 Mb.
...

But if we have allocated fonts only for visible pages, we have
~visible_pages_count*500kb allocated data on any pdf document.

Also, as I see in http://crbug.com/48791 we should unload any unused data on
PageUnload to reduce memory usage.

[snake, 2016-09-13 17:49:41]

Description was changed from

==========
Fix fonts leaking on ClosePage.

BUG=79367

The fonts was not cleared after unloading pages.

Test pdf:

http://www.nasa.gov/pdf/750614main_NASA_FY_2014_Budget_Estimates-508.pdf

For this file, we have ~5 fonts per page, which equal ~1 Mb per page.
In this PDF we have 670 pages, as result after slow scrolling(reading) full
document we have ~600 Mb fonts data in memory.

UPD: Also fix other leaks like this.
==========

to

==========
Fix fonts leaking on ClosePage.

BUG=79367,48791

The fonts was not cleared after unloading pages.

Test pdf:

http://www.nasa.gov/pdf/750614main_NASA_FY_2014_Budget_Estimates-508.pdf

For this file, we have ~5 fonts per page, which equal ~1 Mb per page.
In this PDF we have 670 pages, as result after slow scrolling(reading) full
document we have ~600 Mb fonts data in memory.

UPD: Also fix other leaks like this.
==========

[dsinclair, 2016-09-13 17:57:48]

On 2016/09/13 17:44:40, snake wrote:
> But after loading the last page, all other pages are unloaded, and not refs
are
> used for unloaded pages, but cache is not cleared!!
> 
> And without my changes, clean code will be never called (but should be called
on
> PageUnload, as I see in code.).
> 
> Why we need this, if we do not clear cache?
>    pFontData->clear();
>    pPattern->clear();
>    ....
> 
> Most pdfs will have font in each page. Each font have size ~500kb. For any
pdfs
> we allocate ~page_count*500Kb memory.


How common is it to use different fonts on every page of a given PDF? Won't most
PDFs load one or two fonts and use those over all pages? 



> Also, as I see in http://crbug.com/48791 we should unload any unused data on
> PageUnload to reduce memory usage.


If the font is not used on the current page, we should probably be unloading it.

[snake, 2016-09-13 18:27:11]

On 2016/09/13 17:57:48, dsinclair wrote:
> On 2016/09/13 17:44:40, snake wrote:
> > But after loading the last page, all other pages are unloaded, and not refs
> are
> > used for unloaded pages, but cache is not cleared!!
> > 
> > And without my changes, clean code will be never called (but should be
called
> on
> > PageUnload, as I see in code.).
> > 
> > Why we need this, if we do not clear cache?
> >    pFontData->clear();
> >    pPattern->clear();
> >    ....
> > 
> > Most pdfs will have font in each page. Each font have size ~500kb. For any
> pdfs
> > we allocate ~page_count*500Kb memory.
> 
> 
> How common is it to use different fonts on every page of a given PDF? Won't
most
> PDFs load one or two fonts and use those over all pages? 
> 
Each page contains fonts dictionary, which will be loaded separately (but realy
it can be identical to other pages).
> 
> 
> > Also, as I see in http://crbug.com/48791 we should unload any unused data on
> > PageUnload to reduce memory usage.
> 
> 
> If the font is not used on the current page, we should probably be unloading
it.

Without my changes "pFontData->clear()" will be never called. (I did add
ASSERT(false), and did run tests and plugin to check this).
Now The Fonts Data is cleared only in destructor, but should on PageClose
action.

[npm, 2016-09-13 21:50:14]

On 2016/09/13 18:27:11, snake wrote:
> On 2016/09/13 17:57:48, dsinclair wrote:
> > On 2016/09/13 17:44:40, snake wrote:
> > > But after loading the last page, all other pages are unloaded, and not
refs
> > are
> > > used for unloaded pages, but cache is not cleared!!
> > > 
> > > And without my changes, clean code will be never called (but should be
> called
> > on
> > > PageUnload, as I see in code.).
> > > 
> > > Why we need this, if we do not clear cache?
> > >    pFontData->clear();
> > >    pPattern->clear();
> > >    ....
> > > 
> > > Most pdfs will have font in each page. Each font have size ~500kb. For any
> > pdfs
> > > we allocate ~page_count*500Kb memory.
> > 
> > 
> > How common is it to use different fonts on every page of a given PDF? Won't
> most
> > PDFs load one or two fonts and use those over all pages? 
> > 
> Each page contains fonts dictionary, which will be loaded separately (but
realy
> it can be identical to other pages).
> > 
> > 
> > > Also, as I see in http://crbug.com/48791 we should unload any unused data
on
> > > PageUnload to reduce memory usage.
> > 
> > 
> > If the font is not used on the current page, we should probably be unloading
> it.
> 
> Without my changes "pFontData->clear()" will be never called. (I did add
> ASSERT(false), and did run tests and plugin to check this).
> Now The Fonts Data is cleared only in destructor, but should on PageClose
> action.

Ah, you're right. new CPDF_CountedFont initializes the count to 1. 
So the change makes sense.

The caches you mention do not cache exactly the same objects, although it's
true that deleting the object from the CPDF_DocPageData affects something else.
I did the change in ReleaseFont and ran corpus tests, They fail, and there is
a noticeable flaw in rendering in at least one test case.

[snake, 2016-09-14 10:37:36]

On 2016/09/13 21:50:14, npm wrote:
> On 2016/09/13 18:27:11, snake wrote:
> > On 2016/09/13 17:57:48, dsinclair wrote:
> > > On 2016/09/13 17:44:40, snake wrote:
> > > > But after loading the last page, all other pages are unloaded, and not
> refs
> > > are
> > > > used for unloaded pages, but cache is not cleared!!
> > > > 
> > > > And without my changes, clean code will be never called (but should be
> > called
> > > on
> > > > PageUnload, as I see in code.).
> > > > 
> > > > Why we need this, if we do not clear cache?
> > > >    pFontData->clear();
> > > >    pPattern->clear();
> > > >    ....
> > > > 
> > > > Most pdfs will have font in each page. Each font have size ~500kb. For
any
> > > pdfs
> > > > we allocate ~page_count*500Kb memory.
> > > 
> > > 
> > > How common is it to use different fonts on every page of a given PDF?
Won't
> > most
> > > PDFs load one or two fonts and use those over all pages? 
> > > 
> > Each page contains fonts dictionary, which will be loaded separately (but
> realy
> > it can be identical to other pages).
> > > 
> > > 
> > > > Also, as I see in http://crbug.com/48791 we should unload any unused
data
> on
> > > > PageUnload to reduce memory usage.
> > > 
> > > 
> > > If the font is not used on the current page, we should probably be
unloading
> > it.
> > 
> > Without my changes "pFontData->clear()" will be never called. (I did add
> > ASSERT(false), and did run tests and plugin to check this).
> > Now The Fonts Data is cleared only in destructor, but should on PageClose
> > action.
> 
> Ah, you're right. new CPDF_CountedFont initializes the count to 1. 
> So the change makes sense.
> 
> The caches you mention do not cache exactly the same objects, although it's
> true that deleting the object from the CPDF_DocPageData affects something
else.
> I did the change in ReleaseFont and ran corpus tests, They fail, and there is
> a noticeable flaw in rendering in at least one test case.

The current CFX_FontCache and CPDF_DocPageData implementation works like this:

CPDF_CountedFont contains CPDF_Font
(https://cs.chromium.org/chromium/src/third_party/pdfium/core/fpdfapi/fpdf_pag...
)
CPDF_Font contains CFX_Font
(https://cs.chromium.org/chromium/src/third_party/pdfium/core/fpdfapi/fpdf_fon...
)
CFX_Font contains FXFT_FACE
(https://cs.chromium.org/chromium/src/third_party/pdfium/core/fxge/include/fx_...)

CFX_FontCache::GetCachedFace caches the CFX_FaceCache by FXFT_FACE key, which is
a pointer. (
https://cs.chromium.org/chromium/src/third_party/pdfium/core/fxge/include/cfx...
)

In destructor of CFX_Font the FXFT_FACE is being released (
https://cs.chromium.org/chromium/src/third_party/pdfium/core/fxge/ge/cfx_font...
) , and as result in the FontCache we have an invalid cached object.
But when we create a new CPDF_Font (and FXFT_FACE alongside it), quite
frequently the FXFT_FACE is allocated at the *same address* as just released
FXFT_FACE (maybe because of a custom allocator?). 

And as a result in CFX_FontCache we already have item for new FXFT_FACE, but in
CFX_FaceCache (
https://cs.chromium.org/chromium/src/third_party/pdfium/core/fxge/include/cfx...
), we have Glyphs and PathData
(https://cs.chromium.org/chromium/src/third_party/pdfium/core/fxge/include/cfx...
) for previous ( deleted ) FXFT_FACE.

i.e. when we release the FXFT_FACE it is *not removed* from the CFX_FontCache.
Then when we try to access the CFX_FontCache using a new FXFT_FACE (which was
allocated on the same address as an old released one) it will give us the
outdated CFX_FaceCache data, which will result in slightly different font being
used, and the tests fail as result of that.

What would you advice me to do to solve this invalid cache issue?

[dsinclair, 2016-09-14 12:46:21]

On 2016/09/14 10:37:36, snake wrote:
> The current CFX_FontCache and CPDF_DocPageData implementation works like this:
> 
> CPDF_CountedFont contains CPDF_Font
>
(https://cs.chromium.org/chromium/src/third_party/pdfium/core/fpdfapi/fpdf_pag...
> )
> CPDF_Font contains CFX_Font
>
(https://cs.chromium.org/chromium/src/third_party/pdfium/core/fpdfapi/fpdf_fon...
> )
> CFX_Font contains FXFT_FACE
>
(https://cs.chromium.org/chromium/src/third_party/pdfium/core/fxge/include/fx_...)
> 
> CFX_FontCache::GetCachedFace caches the CFX_FaceCache by FXFT_FACE key, which
is
> a pointer. (
>
https://cs.chromium.org/chromium/src/third_party/pdfium/core/fxge/include/cfx...
> )
> 
> In destructor of CFX_Font the FXFT_FACE is being released (
>
https://cs.chromium.org/chromium/src/third_party/pdfium/core/fxge/ge/cfx_font...
> ) , and as result in the FontCache we have an invalid cached object.
> But when we create a new CPDF_Font (and FXFT_FACE alongside it), quite
> frequently the FXFT_FACE is allocated at the *same address* as just released
> FXFT_FACE (maybe because of a custom allocator?). 
> 
> And as a result in CFX_FontCache we already have item for new FXFT_FACE, but
in
> CFX_FaceCache (
>
https://cs.chromium.org/chromium/src/third_party/pdfium/core/fxge/include/cfx...
> ), we have Glyphs and PathData
>
(https://cs.chromium.org/chromium/src/third_party/pdfium/core/fxge/include/cfx...
> ) for previous ( deleted ) FXFT_FACE.
> 
> i.e. when we release the FXFT_FACE it is *not removed* from the CFX_FontCache.
> Then when we try to access the CFX_FontCache using a new FXFT_FACE (which was
> allocated on the same address as an old released one) it will give us the
> outdated CFX_FaceCache data, which will result in slightly different font
being
> used, and the tests fail as result of that.
> 
> What would you advice me to do to solve this invalid cache issue?


A few questions:
  * Are both caches needed? Can we rework the code to remove one of them?
  * When we free the FXFT_FACE do we have any way to get to the CFX_FontCache to
cleanup?
  * Should we wrap the FXFT_FACE in something ref-counted so we maintain the
lifetime?
     * This has the downside that we'll probably never free it.

I think we need to sit down and figure out where all the font caches work (there
are a bunch of them I believe) and figure out if they're all needed and how they
work together.

[snake, 2016-09-14 17:08:22]

On 2016/09/14 12:46:21, dsinclair wrote:
> On 2016/09/14 10:37:36, snake wrote:
> > The current CFX_FontCache and CPDF_DocPageData implementation works like
this:
> > 
> > CPDF_CountedFont contains CPDF_Font
> >
>
(https://cs.chromium.org/chromium/src/third_party/pdfium/core/fpdfapi/fpdf_pag...
> > )
> > CPDF_Font contains CFX_Font
> >
>
(https://cs.chromium.org/chromium/src/third_party/pdfium/core/fpdfapi/fpdf_fon...
> > )
> > CFX_Font contains FXFT_FACE
> >
>
(https://cs.chromium.org/chromium/src/third_party/pdfium/core/fxge/include/fx_...)
> > 
> > CFX_FontCache::GetCachedFace caches the CFX_FaceCache by FXFT_FACE key,
which
> is
> > a pointer. (
> >
>
https://cs.chromium.org/chromium/src/third_party/pdfium/core/fxge/include/cfx...
> > )
> > 
> > In destructor of CFX_Font the FXFT_FACE is being released (
> >
>
https://cs.chromium.org/chromium/src/third_party/pdfium/core/fxge/ge/cfx_font...
> > ) , and as result in the FontCache we have an invalid cached object.
> > But when we create a new CPDF_Font (and FXFT_FACE alongside it), quite
> > frequently the FXFT_FACE is allocated at the *same address* as just released
> > FXFT_FACE (maybe because of a custom allocator?). 
> > 
> > And as a result in CFX_FontCache we already have item for new FXFT_FACE, but
> in
> > CFX_FaceCache (
> >
>
https://cs.chromium.org/chromium/src/third_party/pdfium/core/fxge/include/cfx...
> > ), we have Glyphs and PathData
> >
>
(https://cs.chromium.org/chromium/src/third_party/pdfium/core/fxge/include/cfx...
> > ) for previous ( deleted ) FXFT_FACE.
> > 
> > i.e. when we release the FXFT_FACE it is *not removed* from the
CFX_FontCache.
> > Then when we try to access the CFX_FontCache using a new FXFT_FACE (which
was
> > allocated on the same address as an old released one) it will give us the
> > outdated CFX_FaceCache data, which will result in slightly different font
> being
> > used, and the tests fail as result of that.
> > 
> > What would you advice me to do to solve this invalid cache issue?
> 
> 
> A few questions:
>   * Are both caches needed? Can we rework the code to remove one of them?
>   * When we free the FXFT_FACE do we have any way to get to the CFX_FontCache
to
> cleanup?
>   * Should we wrap the FXFT_FACE in something ref-counted so we maintain the
> lifetime?
>      * This has the downside that we'll probably never free it.
> 
> I think we need to sit down and figure out where all the font caches work
(there
> are a bunch of them I believe) and figure out if they're all needed and how
they
> work together.

I did refactore logic for using CFX_FontCache.

I did move using it logic into CFX_Font class. And now CFX_FontCache is
synchronized with life time of CFX_Font.

pls rereview.

[snake, 2016-09-14 17:08:42]

The CQ bit was checked by art-snake@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 17:08:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[snake, 2016-09-14 17:08:50]

The CQ bit was unchecked by art-snake@yandex-team.ru

[snake, 2016-09-14 17:23:50]

> Are both caches needed? Can we rework the code to remove one of them?
Yes, both are needed: 
CPDF_DocPageData is a per-page cache, and fonts are not sharing between pages.

Second one is used when painting the individual pages: CFX_FontCache and
CFX_FaceCache are used together for this. It is global share fonts data.

>   * When we free the FXFT_FACE do we have any way to get to the CFX_FontCache
to cleanup?

I've refactored the code to make it possible.

>   * Should we wrap the FXFT_FACE in something ref-counted so we maintain the
> lifetime?
>      * This has the downside that we'll probably never free it.

They're already ref-counted by CFX_FontManager.

[snake, 2016-09-14 17:24:12]

The CQ bit was checked by art-snake@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 17:24:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dsinclair, 2016-09-14 17:24:47]

The CQ bit was checked by dsinclair@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 17:24:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 17:27:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-14 17:27:59]

Dry run: Try jobs failed on following builders:
  android on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/android/builds/750)

[snake, 2016-09-14 17:43:16]

The CQ bit was checked by art-snake@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 17:43:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dsinclair, 2016-09-14 17:51:51]

I'm still a bit confused about the purpose of the two levels of caching. Is it
correct that one cache is for the whole Document and the other cache is per
page?

If so, why do we end up caching the same font in both places? Shouldn't the page
cache check the document cache before caching the font?

https://codereview.chromium.org/2158023002/diff/80001/core/fxge/ge/cfx_font.cpp
File core/fxge/ge/cfx_font.cpp (right):

https://codereview.chromium.org/2158023002/diff/80001/core/fxge/ge/cfx_font.c...
core/fxge/ge/cfx_font.cpp:269: m_FaceCache = pFont->GetFaceCache();
What about m_FontCache?

https://codereview.chromium.org/2158023002/diff/80001/core/fxge/ge/cfx_font.c...
core/fxge/ge/cfx_font.cpp:550: const_cast<CFX_Font*>(this));
We should remove the const from the signature instead of doing const_cast

https://codereview.chromium.org/2158023002/diff/80001/core/fxge/ge/cfx_render...
File core/fxge/ge/cfx_renderdevice.cpp (right):

https://codereview.chromium.org/2158023002/diff/80001/core/fxge/ge/cfx_render...
core/fxge/ge/cfx_renderdevice.cpp:917: glyph.m_pGlyph =
pFont->GetFontCache()->LoadGlyphBitmap(
It's strange we replaced the FaceCache with a FontCache. Should GetFontCache be
called GetFaceCache?

https://codereview.chromium.org/2158023002/diff/80001/core/fxge/include/cfx_f...
File core/fxge/include/cfx_fontcache.h (right):

https://codereview.chromium.org/2158023002/diff/80001/core/fxge/include/cfx_f...
core/fxge/include/cfx_fontcache.h:34: using CFX_FTCacheMap = std::map<FXFT_Face,
CountedFaceCache*>;
Can CountedFaceCache be a unique_ptr?

https://codereview.chromium.org/2158023002/diff/80001/core/fxge/include/fx_fo...
File core/fxge/include/fx_font.h (right):

https://codereview.chromium.org/2158023002/diff/80001/core/fxge/include/fx_fo...
core/fxge/include/fx_font.h:144: const CFX_FontCacheItem* GetFontCache() const {
return m_FontCache.get(); }
This isn't really a cache as it's always just one item? What do we need this
for?

https://codereview.chromium.org/2158023002/diff/80001/core/fxge/include/fx_fo...
core/fxge/include/fx_font.h:163: CFX_PathData* LoadGlyphPath(uint32_t
glyph_index, int dest_width = 0);
Why not leave this public then we don't need the friend entry.

https://codereview.chromium.org/2158023002/diff/80001/core/fxge/include/fx_fo...
core/fxge/include/fx_font.h:172: mutable CFX_FaceCache* m_FaceCache;
Why mutable?

What is the ownership of this? If it isn't owned by us can you add a:

  // not owned.

If it is owned, it should be a std::unique_ptr.

[commit-bot: I haz the power, 2016-09-14 17:55:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-14 17:55:56]

Dry run: Try jobs failed on following builders:
  linux_xfa on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/linux_xfa/build...)

[npm, 2016-09-14 19:04:14]

On 2016/09/14 17:51:51, dsinclair wrote:
> I'm still a bit confused about the purpose of the two levels of caching. Is it
> correct that one cache is for the whole Document and the other cache is per
> page?
> 
> If so, why do we end up caching the same font in both places? Shouldn't the
page
> cache check the document cache before caching the font?
> 
>
https://codereview.chromium.org/2158023002/diff/80001/core/fxge/ge/cfx_font.cpp
> File core/fxge/ge/cfx_font.cpp (right):
> 
>
https://codereview.chromium.org/2158023002/diff/80001/core/fxge/ge/cfx_font.c...
> core/fxge/ge/cfx_font.cpp:269: m_FaceCache = pFont->GetFaceCache();
> What about m_FontCache?
> 
>
https://codereview.chromium.org/2158023002/diff/80001/core/fxge/ge/cfx_font.c...
> core/fxge/ge/cfx_font.cpp:550: const_cast<CFX_Font*>(this));
> We should remove the const from the signature instead of doing const_cast
> 
>
https://codereview.chromium.org/2158023002/diff/80001/core/fxge/ge/cfx_render...
> File core/fxge/ge/cfx_renderdevice.cpp (right):
> 
>
https://codereview.chromium.org/2158023002/diff/80001/core/fxge/ge/cfx_render...
> core/fxge/ge/cfx_renderdevice.cpp:917: glyph.m_pGlyph =
> pFont->GetFontCache()->LoadGlyphBitmap(
> It's strange we replaced the FaceCache with a FontCache. Should GetFontCache
be
> called GetFaceCache?
> 
>
https://codereview.chromium.org/2158023002/diff/80001/core/fxge/include/cfx_f...
> File core/fxge/include/cfx_fontcache.h (right):
> 
>
https://codereview.chromium.org/2158023002/diff/80001/core/fxge/include/cfx_f...
> core/fxge/include/cfx_fontcache.h:34: using CFX_FTCacheMap =
std::map<FXFT_Face,
> CountedFaceCache*>;
> Can CountedFaceCache be a unique_ptr?
> 
>
https://codereview.chromium.org/2158023002/diff/80001/core/fxge/include/fx_fo...
> File core/fxge/include/fx_font.h (right):
> 
>
https://codereview.chromium.org/2158023002/diff/80001/core/fxge/include/fx_fo...
> core/fxge/include/fx_font.h:144: const CFX_FontCacheItem* GetFontCache() const
{
> return m_FontCache.get(); }
> This isn't really a cache as it's always just one item? What do we need this
> for?
> 
>
https://codereview.chromium.org/2158023002/diff/80001/core/fxge/include/fx_fo...
> core/fxge/include/fx_font.h:163: CFX_PathData* LoadGlyphPath(uint32_t
> glyph_index, int dest_width = 0);
> Why not leave this public then we don't need the friend entry.
> 
>
https://codereview.chromium.org/2158023002/diff/80001/core/fxge/include/fx_fo...
> core/fxge/include/fx_font.h:172: mutable CFX_FaceCache* m_FaceCache;
> Why mutable?
> 
> What is the ownership of this? If it isn't owned by us can you add a:
> 
>   // not owned.
> 
> If it is owned, it should be a std::unique_ptr.

One problem is that the page-level cache caches CPDF_Fonts and the
document-level caches FaceCaches (but I don't know why).

I am not sure I understand what you are trying to accomplish with the
changes. But if the problem is CFX_Font deletes the face when it should
not, why not change the destructor so that it leaves it alone?

[snake, 2016-09-14 19:06:30]

https://codereview.chromium.org/2158023002/diff/80001/core/fxge/ge/cfx_font.cpp
File core/fxge/ge/cfx_font.cpp (right):

https://codereview.chromium.org/2158023002/diff/80001/core/fxge/ge/cfx_font.c...
core/fxge/ge/cfx_font.cpp:269: m_FaceCache = pFont->GetFaceCache();
On 2016/09/14 17:51:50, dsinclair wrote:
> What about m_FontCache?

m_FontCache it is just wrapper (which will never changed for CFX_Font instance),
which translate calls to m_FaceCache.
It is allocated once in constructor.
UPD: I did remove wrapper "m_FontCache"

https://codereview.chromium.org/2158023002/diff/80001/core/fxge/ge/cfx_font.c...
core/fxge/ge/cfx_font.cpp:550: const_cast<CFX_Font*>(this));
On 2016/09/14 17:51:50, dsinclair wrote:
> We should remove the const from the signature instead of doing const_cast

Done

https://codereview.chromium.org/2158023002/diff/80001/core/fxge/ge/cfx_render...
File core/fxge/ge/cfx_renderdevice.cpp (right):

https://codereview.chromium.org/2158023002/diff/80001/core/fxge/ge/cfx_render...
core/fxge/ge/cfx_renderdevice.cpp:917: glyph.m_pGlyph =
pFont->GetFontCache()->LoadGlyphBitmap(
On 2016/09/14 17:51:51, dsinclair wrote:
> It's strange we replaced the FaceCache with a FontCache. Should GetFontCache
be
> called GetFaceCache?
I did remove GetFontCache method, now should call direct pFont methods
(pFont->LoadGlyphBitmap in this case)

https://codereview.chromium.org/2158023002/diff/80001/core/fxge/include/cfx_f...
File core/fxge/include/cfx_fontcache.h (right):

https://codereview.chromium.org/2158023002/diff/80001/core/fxge/include/cfx_f...
core/fxge/include/cfx_fontcache.h:34: using CFX_FTCacheMap = std::map<FXFT_Face,
CountedFaceCache*>;
On 2016/09/14 17:51:51, dsinclair wrote:
> Can CountedFaceCache be a unique_ptr?

Done
Now used unique_ptr

https://codereview.chromium.org/2158023002/diff/80001/core/fxge/include/fx_fo...
File core/fxge/include/fx_font.h (right):

https://codereview.chromium.org/2158023002/diff/80001/core/fxge/include/fx_fo...
core/fxge/include/fx_font.h:144: const CFX_FontCacheItem* GetFontCache() const {
return m_FontCache.get(); }
On 2016/09/14 17:51:51, dsinclair wrote:
> This isn't really a cache as it's always just one item? What do we need this
> for?
I did remove m_FontCache member, now CFX_Font translate calls to cache directly.

https://codereview.chromium.org/2158023002/diff/80001/core/fxge/include/fx_fo...
core/fxge/include/fx_font.h:163: CFX_PathData* LoadGlyphPath(uint32_t
glyph_index, int dest_width = 0);
This method is not using cache, for cleanly any caller should call method, which
use cache.

https://codereview.chromium.org/2158023002/diff/80001/core/fxge/include/fx_fo...
core/fxge/include/fx_font.h:172: mutable CFX_FaceCache* m_FaceCache;
On 2016/09/14 17:51:51, dsinclair wrote:
> Why mutable?
> 
It is initialized on demand in GetFaceCache() const getter.
In other case we should initialize it in much of initialization methods
(LoadSubst, LoadEmbeded, SetFace, ...)

> What is the ownership of this? If it isn't owned by us can you add a:
> 
>   // not owned.
> 
> If it is owned, it should be a std::unique_ptr.
Done

[snake, 2016-09-14 19:10:01]

> I'm still a bit confused about the purpose of the two levels of caching. Is it
> correct that one cache is for the whole Document and the other cache is per
> page?
> 
> If so, why do we end up caching the same font in both places? Shouldn't the
page
> cache check the document cache before caching the font?

CPDF_DocPageData - it is cache for pdf objects within Document. Because each
page contains its Font Dictionary, for fonts it is working like cache per page.
CFX_FontCache - it is cache for fonts representation on system, which
independent from PDF.

[snake, 2016-09-14 19:12:56]

On 2016/09/14 19:04:14, npm wrote:
> On 2016/09/14 17:51:51, dsinclair wrote:
> > I'm still a bit confused about the purpose of the two levels of caching. Is
it
> > correct that one cache is for the whole Document and the other cache is per
> > page?
> > 
> > If so, why do we end up caching the same font in both places? Shouldn't the
> page
> > cache check the document cache before caching the font?
> > 
> >
>
https://codereview.chromium.org/2158023002/diff/80001/core/fxge/ge/cfx_font.cpp
> > File core/fxge/ge/cfx_font.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2158023002/diff/80001/core/fxge/ge/cfx_font.c...
> > core/fxge/ge/cfx_font.cpp:269: m_FaceCache = pFont->GetFaceCache();
> > What about m_FontCache?
> > 
> >
>
https://codereview.chromium.org/2158023002/diff/80001/core/fxge/ge/cfx_font.c...
> > core/fxge/ge/cfx_font.cpp:550: const_cast<CFX_Font*>(this));
> > We should remove the const from the signature instead of doing const_cast
> > 
> >
>
https://codereview.chromium.org/2158023002/diff/80001/core/fxge/ge/cfx_render...
> > File core/fxge/ge/cfx_renderdevice.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2158023002/diff/80001/core/fxge/ge/cfx_render...
> > core/fxge/ge/cfx_renderdevice.cpp:917: glyph.m_pGlyph =
> > pFont->GetFontCache()->LoadGlyphBitmap(
> > It's strange we replaced the FaceCache with a FontCache. Should GetFontCache
> be
> > called GetFaceCache?
> > 
> >
>
https://codereview.chromium.org/2158023002/diff/80001/core/fxge/include/cfx_f...
> > File core/fxge/include/cfx_fontcache.h (right):
> > 
> >
>
https://codereview.chromium.org/2158023002/diff/80001/core/fxge/include/cfx_f...
> > core/fxge/include/cfx_fontcache.h:34: using CFX_FTCacheMap =
> std::map<FXFT_Face,
> > CountedFaceCache*>;
> > Can CountedFaceCache be a unique_ptr?
> > 
> >
>
https://codereview.chromium.org/2158023002/diff/80001/core/fxge/include/fx_fo...
> > File core/fxge/include/fx_font.h (right):
> > 
> >
>
https://codereview.chromium.org/2158023002/diff/80001/core/fxge/include/fx_fo...
> > core/fxge/include/fx_font.h:144: const CFX_FontCacheItem* GetFontCache()
const
> {
> > return m_FontCache.get(); }
> > This isn't really a cache as it's always just one item? What do we need this
> > for?
> > 
> >
>
https://codereview.chromium.org/2158023002/diff/80001/core/fxge/include/fx_fo...
> > core/fxge/include/fx_font.h:163: CFX_PathData* LoadGlyphPath(uint32_t
> > glyph_index, int dest_width = 0);
> > Why not leave this public then we don't need the friend entry.
> > 
> >
>
https://codereview.chromium.org/2158023002/diff/80001/core/fxge/include/fx_fo...
> > core/fxge/include/fx_font.h:172: mutable CFX_FaceCache* m_FaceCache;
> > Why mutable?
> > 
> > What is the ownership of this? If it isn't owned by us can you add a:
> > 
> >   // not owned.
> > 
> > If it is owned, it should be a std::unique_ptr.
> 
> One problem is that the page-level cache caches CPDF_Fonts and the
> document-level caches FaceCaches (but I don't know why).
> 
> I am not sure I understand what you are trying to accomplish with the
> changes. But if the problem is CFX_Font deletes the face when it should
> not, why not change the destructor so that it leaves it alone?

CFX_Font should deletes the face, and CFX_FontCache should deletes its to, when
its are not used.

[snake, 2016-09-14 19:13:52]

The CQ bit was checked by art-snake@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 19:13:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 19:16:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-14 19:16:29]

Dry run: Try jobs failed on following builders:
  linux on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/linux/builds/2057)

[snake, 2016-09-14 19:20:59]

The CQ bit was checked by art-snake@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 19:21:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[snake, 2016-09-14 19:25:11]

The CQ bit was checked by art-snake@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 19:25:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 19:45:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-14 19:45:09]

Dry run: Try jobs failed on following builders:
  mac_xfa on master.tryserver.client.pdfium (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.pdfium/builders/mac_xfa/builds/...)

[snake, 2016-09-14 20:19:06]

The CQ bit was checked by art-snake@yandex-team.ru to run a CQ dry run

[commit-bot: I haz the power, 2016-09-14 20:19:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-14 20:50:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-14 20:50:08]

Dry run: This issue passed the CQ dry run.

[npm, 2016-09-14 23:07:59]

Your suggested change now uses the cfx_fontcache from the ge module. This is a
global cache, whereas the previous docdata cache was a cache per document. Is
this really what we want?

https://codereview.chromium.org/2158023002/diff/180001/core/fpdfapi/fpdf_page...
File core/fpdfapi/fpdf_page/fpdf_page_doc.cpp (right):

https://codereview.chromium.org/2158023002/diff/180001/core/fpdfapi/fpdf_page...
core/fpdfapi/fpdf_page/fpdf_page_doc.cpp:462: return
it_copied_stream->second->AddRef();
If the code reaches to this point but the if statement is false,
m_HashProfileMap[bsDigest] is replaced below even though it is not null. Is that
fine?

https://codereview.chromium.org/2158023002/diff/180001/core/fxge/ge/cfx_font.cpp
File core/fxge/ge/cfx_font.cpp (right):

https://codereview.chromium.org/2158023002/diff/180001/core/fxge/ge/cfx_font....
core/fxge/ge/cfx_font.cpp:554: m_FaceCache =
CFX_GEModule::Get()->GetFontCache()->GetCachedFace(this);
Now you always get the facecache from CFX_GEModule.

https://codereview.chromium.org/2158023002/diff/180001/core/fxge/ge/cfx_fontc...
File core/fxge/ge/cfx_fontcache.cpp (right):

https://codereview.chromium.org/2158023002/diff/180001/core/fxge/ge/cfx_fontc...
core/fxge/ge/cfx_fontcache.cpp:61: } else {
Shouldn't you delete the CountedFaceCache as well?

https://codereview.chromium.org/2158023002/diff/180001/core/fxge/include/cfx_...
File core/fxge/include/cfx_fontcache.h (right):

https://codereview.chromium.org/2158023002/diff/180001/core/fxge/include/cfx_...
core/fxge/include/cfx_fontcache.h:30: struct CountedFaceCache {
Why not use CPDF_CountedObject<CFX_FaceCache> instead of a new struct?

[snake, 2016-09-15 10:39:26]

https://codereview.chromium.org/2158023002/diff/180001/core/fpdfapi/fpdf_page...
File core/fpdfapi/fpdf_page/fpdf_page_doc.cpp (right):

https://codereview.chromium.org/2158023002/diff/180001/core/fpdfapi/fpdf_page...
core/fpdfapi/fpdf_page/fpdf_page_doc.cpp:462: return
it_copied_stream->second->AddRef();
On 2016/09/14 23:07:59, npm wrote:
> If the code reaches to this point but the if statement is false,
> m_HashProfileMap[bsDigest] is replaced below even though it is not null. Is
that
> fine?
Yes.
CPDF_DocPageData is not owned pIccProfileStream. HashProfileMap is just
container to store hash from raw stream data. And if we have two different
pointers pIccProfileStream with identical data, we create just one profile for
both.

https://codereview.chromium.org/2158023002/diff/180001/core/fxge/ge/cfx_font.cpp
File core/fxge/ge/cfx_font.cpp (right):

https://codereview.chromium.org/2158023002/diff/180001/core/fxge/ge/cfx_font....
core/fxge/ge/cfx_font.cpp:554: m_FaceCache =
CFX_GEModule::Get()->GetFontCache()->GetCachedFace(this);
On 2016/09/14 23:07:59, npm wrote:
> Now you always get the facecache from CFX_GEModule.

Yes.

Before: Cached items in CFX_FontCache, was never deleted without call
::FreeCache (see cfx_fontcache.cpp ). As result, we had to use different
instance of cache for each document.

Now(with my changes): Cached items are released, when its are not used. I.e.
when we unload document, all related cached items will be released. And not
necessary to store FontCache in document.

https://codereview.chromium.org/2158023002/diff/180001/core/fxge/ge/cfx_fontc...
File core/fxge/ge/cfx_fontcache.cpp (right):

https://codereview.chromium.org/2158023002/diff/180001/core/fxge/ge/cfx_fontc...
core/fxge/ge/cfx_fontcache.cpp:61: } else {
On 2016/09/14 23:07:59, npm wrote:
> Shouldn't you delete the CountedFaceCache as well?

It is unique_ptr.

https://codereview.chromium.org/2158023002/diff/180001/core/fxge/include/cfx_...
File core/fxge/include/cfx_fontcache.h (right):

https://codereview.chromium.org/2158023002/diff/180001/core/fxge/include/cfx_...
core/fxge/include/cfx_fontcache.h:30: struct CountedFaceCache {
On 2016/09/14 23:07:59, npm wrote:
> Why not use CPDF_CountedObject<CFX_FaceCache> instead of a new struct?
The CPDF_CountedObject was used only in this class.
I did delete it, and wrote new internal implementation CountedFaceCache.
(see changes int fx_font.h)

[npm, 2016-09-15 14:27:53]

lgtm

Could you update the description to comment on the changes regarding
CFX_FontCache? Also, include the new memory usage for the pdf file with this CL.
Thank you!

[snake, 2016-09-15 20:57:15]

Description was changed from

==========
Fix fonts leaking on ClosePage.

BUG=79367,48791

The fonts was not cleared after unloading pages.

Test pdf:

http://www.nasa.gov/pdf/750614main_NASA_FY_2014_Budget_Estimates-508.pdf

For this file, we have ~5 fonts per page, which equal ~1 Mb per page.
In this PDF we have 670 pages, as result after slow scrolling(reading) full
document we have ~600 Mb fonts data in memory.

UPD: Also fix other leaks like this.
==========

to

==========
Fix memory leaking on ClosePage.
CFX_FontCache refactoring:
  after this CL: Only one global CFX_FontCache used. Any cached items from it,
are released, when its are not used.

BUG=79367,48791

The fonts was not cleared after unloading pages.

Test pdf:

http://www.nasa.gov/pdf/750614main_NASA_FY_2014_Budget_Estimates-508.pdf

For this file, we have ~5 fonts per page, which equal ~1 Mb per page.
In this PDF we have 670 pages, as result after slow scrolling(reading) full
document we have ~600 Mb fonts data in memory.

memory usage of PDF Plugin:
  before this CL: ~660 Mb
  after this CL: ~100 Mb
==========

[snake, 2016-09-15 20:58:26]

On 2016/09/15 14:27:53, npm wrote:
> lgtm
> 
> Could you update the description to comment on the changes regarding
> CFX_FontCache? Also, include the new memory usage for the pdf file with this
CL.
> Thank you!

Done.
Can I commit the patch?

[dsinclair, 2016-09-15 20:59:12]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2016-09-15 20:59:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
dsinclair@chromium.org
Link to the patchset: https://codereview.chromium.org/2158023002/#ps180001
(title: "Fix xfa tests.")

[commit-bot: I haz the power, 2016-09-15 20:59:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-15 21:11:12]

Description was changed from

==========
Fix memory leaking on ClosePage.
CFX_FontCache refactoring:
  after this CL: Only one global CFX_FontCache used. Any cached items from it,
are released, when its are not used.

BUG=79367,48791

The fonts was not cleared after unloading pages.

Test pdf:

http://www.nasa.gov/pdf/750614main_NASA_FY_2014_Budget_Estimates-508.pdf

For this file, we have ~5 fonts per page, which equal ~1 Mb per page.
In this PDF we have 670 pages, as result after slow scrolling(reading) full
document we have ~600 Mb fonts data in memory.

memory usage of PDF Plugin:
  before this CL: ~660 Mb
  after this CL: ~100 Mb
==========

to

==========
Fix memory leaking on ClosePage.
CFX_FontCache refactoring:
  after this CL: Only one global CFX_FontCache used. Any cached items from it,
are released, when its are not used.

BUG=79367,48791

The fonts was not cleared after unloading pages.

Test pdf:

http://www.nasa.gov/pdf/750614main_NASA_FY_2014_Budget_Estimates-508.pdf

For this file, we have ~5 fonts per page, which equal ~1 Mb per page.
In this PDF we have 670 pages, as result after slow scrolling(reading) full
document we have ~600 Mb fonts data in memory.

memory usage of PDF Plugin:
  before this CL: ~660 Mb
  after this CL: ~100 Mb

Committed:
https://pdfium.googlesource.com/pdfium/+/cde5101eb15b24519e89fa500fe37038bc8e...
==========

[commit-bot: I haz the power, 2016-09-15 21:11:14]

Committed patchset #10 (id:180001) as
https://pdfium.googlesource.com/pdfium/+/cde5101eb15b24519e89fa500fe37038bc8e...

[dsinclair, 2016-09-19 15:17:44]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/2350763002/ by dsinclair@chromium.org.

The reason for reverting is: Causes heap-use-after-free. See crbug.com/647612..