Sanitize CJBig2_SymbolDict's memory usage.

Sanitize CJBig2_SymbolDict's memory usage.

- Use std::vector<JBig2ArithCtx> instead of storing pointers to arrays.
- Make CJBig2_SymbolDict's members private with accessors.
- Use std::vector<JBig2ArithCtx> in related places.
- Steal Chromium's vector_as_array() and use it as an adaptor as needed.

BUG=514891
R=tsepez@chromium.org

Committed: https://pdfium.googlesource.com/pdfium/+/3acb1ef909a22368507ed13817c4988c818e3aee

[Lei Zhang, 2015-10-09 08:55:53]

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

[Lei Zhang, 2015-10-09 08:55:54]

The behavior does change a bit since CJBig2_SymbolDict's DeepCopy() can no
longer fail. I have not seen any rendering failures from the PDFs I tested with
though.

[Lei Zhang, 2015-10-09 09:20:08]

I can't trigger the condition with Karl's test PDF, but more checks in patch set
2 -> better safe than sorry.

[Tom Sepez, 2015-10-09 16:10:52]

https://codereview.chromium.org/1388203003/diff/20001/core/src/fxcodec/jbig2/...
File core/src/fxcodec/jbig2/JBig2_Context.cpp (left):

https://codereview.chromium.org/1388203003/diff/20001/core/src/fxcodec/jbig2/...
core/src/fxcodec/jbig2/JBig2_Context.cpp:573: if ((wFlags & 0x0100) && pLRSeg &&
pLRSeg->m_Result.sd->m_bContextRetained) {
m_bContextRetained removed, but I only every saw it being set to FALSE.  Does
this block ever get to execute then? Or did I miss something?

https://codereview.chromium.org/1388203003/diff/20001/core/src/fxcodec/jbig2/...
File core/src/fxcodec/jbig2/JBig2_SymbolDict.h (right):

https://codereview.chromium.org/1388203003/diff/20001/core/src/fxcodec/jbig2/...
core/src/fxcodec/jbig2/JBig2_SymbolDict.h:36: m_gbContext = gbContext;
maybe std::vector<>&&  and use nonstd::move when calling this to avoid copying
the elts?

[Lei Zhang, 2015-10-09 18:50:28]

https://codereview.chromium.org/1388203003/diff/20001/core/src/fxcodec/jbig2/...
File core/src/fxcodec/jbig2/JBig2_Context.cpp (left):

https://codereview.chromium.org/1388203003/diff/20001/core/src/fxcodec/jbig2/...
core/src/fxcodec/jbig2/JBig2_Context.cpp:573: if ((wFlags & 0x0100) && pLRSeg &&
pLRSeg->m_Result.sd->m_bContextRetained) {
On 2015/10/09 16:10:52, Tom Sepez wrote:
> m_bContextRetained removed, but I only every saw it being set to FALSE.  Does
> this block ever get to execute then? Or did I miss something?

m_bContextRetained is set to TRUE below on line 646.

[Lei Zhang, 2015-10-09 18:59:52]

https://codereview.chromium.org/1388203003/diff/20001/core/src/fxcodec/jbig2/...
File core/src/fxcodec/jbig2/JBig2_SymbolDict.h (right):

https://codereview.chromium.org/1388203003/diff/20001/core/src/fxcodec/jbig2/...
core/src/fxcodec/jbig2/JBig2_SymbolDict.h:36: m_gbContext = gbContext;
On 2015/10/09 16:10:52, Tom Sepez wrote:
> maybe std::vector<>&&  and use nonstd::move when calling this to avoid copying
> the elts?

+thakis - I thought we don't do that yet?

[Tom Sepez, 2015-10-09 19:01:13]

On 2015/10/09 18:59:52, Lei Zhang wrote:
>
https://codereview.chromium.org/1388203003/diff/20001/core/src/fxcodec/jbig2/...
> File core/src/fxcodec/jbig2/JBig2_SymbolDict.h (right):
> 
>
https://codereview.chromium.org/1388203003/diff/20001/core/src/fxcodec/jbig2/...
> core/src/fxcodec/jbig2/JBig2_SymbolDict.h:36: m_gbContext = gbContext;
> On 2015/10/09 16:10:52, Tom Sepez wrote:
> > maybe std::vector<>&&  and use nonstd::move when calling this to avoid
copying
> > the elts?
> 
> +thakis - I thought we don't do that yet?
Chrome doesn't do this, because c++11 not everywhere, so no std::move.  But we
have nonstd::move(), so we're ahead of them (!!!).

[Tom Sepez, 2015-10-09 19:02:40]

Ah, LGTM.

[Nico, 2015-10-09 20:23:24]

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

[Nico, 2015-10-09 20:23:24]

https://codereview.chromium.org/1388203003/diff/20001/core/src/fxcodec/jbig2/...
File core/src/fxcodec/jbig2/JBig2_SymbolDict.h (right):

https://codereview.chromium.org/1388203003/diff/20001/core/src/fxcodec/jbig2/...
core/src/fxcodec/jbig2/JBig2_SymbolDict.h:36: m_gbContext = gbContext;
On 2015/10/09 18:59:52, Lei Zhang wrote:
> On 2015/10/09 16:10:52, Tom Sepez wrote:
> > maybe std::vector<>&&  and use nonstd::move when calling this to avoid
copying
> > the elts?
> 
> +thakis - I thought we don't do that yet?

chromium-cpp.appspot.com hopefully applies to pdfium too. rvalue refs aren't
generally allowed
(https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/xMscQuYBwyc...
"(for example, if you want to   allow rvalue references, you should be able to
make a convincing case that most chromium developers can explain the difference
between std::move and std::forward)"… even with pdfium's much smaller crew, I'm
not sure if that applies with s/chromium/pdfium/)

[Lei Zhang, 2015-10-09 20:50:27]

Ok, then landing as is.

[Lei Zhang, 2015-10-09 20:51:08]

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