CFX_MapByteStringToPtr considered harmful.

fpdfsdk/src/javascript/global.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 "../../include/javascript/IJavaScript.h"
 #include "../../include/javascript/JS_Context.h"
 #include "../../include/javascript/JS_Define.h"
 #include "../../include/javascript/JS_EventHandler.h"
@@ skipping 106 matching lines @@
   UpdateGlobalPersistentVariables();
 }
 
 FX_BOOL global_alternate::QueryProperty(const FX_WCHAR* propname) {
   return CFX_WideString(propname) != L"setPersistent";
 }
 
 FX_BOOL global_alternate::DelProperty(IFXJS_Context* cc,
                                       const FX_WCHAR* propname,
                                       CFX_WideString& sError) {
-  js_global_data* pData = NULL;
-  CFX_ByteString sPropName = CFX_ByteString::FromUnicode(propname);
+  auto it = m_mapGlobal.find(CFX_ByteString::FromUnicode(propname));
+  if (it == m_mapGlobal.end())
+    return FALSE;
 
-  if (m_mapGlobal.Lookup(sPropName, (void*&)pData)) {
-    pData->bDeleted = TRUE;
-    return TRUE;
-  }
-
-  return FALSE;
+  it->second->bDeleted = TRUE;
+  return TRUE;
 }
 
 FX_BOOL global_alternate::DoProperty(IFXJS_Context* cc,
                                      const FX_WCHAR* propname,
                                      CJS_PropValue& vp,
                                      CFX_WideString& sError) {
   if (vp.IsSetting()) {
     CFX_ByteString sPropName = CFX_ByteString::FromUnicode(propname);
     switch (vp.GetType()) {
       case VT_number: {
@@ skipping 25 matching lines @@
                                   "", v8::Local<v8::Object>(), FALSE);
       }
       case VT_undefined: {
         DelProperty(cc, propname, sError);
         return TRUE;
       }
       default:
         break;
     }
   } else {
-    void* pVoid = nullptr;
-    if (!m_mapGlobal.Lookup(CFX_ByteString::FromUnicode(propname), pVoid)) {
+    auto it = m_mapGlobal.find(CFX_ByteString::FromUnicode(propname));
+    if (it == m_mapGlobal.end()) {
       vp.SetNull();
       return TRUE;
     }
-    if (!pVoid) {
+    js_global_data* pData = it->second;
+    if (!pData || pData->bDeleted) {

[Lei Zhang, 2015/08/14 23:59:35]

This is intentially calling vp.SetNull() for the bDeleted = true case, right?

[Lei Zhang, 2015/08/14 23:59:35]

|pData| is never NULL.

[Tom Sepez, 2015/08/17 20:15:26]

Yes, seems like a bug to return with this unset.

[Tom Sepez, 2015/08/17 20:15:26]

Done.

       vp.SetNull();
       return TRUE;
     }
-    js_global_data* pData = (js_global_data*)pVoid;
-    if (pData->bDeleted)
-      return TRUE;
-
     switch (pData->nType) {
       case JS_GLOBALDATA_TYPE_NUMBER:
         vp << pData->dData;
         return TRUE;
       case JS_GLOBALDATA_TYPE_BOOLEAN:
         vp << pData->bData;
         return TRUE;
       case JS_GLOBALDATA_TYPE_STRING:
         vp << pData->sData;
         return TRUE;
@@ skipping 16 matching lines @@
 FX_BOOL global_alternate::setPersistent(IFXJS_Context* cc,
                                         const CJS_Parameters& params,
                                         CJS_Value& vRet,
                                         CFX_WideString& sError) {
   CJS_Context* pContext = static_cast<CJS_Context*>(cc);
   if (params.size() != 2) {
     sError = JSGetStringFromID(pContext, IDS_STRING_JSPARAMERROR);
     return FALSE;
   }
 
-  CFX_ByteString sName = params[0].ToCFXByteString();
-
-  js_global_data* pData = NULL;
-  if (m_mapGlobal.Lookup(sName, (void*&)pData)) {
+  auto it = m_mapGlobal.find(params[0].ToCFXByteString());
+  if (it != m_mapGlobal.end()) {
+    js_global_data* pData = it->second;
     if (pData && !pData->bDeleted) {

[Lei Zhang, 2015/08/14 23:59:35]

|pData| is never NULL.

[Tom Sepez, 2015/08/17 20:15:26]

Done.

       pData->bPersistent = params[1].ToBool();
       return TRUE;
     }
   }
 
   sError = JSGetStringFromID(pContext, IDS_STRING_JSNOGLOBAL);
   return FALSE;
 }
 
 void global_alternate::UpdateGlobalPersistentVariables() {
@@ skipping 45 matching lines @@
                            "", v8::Local<v8::Object>(),
                            pData->bPersistent == 1);
         JS_PutObjectNull(NULL, (JSFXObject)(*m_pJSObject),
                          pData->data.sKey.UTF8Decode().c_str());
         break;
     }
   }
 }
 
 void global_alternate::CommitGlobalPersisitentVariables() {
-  ASSERT(m_pGlobalData != NULL);
+  ASSERT(m_pGlobalData);
 
-  FX_POSITION pos = m_mapGlobal.GetStartPosition();
-  while (pos) {
-    CFX_ByteString name;
-    js_global_data* pData = NULL;
-    m_mapGlobal.GetNextAssoc(pos, name, (void*&)pData);
-
+  for (auto it = m_mapGlobal.begin(); it != m_mapGlobal.end(); ++it) {
+    CFX_ByteString name = it->first;
+    js_global_data* pData = it->second;
     if (pData) {

[Lei Zhang, 2015/08/14 23:59:35]

|pData| is never NULL.

[Tom Sepez, 2015/08/17 20:15:26]

Done.

       if (pData->bDeleted) {
         m_pGlobalData->DeleteGlobalVariable(name);
       } else {
         switch (pData->nType) {
           case JS_GLOBALDATA_TYPE_NUMBER:
             m_pGlobalData->SetGlobalVariableNumber(name, pData->dData);
             m_pGlobalData->SetGlobalVariablePersistent(name,
                                                        pData->bPersistent);
             break;
           case JS_GLOBALDATA_TYPE_BOOLEAN:
@@ skipping 120 matching lines @@
       } break;
       case JS_GLOBALDATA_TYPE_NULL:
         JS_PutObjectNull(NULL, (JSObject)pObj,
                          pObjData->sKey.UTF8Decode().c_str());
         break;
     }
   }
 }
 
 void global_alternate::DestroyGlobalPersisitentVariables() {
-  FX_POSITION pos = m_mapGlobal.GetStartPosition();
-  while (pos) {
-    CFX_ByteString name;
-    js_global_data* pData = NULL;
-    m_mapGlobal.GetNextAssoc(pos, name, (void*&)pData);
-    delete pData;
+  auto it = m_mapGlobal.begin();

[Lei Zhang, 2015/08/14 23:59:35]

Can't this be written as follows?

for (auto it : m_mapGlobal)
  delete it->second;
m_mapGlobal.clear()

[Tom Sepez, 2015/08/17 20:15:26]

Done.

+  while (it != m_mapGlobal.end()) {
+    auto temp = it++;
+    delete temp->second;
+    m_mapGlobal.erase(temp);
   }
-
-  m_mapGlobal.RemoveAll();
 }
 
 FX_BOOL global_alternate::SetGlobalVariables(const FX_CHAR* propname,
                                              int nType,
                                              double dData,
                                              bool bData,
                                              const CFX_ByteString& sData,
                                              JSObject pData,
                                              bool bDefaultPersistent) {
-  if (propname == NULL)
+  if (!propname)
     return FALSE;
 
-  js_global_data* pTemp = NULL;
-  m_mapGlobal.Lookup(propname, (void*&)pTemp);
-
-  if (pTemp) {
+  auto it = m_mapGlobal.find(propname);
+  if (it != m_mapGlobal.end() && it->second) {

[Lei Zhang, 2015/08/14 23:59:35]

|it->second| is never NULL.

[Tom Sepez, 2015/08/17 20:15:26]

Done.

+    js_global_data* pTemp = it->second;
     if (pTemp->bDeleted || pTemp->nType != nType) {
       pTemp->dData = 0;
       pTemp->bData = 0;
       pTemp->sData = "";
       pTemp->nType = nType;
     }
 
     pTemp->bDeleted = FALSE;
-
     switch (nType) {
       case JS_GLOBALDATA_TYPE_NUMBER: {
         pTemp->dData = dData;
       } break;
       case JS_GLOBALDATA_TYPE_BOOLEAN: {
         pTemp->bData = bData;
       } break;
       case JS_GLOBALDATA_TYPE_STRING: {
         pTemp->sData = sData;
       } break;
       case JS_GLOBALDATA_TYPE_OBJECT: {
         pTemp->pData.Reset(JS_GetRuntime(pData), pData);
       } break;
       case JS_GLOBALDATA_TYPE_NULL:
         break;
       default:
         return FALSE;
     }
-
     return TRUE;
   }
 
   js_global_data* pNewData = NULL;
 
   switch (nType) {
     case JS_GLOBALDATA_TYPE_NUMBER: {
       pNewData = new js_global_data;
       pNewData->nType = JS_GLOBALDATA_TYPE_NUMBER;
       pNewData->dData = dData;
@@ skipping 19 matching lines @@
     } break;
     case JS_GLOBALDATA_TYPE_NULL: {
       pNewData = new js_global_data;
       pNewData->nType = JS_GLOBALDATA_TYPE_NULL;
       pNewData->bPersistent = bDefaultPersistent;
     } break;
     default:
       return FALSE;
   }
 
-  m_mapGlobal.SetAt(propname, (void*)pNewData);
-
+  m_mapGlobal[propname] = pNewData;

[Lei Zhang, 2015/08/14 23:59:35]

Since this is the only setter, and |pNewData| cannot be null, we can get rid of
all the checks above.

[Tom Sepez, 2015/08/17 20:15:26]

Presume covered by the |pdata| can't be null comments above.

   return TRUE;
 }
 
 FXJSVALUETYPE GET_VALUE_TYPE(v8::Local<v8::Value> p) {
   const unsigned int nHash = JS_CalcHash(JS_GetTypeof(p));
 
   if (nHash == JSCONST_nUndefHash)
     return VT_undefined;
   if (nHash == JSCONST_nNullHash)
     return VT_null;
   if (nHash == JSCONST_nStringHash)
     return VT_string;
   if (nHash == JSCONST_nNumberHash)
     return VT_number;
   if (nHash == JSCONST_nBoolHash)
     return VT_boolean;
   if (nHash == JSCONST_nDateHash)
     return VT_date;
   if (nHash == JSCONST_nObjectHash)
     return VT_object;
   if (nHash == JSCONST_nFXobjHash)
     return VT_fxobject;
 
   return VT_unknown;
 }