Various changes to JBig2 cache:

core/src/fxcodec/jbig2/JBig2_Context.cpp

Index: core/src/fxcodec/jbig2/JBig2_Context.cpp
diff --git a/core/src/fxcodec/jbig2/JBig2_Context.cpp b/core/src/fxcodec/jbig2/JBig2_Context.cpp
index ca895e5b9c0378fd4985c2d8493c658bb248b228..1a26ad7bc22ff4be91e13cbc5efe93c4ad5fe10c 100644
--- a/core/src/fxcodec/jbig2/JBig2_Context.cpp
+++ b/core/src/fxcodec/jbig2/JBig2_Context.cpp
@@ -25,32 +25,28 @@
 //
 // Disabled until we can figure out how to clear cache between documents.

[Lei Zhang, 2015/10/06 22:40:24]

This should be deleted too.

[David Lattimore, 2015/10/07 22:41:53]

Done.

 // https://code.google.com/p/pdfium/issues/detail?id=207
-#define DISABLE_SYMBOL_CACHE
 #ifndef DISABLE_SYMBOL_CACHE

[Lei Zhang, 2015/10/06 22:40:24]

Do we still want to keep this then? With a proper fix, I see no real need to
offer the option to disable the cache.

[jbreiden, 2015/10/07 17:50:17]

I'd like to think that we can disable the cache any time we want just by setting
kSymbolDictCacheMaxSize to 0. No #ifdef required. I suppose it makes sense to
try that before checking in to make sure nothing crashes.

Also, I think this should be 2 because:
 
  (a) I still can't wrap my head around why we can get away with 1.
  (b) 2 exercises the LRU machinery hopefully keeping it from bitrot.
  (c) I don't see much downside

[Lei Zhang, 2015/10/07 17:59:24]

No, you can't. Setting it to 0 triggers compiler warnings with Clang, and we
want to get to a world where (core) PDFium is warning free.

[David Lattimore, 2015/10/07 22:41:53]

Removed DISABLE_SYMBOL_CACHE

Set cache size back to 2. I agree there's not much downside. Especially now the
cache is owned by the document so will be freed at the end of the document.

-static const int kSymbolDictCacheMaxSize = 2;
+static const int kSymbolDictCacheMaxSize = 1;
 #endif
 
 CJBig2_Context* CJBig2_Context::CreateContext(
-    const uint8_t* pGlobalData,
-    FX_DWORD dwGlobalLength,
-    const uint8_t* pData,
-    FX_DWORD dwLength,
+    CPDF_StreamAcc* pGlobalStream,
+    CPDF_StreamAcc* pSrcStream,
     std::list<CJBig2_CachePair>* pSymbolDictCache,
     IFX_Pause* pPause) {
-  return new CJBig2_Context(pGlobalData, dwGlobalLength, pData, dwLength,
-                            pSymbolDictCache, pPause);
+  return new CJBig2_Context(pGlobalStream, pSrcStream, pSymbolDictCache, pPause,
+                            false);
 }
 
 void CJBig2_Context::DestroyContext(CJBig2_Context* pContext) {
   delete pContext;
 }
 
-CJBig2_Context::CJBig2_Context(const uint8_t* pGlobalData,
-                               FX_DWORD dwGlobalLength,
-                               const uint8_t* pData,
-                               FX_DWORD dwLength,
+CJBig2_Context::CJBig2_Context(CPDF_StreamAcc* pGlobalStream,
+                               CPDF_StreamAcc* pSrcStream,
                                std::list<CJBig2_CachePair>* pSymbolDictCache,
-                               IFX_Pause* pPause)
+                               IFX_Pause* pPause,
+                               bool isGlobal)
     : m_nSegmentDecoded(0),
       m_bInPage(false),
       m_bBufSpecified(false),
@@ -60,15 +56,16 @@ CJBig2_Context::CJBig2_Context(const uint8_t* pGlobalData,
       m_pArithDecoder(NULL),
       m_gbContext(NULL),
       m_dwOffset(0),
-      m_pSymbolDictCache(pSymbolDictCache) {
-  if (pGlobalData && (dwGlobalLength > 0)) {
-    m_pGlobalContext = new CJBig2_Context(
-        nullptr, 0, pGlobalData, dwGlobalLength, pSymbolDictCache, pPause);
+      m_pSymbolDictCache(pSymbolDictCache),
+      m_IsGlobal(isGlobal) {
+  if (pGlobalStream && (pGlobalStream->GetSize() > 0)) {
+    m_pGlobalContext =
+        new CJBig2_Context(NULL, pGlobalStream, pSymbolDictCache, pPause, true);
   } else {
     m_pGlobalContext = nullptr;
   }
 
-  m_pStream.reset(new CJBig2_BitStream(pData, dwLength));
+  m_pStream.reset(new CJBig2_BitStream(pSrcStream));
 }
 
 CJBig2_Context::~CJBig2_Context() {
@@ -325,7 +322,8 @@ int32_t CJBig2_Context::parseSegmentHeader(CJBig2_Segment* pSegment) {
   if (m_pStream->readInteger(&pSegment->m_dwData_length) != 0)
     return JBIG2_ERROR_TOO_SHORT;
 
-  pSegment->m_pData = m_pStream->getPointer();
+  pSegment->m_dwObjNum = m_pStream->getObjNum();
+  pSegment->m_dwDataOffset = m_pStream->getOffset();
   pSegment->m_State = JBIG2_SEGMENT_DATA_UNPARSED;
   return JBIG2_SUCCESS;
 }
@@ -445,7 +443,8 @@ int32_t CJBig2_Context::parseSymbolDict(CJBig2_Segment* pSegment,
   JBig2ArithCtx* grContext = nullptr;
   CJBig2_ArithDecoder* pArithDecoder;
   CJBig2_SDDProc* pSymbolDictDecoder = new CJBig2_SDDProc();
-  const uint8_t* key = pSegment->m_pData;
+  CJBig2_CacheKey key =
+      CJBig2_CacheKey(pSegment->m_dwObjNum, pSegment->m_dwDataOffset);
   FX_BOOL cache_hit = false;
   if (m_pStream->readShortInteger(&wFlags) != 0) {
     nRet = JBIG2_ERROR_TOO_SHORT;
@@ -623,15 +622,17 @@ int32_t CJBig2_Context::parseSymbolDict(CJBig2_Segment* pSegment,
     }
   }
   pSegment->m_nResultType = JBIG2_SYMBOL_DICT_POINTER;
-  for (std::list<CJBig2_CachePair>::iterator it = m_pSymbolDictCache->begin();
-       it != m_pSymbolDictCache->end(); ++it) {
-    if (it->first == key) {
-      nonstd::unique_ptr<CJBig2_SymbolDict> copy(it->second->DeepCopy());
-      pSegment->m_Result.sd = copy.release();
-      m_pSymbolDictCache->push_front(*it);
-      m_pSymbolDictCache->erase(it);
-      cache_hit = true;
-      break;
+  if (m_IsGlobal) {
+    for (std::list<CJBig2_CachePair>::iterator it = m_pSymbolDictCache->begin();

[Lei Zhang, 2015/10/06 22:40:24]

You can probaby use auto here.

[David Lattimore, 2015/10/07 22:41:53]

Done.

+         it != m_pSymbolDictCache->end(); ++it) {
+      if (it->first == key) {
+        nonstd::unique_ptr<CJBig2_SymbolDict> copy(it->second->DeepCopy());
+        pSegment->m_Result.sd = copy.release();
+        m_pSymbolDictCache->push_front(*it);
+        m_pSymbolDictCache->erase(it);
+        cache_hit = true;
+        break;
+      }
     }
   }
   if (!cache_hit) {
@@ -656,14 +657,16 @@ int32_t CJBig2_Context::parseSymbolDict(CJBig2_Segment* pSegment,
       m_pStream->alignByte();
     }
 #ifndef DISABLE_SYMBOL_CACHE

[jbreiden, 2015/10/07 17:50:17]

not sure why we have the ifdef

[Lei Zhang, 2015/10/07 17:59:24]

Because the compiler sees a constant with a value 0 and decides some of the if /
while statements below are worthless.

[jbreiden, 2015/10/07 19:22:51]

Something seems wrong when a compiler pushes us to use an ifdef in the name of
code cleanliness. 

If kSymbolDictCacheMaxSize can never be 0, there's not a lot of benefit from the
(kSymbolDictCacheMaxSize > 0) check below. But honestly, this probably is not
worth sweating over one way or the other.

[Lei Zhang, 2015/10/07 19:31:59]

I agree Clang behaved unexpectedly in this case. It did not warn about the if
statement, which always evals to false, but did warn about the while statement
that always evals to true.

But taking a step back, this is silly to argue about. Let's just fix the bug and
get rid of DISABLE_SYMBOL_CACHE and call it a day.

[David Lattimore, 2015/10/07 22:41:53]

Done.

-    nonstd::unique_ptr<CJBig2_SymbolDict> value =
-        pSegment->m_Result.sd->DeepCopy();
-    if (value && kSymbolDictCacheMaxSize > 0) {
-      while (m_pSymbolDictCache->size() >= kSymbolDictCacheMaxSize) {
-        delete m_pSymbolDictCache->back().second;
-        m_pSymbolDictCache->pop_back();
+    if (m_IsGlobal && kSymbolDictCacheMaxSize > 0) {
+      nonstd::unique_ptr<CJBig2_SymbolDict> value =
+          pSegment->m_Result.sd->DeepCopy();
+      if (value) {
+        while (m_pSymbolDictCache->size() >= kSymbolDictCacheMaxSize) {
+          delete m_pSymbolDictCache->back().second;
+          m_pSymbolDictCache->pop_back();
+        }
+        m_pSymbolDictCache->push_front(CJBig2_CachePair(key, value.release()));
       }
-      m_pSymbolDictCache->push_front(CJBig2_CachePair(key, value.release()));
     }
 #endif
   }