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

chrome/browser/extensions/menu_manager.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/menu_manager.h"
 
 #include <algorithm>
 #include <memory>
 #include <tuple>
 #include <utility>
@@ skipping 647 matching lines @@
   properties->SetBoolean("editable", params.is_editable);
 
   WebViewGuest* webview_guest = WebViewGuest::FromWebContents(web_contents);
   if (webview_guest) {
     // This is used in web_view_internalcustom_bindings.js.
     // The property is not exposed to developer API.
     properties->SetInteger("webviewInstanceId",
                            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.
   if (!extension || !extension->is_platform_app()) {
     // Note: web_contents are null in unit tests :(
     if (web_contents) {
       int frame_id = ExtensionApiFrameIdMap::GetFrameId(render_frame_host);
       if (frame_id != ExtensionApiFrameIdMap::kInvalidFrameId)
         raw_properties->SetInteger("frameId", frame_id);

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

Given raw_properties is only safe to use before the appends immediately below,
we may as well put it in this if statement.

 
       args->Append(ExtensionTabUtil::CreateTabObject(web_contents)->ToValue());
     } else {
       args->Append(base::MakeUnique<base::DictionaryValue>());
     }
   }
 
+  // |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();
     raw_properties->SetBoolean("wasChecked", was_checked);

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

Similarly here, let's only grab the dictionary if we need to, and scope its use
to this block.

 
     // RADIO items always get set to true when you click on them, but CHECKBOX
     // items get their state toggled.
     bool checked =
         (item->type() == MenuItem::RADIO) ? true : !was_checked;
 
     item->SetChecked(checked);
     raw_properties->SetBoolean("checked", item->checked());
 
     if (extension)
@@ skipping 264 matching lines @@
 bool MenuItem::Id::operator!=(const Id& other) const {
   return !(*this == other);
 }
 
 bool MenuItem::Id::operator<(const Id& other) const {
   return std::tie(incognito, extension_key, uid, string_uid) <
     std::tie(other.incognito, other.extension_key, other.uid, other.string_uid);
 }
 
 }  // namespace extensions