Various changes to JBig2 cache:

core/src/fxcodec/jbig2/JBig2_Context.cpp

 // Copyright 2014 PDFium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Original code copyright 2014 Foxit Software Inc. http://www.foxitsoftware.com
 
 #include "JBig2_Context.h"
 
 #include <list>
 
 #include "JBig2_GrdProc.h"
 #include "JBig2_GrrdProc.h"
 #include "JBig2_HtrdProc.h"
 #include "JBig2_PddProc.h"
 #include "JBig2_SddProc.h"
 #include "JBig2_TrdProc.h"
 
 // Implement a very small least recently used (LRU) cache. It is very
 // common for a JBIG2 dictionary to span multiple pages in a PDF file,
 // and we do not want to decode the same dictionary over and over
 // again. We key off of the memory location of the dictionary. The

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

not keying off memory location anymore.

 // list keeps track of the freshness of entries, with freshest ones
 // at the front. Even a tiny cache size like 2 makes a dramatic
 // difference for typical JBIG2 documents.
 //
 // 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),
       m_PauseStep(10),
       m_pPause(pPause),
       m_ProcessingStatus(FXCODEC_STATUS_FRAME_READY),
       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() {
   delete m_pArithDecoder;
   m_pArithDecoder = NULL;
   FX_Free(m_gbContext);
   m_gbContext = NULL;
   delete m_pGlobalContext;
   m_pGlobalContext = NULL;
 }
@@ skipping 236 matching lines @@
       return JBIG2_ERROR_TOO_SHORT;
     pSegment->m_dwPage_association = cTemp;
   } else {
     if (m_pStream->readInteger(&pSegment->m_dwPage_association) != 0) {
       return JBIG2_ERROR_TOO_SHORT;
     }
   }
   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;
 }
 
 int32_t CJBig2_Context::parseSegmentData(CJBig2_Segment* pSegment,
                                          IFX_Pause* pPause) {
   int32_t ret = ProcessingParseSegmentData(pSegment, pPause);
   while (m_ProcessingStatus == FXCODEC_STATUS_DECODE_TOBECONTINUE &&
          m_pStream->getByteLeft() > 0) {
     ret = ProcessingParseSegmentData(pSegment, pPause);
@@ skipping 99 matching lines @@
   CJBig2_HuffmanTable* Table_B5 = nullptr;
   int32_t i, nIndex, nRet;
   CJBig2_Segment* pSeg = nullptr;
   CJBig2_Segment* pLRSeg = nullptr;
   FX_BOOL bUsed;
   CJBig2_Image** SDINSYMS = nullptr;
   JBig2ArithCtx* gbContext = nullptr;
   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;
     goto failed;
   }
   pSymbolDictDecoder->SDHUFF = wFlags & 0x0001;
   pSymbolDictDecoder->SDREFAGG = (wFlags >> 1) & 0x0001;
   pSymbolDictDecoder->SDTEMPLATE = (wFlags >> 10) & 0x0003;
   pSymbolDictDecoder->SDRTEMPLATE = (wFlags >> 12) & 0x0003;
   cSDHUFFDH = (wFlags >> 2) & 0x0003;
@@ skipping 157 matching lines @@
       gbContext = FX_Alloc(JBig2ArithCtx, dwTemp);
       JBIG2_memset(gbContext, 0, sizeof(JBig2ArithCtx) * dwTemp);
     }
     if (pSymbolDictDecoder->SDREFAGG == 1) {
       dwTemp = pSymbolDictDecoder->SDRTEMPLATE ? 1 << 10 : 1 << 13;
       grContext = FX_Alloc(JBig2ArithCtx, dwTemp);
       JBIG2_memset(grContext, 0, sizeof(JBig2ArithCtx) * dwTemp);
     }
   }
   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) {
     if (pSymbolDictDecoder->SDHUFF == 0) {
       pArithDecoder = new CJBig2_ArithDecoder(m_pStream.get());
       pSegment->m_Result.sd =
           pSymbolDictDecoder->decode_Arith(pArithDecoder, gbContext, grContext);
       delete pArithDecoder;
       if (pSegment->m_Result.sd == NULL) {
         nRet = JBIG2_ERROR_FATAL;
         goto failed;
       }
       m_pStream->alignByte();
       m_pStream->offset(2);
     } else {
       pSegment->m_Result.sd = pSymbolDictDecoder->decode_Huffman(
           m_pStream.get(), gbContext, grContext, pPause);
       if (pSegment->m_Result.sd == NULL) {
         nRet = JBIG2_ERROR_FATAL;
         goto failed;
       }
       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
   }
   if (wFlags & 0x0200) {
     pSegment->m_Result.sd->m_bContextRetained = TRUE;
     if (pSymbolDictDecoder->SDHUFF == 0) {
       pSegment->m_Result.sd->m_gbContext = gbContext;
     }
     if (pSymbolDictDecoder->SDREFAGG == 1) {
       pSegment->m_Result.sd->m_grContext = grContext;
@@ skipping 902 matching lines @@
         SBSYMCODES[CURTEMP].code = CURCODE;
         CURCODE = CURCODE + 1;
       }
       CURTEMP = CURTEMP + 1;
     }
     CURLEN = CURLEN + 1;
   }
   FX_Free(LENCOUNT);
   FX_Free(FIRSTCODE);
 }