CFX_MapByteStringToPtr considered harmful.

fpdfsdk/src/javascript/global.cpp

Index: fpdfsdk/src/javascript/global.cpp
diff --git a/fpdfsdk/src/javascript/global.cpp b/fpdfsdk/src/javascript/global.cpp
index ee0591b9500ad5e84e32609d27ae288ecc9baaac..30fb3544a0befb2d2e10b072a7cdb41a1f50c9fe 100644
--- a/fpdfsdk/src/javascript/global.cpp
+++ b/fpdfsdk/src/javascript/global.cpp
@@ -124,15 +124,12 @@ FX_BOOL global_alternate::QueryProperty(const FX_WCHAR* propname) {
 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);
-
-  if (m_mapGlobal.Lookup(sPropName, (void*&)pData)) {
-    pData->bDeleted = TRUE;
-    return TRUE;
-  }
+  auto it = m_mapGlobal.find(CFX_ByteString::FromUnicode(propname));
+  if (it == m_mapGlobal.end())
+    return FALSE;
 
-  return FALSE;
+  it->second->bDeleted = TRUE;
+  return TRUE;
 }
 
 FX_BOOL global_alternate::DoProperty(IFXJS_Context* cc,
@@ -178,19 +175,16 @@ FX_BOOL global_alternate::DoProperty(IFXJS_Context* cc,
         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;
@@ -227,10 +221,9 @@ FX_BOOL global_alternate::setPersistent(IFXJS_Context* cc,
     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;
@@ -297,14 +290,11 @@ void global_alternate::UpdateGlobalPersistentVariables() {
 }
 
 void global_alternate::CommitGlobalPersisitentVariables() {
-  ASSERT(m_pGlobalData != NULL);
-
-  FX_POSITION pos = m_mapGlobal.GetStartPosition();
-  while (pos) {
-    CFX_ByteString name;
-    js_global_data* pData = NULL;
-    m_mapGlobal.GetNextAssoc(pos, name, (void*&)pData);
+  ASSERT(m_pGlobalData);
 
+  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);
@@ -446,15 +436,12 @@ void global_alternate::PutObjectProperty(v8::Local<v8::Object> pObj,
 }
 
 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,
@@ -464,13 +451,12 @@ FX_BOOL global_alternate::SetGlobalVariables(const FX_CHAR* propname,
                                              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;
@@ -479,7 +465,6 @@ FX_BOOL global_alternate::SetGlobalVariables(const FX_CHAR* propname,
     }
 
     pTemp->bDeleted = FALSE;
-
     switch (nType) {
       case JS_GLOBALDATA_TYPE_NUMBER: {
         pTemp->dData = dData;
@@ -498,7 +483,6 @@ FX_BOOL global_alternate::SetGlobalVariables(const FX_CHAR* propname,
       default:
         return FALSE;
     }
-
     return TRUE;
   }
 
@@ -538,8 +522,7 @@ FX_BOOL global_alternate::SetGlobalVariables(const FX_CHAR* propname,
       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;
 }