Change base::Value::ListStorage to std::vector<base::Value>

chrome/browser/extensions/menu_manager.cc

Index: chrome/browser/extensions/menu_manager.cc
diff --git a/chrome/browser/extensions/menu_manager.cc b/chrome/browser/extensions/menu_manager.cc
index 11acf54ca5d1458c8ee8c4ea7ee8248ce2157998..798fe5e3a75bd92845f201cc35e63c01f87023d8 100644
--- a/chrome/browser/extensions/menu_manager.cc
+++ b/chrome/browser/extensions/menu_manager.cc
@@ -665,8 +665,12 @@ void MenuManager::ExecuteCommand(content::BrowserContext* context,
                            webview_guest->view_instance_id());
   }
 
-  base::DictionaryValue* raw_properties = properties.get();
+  size_t properties_idx = args->GetSize();
   args->Append(std::move(properties));
+  // |properties| is invalidated at this time, which is why |args| needs to be
+  // queried for the pointer.
+  base::DictionaryValue* raw_properties;

[Devlin, 2017/03/15 15:25:04]

nit: initialize to nullptr

[jdoerrie, 2017/03/23 18:11:17]

Done.

+  args->GetDictionary(properties_idx, &raw_properties);
 
   // Add the tab info to the argument list.
   // No tab info in a platform app.
@@ -683,6 +687,10 @@ void MenuManager::ExecuteCommand(content::BrowserContext* context,
     }
   }
 
+  // |args->Append| might have invalidated pointers, which is why |args| needs

[Devlin, 2017/03/15 15:25:03]

Oof, that's subtle.  Do we have any way of checking when this could happen?  My
guess is that keeping a raw ptr to a value and then modifying the value is not
that uncommon.

Relatedly, what are we going to do going forward?  It seems like if the
underlying pointers can change as a result of modification, then an API that
returns those pointers is pretty dangerous and easy to misuse.  Is there a plan
to change it?

[brettw, 2017/03/15 22:14:14]

Yes, there should be no pointers in the API when we're done:
https://docs.google.com/document/d/1uDLu5uTRlCWePxQUEHc8yNQdEoE1BDISYdpggWEAB...

[jdoerrie, 2017/03/23 18:11:17]

To answer your first question, yes, we can determine when this happens. It will
reallocate iff args->size() == args.capacity() before calling Append. 

I introduced a call to |Reserve| now, that will make sure that the pointer will
not get invalidated again. Let me know if you prefer this over querying twice in
the locations where |raw_properties| are needed.

+  // to be queried again for the pointer.
+  args->GetDictionary(properties_idx, &raw_properties);
+
   if (item->type() == MenuItem::CHECKBOX ||
       item->type() == MenuItem::RADIO) {
     bool was_checked = item->checked();