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

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

This CL is a first step to inlining base::ListValue. It is proposed to use an
std::vector<base::Value> as the underlying ListStorage. This CL implements the
change and updates the code accordingly.

TBR=bajones@chromium.org, dbeam@chromium.org, stevenjb@chromium.org
BUG=646113
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2740143002
Cr-Commit-Position: refs/heads/master@{#463618}
Committed: https://chromium.googlesource.com/chromium/src/+/ebab0defaea3aed024283a64088ebc0cd352b47f

[jdoerrie, 2017-03-09 15:26:29]

Description was changed from

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

This CL is a first step to inlining base::ListValue. It is proposed to use an
std::vector<base::Value> as the underlying ListStorage. This CL implements the
change and updates the code accordingly.

BUG=646113
==========

to

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

This CL is a first step to inlining base::ListValue. It is proposed to use an
std::vector<base::Value> as the underlying ListStorage. This CL implements the
change and updates the code accordingly.

BUG=646113
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[jdoerrie, 2017-03-09 15:27:23]

The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 15:27:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-09 15:36:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-09 15:36:36]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[jdoerrie, 2017-03-10 08:50:18]

The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 08:50:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 08:59:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 08:59:15]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)

[jdoerrie, 2017-03-10 12:41:55]

The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 12:42:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 12:50:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-10 12:50:35]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[jdoerrie, 2017-03-14 12:23:34]

The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 12:23:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 12:36:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-14 12:36:21]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[jdoerrie, 2017-03-14 20:58:38]

The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 21:00:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 22:43:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-14 22:43:23]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jdoerrie, 2017-03-15 09:23:20]

The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 09:23:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jdoerrie, 2017-03-15 09:50:59]

Description was changed from

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

This CL is a first step to inlining base::ListValue. It is proposed to use an
std::vector<base::Value> as the underlying ListStorage. This CL implements the
change and updates the code accordingly.

BUG=646113
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

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

This CL is a first step to inlining base::ListValue. It is proposed to use an
std::vector<base::Value> as the underlying ListStorage. This CL implements the
change and updates the code accordingly.

BUG=646113
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[jdoerrie, 2017-03-15 09:51:00]

jdoerrie@chromium.org changed reviewers:
+ brettw@chromium.org, flackr@chromium.org, rdevlin.cronin@chromium.org,
skym@chromium.org

[jdoerrie, 2017-03-15 10:10:41]

This change turned out to be more complicated than expected, but should have net
benefits regarding both performance and memory usage.

Dear Reviewers, please pay special attention to these files:
- base/values.h
- base/values.cc
- tools/json_schema_compiler/cc_generator.py
- components/browser_sync/profile_sync_service.cc
- chrome/browser/ui/webui/certificate_viewer_webui.cc
- components/sync/driver/about_sync_util.cc

In particular:
brettw@: please review base/*
rdevlin.cronin@: please review tools/json_schema_compiler/cc_generator.py
flackr@: plese review chrome/browser/ui/webui/certificate_viewer_webui.cc
skym@: please review:
	- components/sync/driver/about_sync_util.cc
	- components/browser_sync/profile_sync_service.cc

## General Summary of Changes

### Significant Changes
#### changed underlying ListStorage, added |capacity| and |reserve| methods:
- base/values.h
- base/values.cc

#### changed code generation logic:
- tools/json_schema_compiler/cc_generator.py

#### added extra logic to prevent running out of memory in:
- components/browser_sync/profile_sync_service.cc

#### added significant logic to prevent use-after-free in:
- chrome/browser/ui/webui/certificate_viewer_webui.cc
- components/sync/driver/about_sync_util.cc

### Mostly Trivial Changes
#### added some logic to prevent use-after-free in:
- base/trace_event/trace_event_argument.cc
- base/values_unittest.cc
- chrome/browser/extensions/menu_manager.cc
- chrome/browser/net/predictor_unittest.cc
- components/autofill/core/browser/autofill_address_util.cc
- components/error_page/common/localized_error.cc
- components/signin/core/browser/account_tracker_service.cc
- extensions/common/extension_l10n_util_unittest.cc

#### dropped impossible to reach log(error) statement:
- chrome/browser/chromeos/platform_keys/key_permissions.cc

#### wrapped returned base::value in makeunique in:
- chrome/browser/permissions/chooser_context_base.cc

#### dropped const in:
- chrome/browser/extensions/extension_web_ui.cc
- chrome/browser/media_galleries/media_galleries_preferences.cc
- chrome/browser/permissions/chooser_context_base.cc
- chrome/browser/plugins/plugin_prefs.cc
- chromeos/network/onc/onc_utils.cc
- extensions/browser/api/networking_private/networking_private_chromeos.cc

#### added extra consts in:
- chrome/browser/devtools/chrome_devtools_manager_delegate.cc
- chrome/browser/devtools/device/devtools_device_discovery.cc
- chrome/browser/extensions/api/content_settings/content_settings_store.cc
- chrome/browser/media_galleries/media_galleries_preferences.cc
- chrome/browser/metrics/plugin_metrics_provider.cc
- chrome/browser/plugins/plugin_finder.cc
- chrome/browser/plugins/plugin_finder_unittest.cc
- chrome/browser/printing/cloud_print/cloud_print_printer_list.cc
- chrome/browser/supervised_user/child_accounts/family_info_fetcher.cc
- chrome/browser/ui/webui/browsing_history_handler.cc
- chrome/browser/ui/webui/chromeos/login/l10n_util.cc
- chrome/browser/ui/webui/options/content_settings_handler.cc
- chrome/common/extensions/api/file_browser_handlers/file_browser_handler.cc
- chrome/test/chromedriver/logging.cc
- chrome/test/chromedriver/performance_logger.cc
- chromeos/network/managed_network_configuration_handler_impl.cc
- chromeos/network/network_sms_handler.cc
- chromeos/printing/ppd_provider.cc
- components/arc/net/arc_net_host_impl.cc
- components/url_matcher/url_matcher_factory.cc
- extensions/browser/api/networking_private/networking_private_linux.cc
- extensions/browser/verified_contents.cc
- remoting/protocol/http_ice_config_request.cc
- remoting/test/host_list_fetcher.cc

#### changed explicit const std::unique_ptr<base::value>& to const base::value&:
-
chrome/browser/chromeos/extensions/quick_unlock_private/quick_unlock_private_api_unittest.cc
- chrome/browser/extensions/extension_web_ui.cc
- chrome/browser/profiles/profile_manager.cc
- chrome/browser/ui/webui/options/certificate_manager_handler.cc
- chrome/browser/ui/webui/settings/certificates_handler.cc
- chromeos/printing/ppd_provider.cc
- components/ntp_snippets/category_rankers/click_based_category_ranker.cc
- components/ntp_snippets/content_suggestions_service.cc
- components/ntp_snippets/pref_util.cc
- components/ntp_snippets/remote/remote_suggestions_provider_impl.cc
- content/browser/browsing_data/clear_site_data_throttle.cc
- extensions/common/manifest_handlers/action_handlers_handler.cc

#### modified iterator and pointer dereferencing:
- all remaining files, as well as most files mentioned above

https://codereview.chromium.org/2740143002/diff/140001/base/values.h
File base/values.h (right):

https://codereview.chromium.org/2740143002/diff/140001/base/values.h#newcode77
base/values.h:77: Value(Value&& that);
Given the crash in components/browser_sync/profile_sync_service.cc I was
considering adding |noexcept| here to allow |push_back| to move during a
reallocation. I saw you explored |noexcept| in the context of |flat_map|, but
decided against it in the end. Do you think it would make sense here?

https://codereview.chromium.org/2740143002/diff/140001/base/values.h#newcode390
base/values.h:390: void Reserve(size_t n);
I tried to minimize API changes, but these were strictly necessary to not have
crashes in components/sync/driver/about_sync_util.cc.

https://codereview.chromium.org/2740143002/diff/140001/base/values.h#newcode445
base/values.h:445: void Append(std::unique_ptr<Value> in_value);
Given how cumbersome and error-prone fixing
|chrome/browser/ui/webui/certificate_viewer_webui.cc| became, I was seriously
considering returning a raw pointer to the just inserted element here. However,
I did want to minimize API changes and this change would likely only be
temporary anyway, so I didn't do it yet. What do you think?

https://codereview.chromium.org/2740143002/diff/140001/components/browser_syn...
File components/browser_sync/profile_sync_service.cc (right):

https://codereview.chromium.org/2740143002/diff/140001/components/browser_syn...
components/browser_sync/profile_sync_service.cc:2115:
result_accumulator_->Reserve(awaiting_types_.Size());
This was a tricky one to debug. Prior to this change chrome_sync_shell_test_apk
crashed in line 2135 when |Append| triggered a reallocation and ran out of
memory
(https://luci-milo.appspot.com/buildbot/tryserver.chromium.android/linux_andro...).
This was not a problem before, because a reallocation simply resulted in moving
unique_ptrs around and did not trigger a deep copy of all results.

Adding a |Reserve| here fixes this issue, because it implies push_back never has
to reallocate. However, there might be other options as well (see the comment
regarding noexcept in |Value(Value&&)|).

[jdoerrie, 2017-03-15 10:12:51]

The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 10:13:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 12:13:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-15 12:13:15]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2740143002/diff/140001/base/values.h
File base/values.h (right):

https://codereview.chromium.org/2740143002/diff/140001/base/values.h#newcode100
base/values.h:100: explicit Value(const ListStorage& in_list);
Not my area, so no need to wait for my signoff, but it seems like doing this for
complex types (like List and Dictionary) could lead to some accidental expensive
copies.  Is there a reason to prefer this rather than just having the move
semantics and an explicit Copy() method?

https://codereview.chromium.org/2740143002/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/menu_manager.cc (right):

https://codereview.chromium.org/2740143002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/menu_manager.cc:672: base::DictionaryValue*
raw_properties;
nit: initialize to nullptr

https://codereview.chromium.org/2740143002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/menu_manager.cc:682:
raw_properties->SetInteger("frameId", frame_id);
Given raw_properties is only safe to use before the appends immediately below,
we may as well put it in this if statement.

https://codereview.chromium.org/2740143002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/menu_manager.cc:690: // |args->Append| might have
invalidated pointers, which is why |args| needs
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?

https://codereview.chromium.org/2740143002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/menu_manager.cc:697:
raw_properties->SetBoolean("wasChecked", was_checked);
Similarly here, let's only grab the dictionary if we need to, and scope its use
to this block.

https://codereview.chromium.org/2740143002/diff/140001/extensions/common/exte...
File extensions/common/extension_l10n_util_unittest.cc (right):

https://codereview.chromium.org/2740143002/diff/140001/extensions/common/exte...
extensions/common/extension_l10n_util_unittest.cc:434:
handlers->GetDictionary(handlers->GetSize() - 1, &handler);
We can just inline the creation of the new DictionaryValue, since we won't use
it before refetching.
handlers->Append(base::MakeUnique<base::DictionaryValue>());
base::DictionaryValue* handler = nullptr;
handlers->GetDictionary(...);

https://codereview.chromium.org/2740143002/diff/140001/tools/json_schema_comp...
File tools/json_schema_compiler/cc_generator.py (right):

https://codereview.chromium.org/2740143002/diff/140001/tools/json_schema_comp...
tools/json_schema_compiler/cc_generator.py:975: .Sblock('if
(!%s%sGetAsString(&%s)) {' % (src_var,
These changes look generally fine, but I'm a little surprised there weren't more
necessary... have you tried rebuilding from scratch* to ensure we regenerate all
the files this affects?  (In an ideal world, this would be done by the deps, and
it largely is... but I think we've seen issues where the rules aren't quite
perfect before.)

*Really, all you need to do is rm -rf the out/debug/gen/extensions/ and
out/debug/gen/chrome/common/extensions/ directories.

https://codereview.chromium.org/2740143002/diff/140001/tools/json_schema_comp...
tools/json_schema_compiler/cc_generator.py:980:
self._util_cc_helper.GetValueTypeString('%%(src_var)s', True)))
We should update this True here to be is_ptr.

[flackr, 2017-03-15 15:29:39]

https://codereview.chromium.org/2740143002/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/certificate_viewer_webui.cc (right):

https://codereview.chromium.org/2740143002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/certificate_viewer_webui.cc:301:
root_list.GetDictionary(root_list.GetSize() - 1, &node_details);
I think this would be a lot cleaner and more efficient to defer appending until
we were done constructing each descendant value so that we never need the raw
pointers. i.e.
std::unique_ptr<base::DictionaryValue> node_details =
MakeUnique(base::DictionaryValue>());
node_details->SetString("label", ...);

// construct and append all nodes to node_details
// and finally

root_list.Append(std::move(node_details));

[skym, 2017-03-15 16:57:24]

https://codereview.chromium.org/2740143002/diff/140001/components/browser_syn...
File components/browser_sync/profile_sync_service.cc (right):

https://codereview.chromium.org/2740143002/diff/140001/components/browser_syn...
components/browser_sync/profile_sync_service.cc:2115:
result_accumulator_->Reserve(awaiting_types_.Size());
On 2017/03/15 10:10:41, jdoerrie wrote:
> This was a tricky one to debug. Prior to this change
chrome_sync_shell_test_apk
> crashed in line 2135 when |Append| triggered a reallocation and ran out of
> memory
>
(https://luci-milo.appspot.com/buildbot/tryserver.chromium.android/linux_andro...).
> This was not a problem before, because a reallocation simply resulted in
moving
> unique_ptrs around and did not trigger a deep copy of all results.
> 
> Adding a |Reserve| here fixes this issue, because it implies push_back never
has
> to reallocate. However, there might be other options as well (see the comment
> regarding noexcept in |Value(Value&&)|).

While I don't understand why noexcept is required for push_back to support
movable types, moving things around seems far superior to having to call
|Reserve|.

Taking a step back, even if this does do a deep copy on reallocation, I'm
surprised it ran out of memory. It must have already been really close to the
threshold. Looking at the failed cases, testUploadOpenTab just opens a single
tab... Most |node_list|s should have been empty...

https://codereview.chromium.org/2740143002/diff/140001/components/sync/driver...
File components/sync/driver/about_sync_util.cc (right):

https://codereview.chromium.org/2740143002/diff/140001/components/sync/driver...
components/sync/driver/about_sync_util.cc:137:
section->GetDictionary(section->GetSize() - 1, &stat_);
Are these GetDictionary(x, DictionaryValue**) methods even sticking around or
being removed as part of 646113?

https://codereview.chromium.org/2740143002/diff/140001/components/sync/driver...
components/sync/driver/about_sync_util.cc:283: stats_list->Reserve(12);
I've always really disliked how this file was implemented. Your approach seems
like a reasonable surgical change to make things work, but I'd really like to
rewrite it to avoid holding raw pointers. Can you create a bug and assign it to
me to remove the Reserve usage from this file.

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

https://codereview.chromium.org/2740143002/diff/140001/base/values.h
File base/values.h (right):

https://codereview.chromium.org/2740143002/diff/140001/base/values.h#newcode445
base/values.h:445: void Append(std::unique_ptr<Value> in_value);
On 2017/03/15 10:10:41, jdoerrie wrote:
> Given how cumbersome and error-prone fixing
> |chrome/browser/ui/webui/certificate_viewer_webui.cc| became, I was seriously
> considering returning a raw pointer to the just inserted element here.
However,
> I did want to minimize API changes and this change would likely only be
> temporary anyway, so I didn't do it yet. What do you think?

STL doesn't return an iterator from vector::push_back and it seems like we
should stick with an API that mirrors that when possible.

https://codereview.chromium.org/2740143002/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/menu_manager.cc (right):

https://codereview.chromium.org/2740143002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/menu_manager.cc:690: // |args->Append| might have
invalidated pointers, which is why |args| needs
On 2017/03/15 15:25:03, Devlin wrote:
> 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?

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

https://codereview.chromium.org/2740143002/diff/140001/chromecast/crash/linux...
File chromecast/crash/linux/crash_testing_utils.cc (right):

https://codereview.chromium.org/2740143002/diff/140001/chromecast/crash/linux...
chromecast/crash/linux/crash_testing_utils.cc:98: std::unique_ptr<DumpInfo>
dump(new DumpInfo(&elem));
The ownership of this is scary to me. Can you do a quick followup patch to make
the DumpInfo take a "const Value&"?

https://codereview.chromium.org/2740143002/diff/140001/components/browser_syn...
File components/browser_sync/profile_sync_service.cc (right):

https://codereview.chromium.org/2740143002/diff/140001/components/browser_syn...
components/browser_sync/profile_sync_service.cc:2115:
result_accumulator_->Reserve(awaiting_types_.Size());
I thought about this for a while and it doesn't make sense to me. I don't want
to delay this patch since it's probably collecting merge conflicts as I write,
but I think we should understand this since I'm worried we could be wallpapering
over another problem.

Were you able to get this in a debugger? Do you know how big an array it was
trying to allocate? Theoretically resizes should be doing moves so we shouldn't
even be copying the values.

This is getting all the sync nodes. This is a lot, but it should fit into
memory, and since this is a test with limited canned data, it should be no
problem at all.

https://codereview.chromium.org/2740143002/diff/140001/components/sync/driver...
File components/sync/driver/about_sync_util.cc (right):

https://codereview.chromium.org/2740143002/diff/140001/components/sync/driver...
components/sync/driver/about_sync_util.cc:137:
section->GetDictionary(section->GetSize() - 1, &stat_);
These should all be removed, see the design doc I linked in a different comment.

https://codereview.chromium.org/2740143002/diff/140001/components/sync/driver...
components/sync/driver/about_sync_util.cc:283: stats_list->Reserve(12);
On 2017/03/15 16:57:23, skym wrote:
> I've always really disliked how this file was implemented. Your approach seems
> like a reasonable surgical change to make things work, but I'd really like to
> rewrite it to avoid holding raw pointers. Can you create a bug and assign it
to
> me to remove the Reserve usage from this file.

Can you add TODOs referencing said bug on the Stat object constructors you
changed above, as well as all the Reserve() calls you added? That way skym can
find all the places to be sure to fix :)

[jdoerrie, 2017-03-16 19:01:56]

Thank you for your reviews so far. I will follow up with actual code changes
once I got to the bottom of what's going on in
components/browser_sync/profile_sync_service.cc.

https://codereview.chromium.org/2740143002/diff/140001/base/values.h
File base/values.h (right):

https://codereview.chromium.org/2740143002/diff/140001/base/values.h#newcode100
base/values.h:100: explicit Value(const ListStorage& in_list);
On 2017/03/15 15:25:03, Devlin wrote:
> Not my area, so no need to wait for my signoff, but it seems like doing this
for
> complex types (like List and Dictionary) could lead to some accidental
expensive
> copies.  Is there a reason to prefer this rather than just having the move
> semantics and an explicit Copy() method?

Currently this is only used within |Value::DeepCopy| where the semantics are
quite explicit. I do agree that having this public is currently not needed and
could be completely inlined. I will soon follow up with a post to the refactor
doc to start a discussion regarding the new public API of ListValue, and will
mention this point there. 

My initial motivation for including it was that it is mostly a convenience
function when callers already happen to have access to a
std::vector<base::Value> and want to turn it into a List Value. Marking it as
explicit should hopefully help to avoid accidental expensive copies. But I
certainly agree that this is better discussed in the doc.

https://codereview.chromium.org/2740143002/diff/140001/base/values.h#newcode445
base/values.h:445: void Append(std::unique_ptr<Value> in_value);
On 2017/03/15 22:14:14, brettw (plz ping after 24h) wrote:
> On 2017/03/15 10:10:41, jdoerrie wrote:
> > Given how cumbersome and error-prone fixing
> > |chrome/browser/ui/webui/certificate_viewer_webui.cc| became, I was
seriously
> > considering returning a raw pointer to the just inserted element here.
> However,
> > I did want to minimize API changes and this change would likely only be
> > temporary anyway, so I didn't do it yet. What do you think?
> 
> STL doesn't return an iterator from vector::push_back and it seems like we
> should stick with an API that mirrors that when possible.

Acknowledged.

https://codereview.chromium.org/2740143002/diff/140001/chromecast/crash/linux...
File chromecast/crash/linux/crash_testing_utils.cc (right):

https://codereview.chromium.org/2740143002/diff/140001/chromecast/crash/linux...
chromecast/crash/linux/crash_testing_utils.cc:98: std::unique_ptr<DumpInfo>
dump(new DumpInfo(&elem));
On 2017/03/15 22:14:14, brettw (plz ping after 24h) wrote:
> The ownership of this is scary to me. Can you do a quick followup patch to
make
> the DumpInfo take a "const Value&"?

Could you elaborate on what exactly you mean? From my understanding of the code
|DumpInfo| simply forwards the pointer to |ParseEntry| where it copies the
contained strings into private members and does not take ownership of the
pointer
(https://codesearch.chromium.org/chromium/src/chromecast/crash/linux/dump_info...).

https://codereview.chromium.org/2740143002/diff/140001/components/browser_syn...
File components/browser_sync/profile_sync_service.cc (right):

https://codereview.chromium.org/2740143002/diff/140001/components/browser_syn...
components/browser_sync/profile_sync_service.cc:2115:
result_accumulator_->Reserve(awaiting_types_.Size());
On 2017/03/15 22:14:14, brettw (plz ping after 24h) wrote:
> I thought about this for a while and it doesn't make sense to me. I don't want
> to delay this patch since it's probably collecting merge conflicts as I write,
> but I think we should understand this since I'm worried we could be
wallpapering
> over another problem.
> 
> Were you able to get this in a debugger? Do you know how big an array it was
> trying to allocate? Theoretically resizes should be doing moves so we
shouldn't
> even be copying the values.
> 
> This is getting all the sync nodes. This is a lot, but it should fit into
> memory, and since this is a test with limited canned data, it should be no
> problem at all.

I reinvestigated this today, but it still does not make much sense to me. I am
able to reliably reproduce this on a local device and in an emulator, but I am
pretty much limited to printf debugging. Here is the stack trace I was able to
get from the device:

pid: 3227, tid: 3227, >>> org.chromium.chrome.sync_shell <<<
signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x0

Stack Trace:
  RELADDR   FUNCTION
  v------>  begin                      .../llvm-libc++/libcxx/include/__tree:911
  v------>  begin                      .../llvm-libc++/libcxx/include/map:970
  001685da  DeepCopy                   base/values.cc:383
  v------>  CreateDeepCopy             base/values.cc:401
  0016734b  InternalCopyConstructFrom  base/values.cc:502
  v------>  Value                      base/values.cc:91
  v------>  construct<...>             .../libcxx/include/memory:1645
  v------>  __construct<...>           .../libcxx/include/memory:1572
  v------>  construct<...>             .../libcxx/include/memory:1453
  v------>  __construct_at_end<...>    .../libcxx/include/vector:1009
  0016befb  vector                     .../libcxx/include/vector:1207
  v------>  Init<...>                  base/memory/manual_constructor.h:58
  v------>  Value                      base/values.cc:176
  001686ae  DeepCopy                   base/values.cc:392
  v------>  CreateDeepCopy             base/values.cc:401
  0016861a  DeepCopy                   base/values.cc:385
  v------>  CreateDeepCopy             base/values.cc:401
  0016734b  InternalCopyConstructFrom  base/values.cc:502
  v------>  Value                      base/values.cc:91
  v------>  construct<...>             .../libcxx/include/memory:1645
  v------>  __construct<...>           .../libcxx/include/memory:1572
  v------>  construct<...>             .../libcxx/include/memory:1453
  v------>  __construct_backward<...>  .../libcxx/include/memory:1535
  0016c93d  __swap_out_circular_buffer .../libcxx/include/vector:878
  0016cae9  __push_back_slow_path<...> .../libcxx/include/vector:1575
  v------>  push_back                  .../libcxx/include/vector:1610
  0016ad79  Append                     base/values.cc:1267
  00f028c5  OnReceivedNodesForType     .../profile_sync_service.cc:2135
  v------>  Invoke<...>                base/bind_internal.h:214
  v------>  MakeItSo<...>              base/bind_internal.h:285
  v------>  RunImpl<...>               base/bind_internal.h:361
  00f05d59  Run                        base/bind_internal.h:339
  v------>  Run                        base/callback.h:85
  00359eed  GetAllNodes                .../directory_data_type_controller.cc:67
  00f02659  GetAllNodes                .../profile_sync_service.cc:2168
  007fc416  GetAllNodes                .../profile_sync_service_android.cc:353

Somewhere an invalid memory location is accessed, but I am not quite sure why.
You are right, though, It probably is not a running out of memory issue. I did
some analysis on the size of objects and found out |results_accumulator_|
consists of about 540 Value instances total (recursively counting through all
contained dictionaries). Calling reserve or adding noexcept makes the test pass
by simply avoiding to call DeepCopy, but this doesn't really fix the underlying
issue here.

I will continue my investigation tomorrow, but I am a bit clueless right now and
would appreciate any ideas and insights into what could go on here.

[skym, 2017-03-16 19:52:39]

https://codereview.chromium.org/2740143002/diff/140001/components/sync/driver...
File components/sync/driver/about_sync_util.cc (right):

https://codereview.chromium.org/2740143002/diff/140001/components/sync/driver...
components/sync/driver/about_sync_util.cc:81: // |parent_list| and its members
will be invalidated. This would result in
Wait, I thought I understood this, but now I'm confused. I thought raw pointers
to the ListValue |parent_list| object would still valid. Only pointers to the
contents of |parent_list| would be invalidated, right? But this comment seems to
indicate otherwise. Or am I confused?

[brettw, 2017-03-17 05:39:19]

I have a suggestion for the crash, test that (there are two similar calls), make
sure it fixes the problem before thinking about noexcept.

I want to do the noexcept also, but maybe it's easier to do in a followup patch.

https://codereview.chromium.org/2740143002/diff/140001/base/values.cc
File base/values.cc (right):

https://codereview.chromium.org/2740143002/diff/140001/base/values.cc#newcode502
base/values.cc:502:
dict_ptr_.Init(std::move(*that.CreateDeepCopy()->dict_ptr_));
I suspect this line is causing the Android crash. CreateDeepCopy creates a
temporary unique_ptr, we get the dict_ptr out of that, but then the unique_ptr
is destroyed and we pass a dangling pointer to std::move.

Try making a named unique_ptr<Value> to hold the deep copy on the previous line
and see if that makes the problem go away.

https://codereview.chromium.org/2740143002/diff/140001/base/values.cc#newcode558
base/values.cc:558: *dict_ptr_ = std::move(*that.CreateDeepCopy()->dict_ptr_);
Ditto here.

https://codereview.chromium.org/2740143002/diff/140001/base/values.h
File base/values.h (right):

https://codereview.chromium.org/2740143002/diff/140001/base/values.h#newcode77
base/values.h:77: Value(Value&& that);
Yeah, any move that we can write noexcept for we should do since it avoid a
bunch of work. I forget the problem I had with flat map, but it ended up with
some case that was really hard to make compile. If it compiles to write noexcept
here, we should definitely do it.

This applies for all the other move operations below.

https://codereview.chromium.org/2740143002/diff/140001/chromecast/crash/linux...
File chromecast/crash/linux/crash_testing_utils.cc (right):

https://codereview.chromium.org/2740143002/diff/140001/chromecast/crash/linux...
chromecast/crash/linux/crash_testing_utils.cc:98: std::unique_ptr<DumpInfo>
dump(new DumpInfo(&elem));
On 2017/03/16 19:01:56, jdoerrie wrote:
> On 2017/03/15 22:14:14, brettw (plz ping after 24h) wrote:
> > The ownership of this is scary to me. Can you do a quick followup patch to
> make
> > the DumpInfo take a "const Value&"?
> 
> Could you elaborate on what exactly you mean? From my understanding of the
code
> |DumpInfo| simply forwards the pointer to |ParseEntry| where it copies the
> contained strings into private members and does not take ownership of the
> pointer
>
(https://codesearch.chromium.org/chromium/src/chromecast/crash/linux/dump_info...).

Yeah, you're right, it's the same as everywhere else. I was imagining DumpInfo
was keeping the pointer.

[jdoerrie, 2017-03-23 18:10:05]

The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-23 18:10:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jdoerrie, 2017-03-23 18:11:18]

Next round. I am still struggling with Android, but I addressed most other
comments for now.

https://codereview.chromium.org/2740143002/diff/140001/base/values.cc
File base/values.cc (right):

https://codereview.chromium.org/2740143002/diff/140001/base/values.cc#newcode502
base/values.cc:502:
dict_ptr_.Init(std::move(*that.CreateDeepCopy()->dict_ptr_));
On 2017/03/17 05:39:19, brettw (plz ping after 24h) wrote:
> I suspect this line is causing the Android crash. CreateDeepCopy creates a
> temporary unique_ptr, we get the dict_ptr out of that, but then the unique_ptr
> is destroyed and we pass a dangling pointer to std::move.
> 
> Try making a named unique_ptr<Value> to hold the deep copy on the previous
line
> and see if that makes the problem go away.

Unfortunately this didn't solve the issue. After more fruitless printf debugging
I am now trying (and currently still failing) to get the test running with an
ASAN build. This will hopefully make the issue obvious.

https://codereview.chromium.org/2740143002/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/menu_manager.cc (right):

https://codereview.chromium.org/2740143002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/menu_manager.cc:672: base::DictionaryValue*
raw_properties;
On 2017/03/15 15:25:04, Devlin wrote:
> nit: initialize to nullptr

Done.

https://codereview.chromium.org/2740143002/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/menu_manager.cc:690: // |args->Append| might have
invalidated pointers, which is why |args| needs
On 2017/03/15 22:14:14, brettw (plz ping after 24h) wrote:
> On 2017/03/15 15:25:03, Devlin wrote:
> > 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?
> 
> Yes, there should be no pointers in the API when we're done:
>
https://docs.google.com/document/d/1uDLu5uTRlCWePxQUEHc8yNQdEoE1BDISYdpggWEAB...

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.

https://codereview.chromium.org/2740143002/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/certificate_viewer_webui.cc (right):

https://codereview.chromium.org/2740143002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/certificate_viewer_webui.cc:301:
root_list.GetDictionary(root_list.GetSize() - 1, &node_details);
On 2017/03/15 15:29:39, flackr wrote:
> I think this would be a lot cleaner and more efficient to defer appending
until
> we were done constructing each descendant value so that we never need the raw
> pointers. i.e.
> std::unique_ptr<base::DictionaryValue> node_details =
> MakeUnique(base::DictionaryValue>());
> node_details->SetString("label", ...);
> 
> // construct and append all nodes to node_details
> // and finally
> 
> root_list.Append(std::move(node_details));

Done. This turned out to be quite tricky with the various levels of nesting.

https://codereview.chromium.org/2740143002/diff/140001/components/sync/driver...
File components/sync/driver/about_sync_util.cc (right):

https://codereview.chromium.org/2740143002/diff/140001/components/sync/driver...
components/sync/driver/about_sync_util.cc:81: // |parent_list| and its members
will be invalidated. This would result in
On 2017/03/16 19:52:39, skym wrote:
> Wait, I thought I understood this, but now I'm confused. I thought raw
pointers
> to the ListValue |parent_list| object would still valid. Only pointers to the
> contents of |parent_list| would be invalidated, right? But this comment seems
to
> indicate otherwise. Or am I confused?

My bad, your understanding is correct. I updated the comment now.

https://codereview.chromium.org/2740143002/diff/140001/components/sync/driver...
components/sync/driver/about_sync_util.cc:283: stats_list->Reserve(12);
On 2017/03/15 22:14:14, brettw (plz ping after 24h) wrote:
> On 2017/03/15 16:57:23, skym wrote:
> > I've always really disliked how this file was implemented. Your approach
seems
> > like a reasonable surgical change to make things work, but I'd really like
to
> > rewrite it to avoid holding raw pointers. Can you create a bug and assign it
> to
> > me to remove the Reserve usage from this file.
> 
> Can you add TODOs referencing said bug on the Stat object constructors you
> changed above, as well as all the Reserve() calls you added? That way skym can
> find all the places to be sure to fix :)

Done.

[flackr, 2017-03-23 19:23:58]

chrome/browser/ui/webui/certificate_viewer_webui.cc lgtm

https://codereview.chromium.org/2740143002/diff/140001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/certificate_viewer_webui.cc (right):

https://codereview.chromium.org/2740143002/diff/140001/chrome/browser/ui/webu...
chrome/browser/ui/webui/certificate_viewer_webui.cc:301:
root_list.GetDictionary(root_list.GetSize() - 1, &node_details);
On 2017/03/23 18:11:17, jdoerrie wrote:
> On 2017/03/15 15:29:39, flackr wrote:
> > I think this would be a lot cleaner and more efficient to defer appending
> until
> > we were done constructing each descendant value so that we never need the
raw
> > pointers. i.e.
> > std::unique_ptr<base::DictionaryValue> node_details =
> > MakeUnique(base::DictionaryValue>());
> > node_details->SetString("label", ...);
> > 
> > // construct and append all nodes to node_details
> > // and finally
> > 
> > root_list.Append(std::move(node_details));
> 
> Done. This turned out to be quite tricky with the various levels of nesting.

I see that, thanks for bearing with me on this. I think the only way to make
things cleaner would be to have more unique_ptrs which are more locally scoped,
then the dictionaries and lists could still be roughly built in top-down order
but appended in bottom-up order. I don't want to put this burden on you though.

https://codereview.chromium.org/2740143002/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/certificate_viewer_webui.cc (right):

https://codereview.chromium.org/2740143002/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/certificate_viewer_webui.cc:296:
std::unique_ptr<base::DictionaryValue> alt_node_details;
nit: Let's try to move some of these declarations to where they are used - see
https://google.github.io/styleguide/cppguide.html#Local_Variables for general
guidelines.

[commit-bot: I haz the power, 2017-03-23 19:46:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-23 19:46:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jdoerrie, 2017-03-24 08:53:45]

The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 08:53:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jdoerrie, 2017-03-24 08:54:37]

https://codereview.chromium.org/2740143002/diff/140001/extensions/common/exte...
File extensions/common/extension_l10n_util_unittest.cc (right):

https://codereview.chromium.org/2740143002/diff/140001/extensions/common/exte...
extensions/common/extension_l10n_util_unittest.cc:434:
handlers->GetDictionary(handlers->GetSize() - 1, &handler);
On 2017/03/15 15:25:04, Devlin wrote:
> We can just inline the creation of the new DictionaryValue, since we won't use
> it before refetching.
> handlers->Append(base::MakeUnique<base::DictionaryValue>());
> base::DictionaryValue* handler = nullptr;
> handlers->GetDictionary(...);

Done.

https://codereview.chromium.org/2740143002/diff/140001/tools/json_schema_comp...
File tools/json_schema_compiler/cc_generator.py (right):

https://codereview.chromium.org/2740143002/diff/140001/tools/json_schema_comp...
tools/json_schema_compiler/cc_generator.py:975: .Sblock('if
(!%s%sGetAsString(&%s)) {' % (src_var,
On 2017/03/15 15:25:04, Devlin wrote:
> These changes look generally fine, but I'm a little surprised there weren't
more
> necessary... have you tried rebuilding from scratch* to ensure we regenerate
all
> the files this affects?  (In an ideal world, this would be done by the deps,
and
> it largely is... but I think we've seen issues where the rules aren't quite
> perfect before.)
> 
> *Really, all you need to do is rm -rf the out/debug/gen/extensions/ and
> out/debug/gen/chrome/common/extensions/ directories.

I was surprised by this as well, but it looks like conditionally changing this
one accessor is all that's necessary. I followed your constructions to clear the
generated code and recompiled without any problems.

https://codereview.chromium.org/2740143002/diff/140001/tools/json_schema_comp...
tools/json_schema_compiler/cc_generator.py:980:
self._util_cc_helper.GetValueTypeString('%%(src_var)s', True)))
On 2017/03/15 15:25:04, Devlin wrote:
> We should update this True here to be is_ptr.

Done.

https://codereview.chromium.org/2740143002/diff/160001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/certificate_viewer_webui.cc (right):

https://codereview.chromium.org/2740143002/diff/160001/chrome/browser/ui/webu...
chrome/browser/ui/webui/certificate_viewer_webui.cc:296:
std::unique_ptr<base::DictionaryValue> alt_node_details;
On 2017/03/23 19:23:57, flackr wrote:
> nit: Let's try to move some of these declarations to where they are used - see
> https://google.github.io/styleguide/cppguide.html#Local_Variables for general
> guidelines.

Done.

[commit-bot: I haz the power, 2017-03-24 09:01:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-24 09:01:09]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jdoerrie, 2017-03-24 09:13:27]

The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 09:13:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 09:20:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-24 09:20:44]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[jdoerrie, 2017-03-24 14:09:20]

The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-24 14:09:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-24 14:33:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-24 14:33:50]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[brettw, 2017-03-24 17:22:27]

Looks like you have a trivial addition needed in
extensions/renderer/api_binding.cc

I spent some more time thinking about the Android crash and I'm out of ideas. Do
you know how to catch it in the debugger? I've never actually used a debugger on
Android...

[jdoerrie, 2017-03-27 15:31:55]

The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 15:32:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jdoerrie, 2017-03-27 15:42:34]

On 2017/03/24 17:22:27, brettw (plz ping after 24h) wrote:
> Looks like you have a trivial addition needed in
> extensions/renderer/api_binding.cc
> 
> I spent some more time thinking about the Android crash and I'm out of ideas.
Do
> you know how to catch it in the debugger? I've never actually used a debugger
on
> Android...

I found the following instructions:
https://chromium.googlesource.com/chromium/src/+/master/docs/android_debuggin...

Unfortunately they are a bit outdated, though. 
--test-arguments=--wait-for-debugger does not exist anymore. Supposedly there is
a simple -w flag now, but for me it did not wait for the debugger.

I managed to reproduce the issue in the following way:

1. Build Chrome and SyncShellTest:
ninja -C out/Android -j 2500 chrome_public_apk chrome_sync_shell_test_apk

2. Install Chrome:
build/android/adb_install_apk.py out/Android/apks/ChromePublic.apk

3. Run Test with correct filter:
./out/Android/bin/run_chrome_sync_shell_test_apk --release
--gtest_filter="*.testDownloadDeletedTypedUrl" -v -v -v

Then you can run this to try to attach with GDB:
build/android/adb_gdb --output-directory=out/Android
--package-name=org.chromium.chrome.sync_shell

Given that the move constructor is now noexcept, you probably need to remove
that keyword in order to reproduce the crash.

[commit-bot: I haz the power, 2017-03-27 16:01:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-27 16:01:37]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[jdoerrie, 2017-03-27 16:29:06]

The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-27 16:29:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skym, 2017-03-27 17:40:58]

about_sync_util.cc and other sync changes lgtm

[commit-bot: I haz the power, 2017-03-27 17:43:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-27 17:43:39]

Dry run: This issue passed the CQ dry run.

[brettw, 2017-03-27 18:06:10]

I asked a Clank engineer who said the debugging instructions should work. he
suggested you email clank-dev and somebody can probably help you.

[Devlin, 2017-03-30 22:44:59]

extensions lgtm.

I'm still a little nervous that this is an excellent footgun, since items can be
invalidated as the list is modified.  Is there a timeline/roadmap for all the
stages of this revamp?

https://codereview.chromium.org/2740143002/diff/220001/chrome/browser/extensi...
File chrome/browser/extensions/api/content_settings/content_settings_store.cc
(right):

https://codereview.chromium.org/2740143002/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/api/content_settings/content_settings_store.cc:297:
const base::DictionaryValue* dict;
nit: init to nullptr

https://codereview.chromium.org/2740143002/diff/220001/chrome/browser/extensi...
File chrome/browser/extensions/menu_manager.cc (right):

https://codereview.chromium.org/2740143002/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/menu_manager.cc:667: std::unique_ptr<base::ListValue>
args(new base::ListValue());
nit: prefer base::MakeUnique<>.

https://codereview.chromium.org/2740143002/diff/220001/extensions/browser/api...
File extensions/browser/api/networking_private/networking_private_linux.cc
(right):

https://codereview.chromium.org/2740143002/diff/220001/extensions/browser/api...
extensions/browser/api/networking_private/networking_private_linux.cc:645: const
base::DictionaryValue* dict;
nit: init to nullptr

[Devlin, 2017-03-30 22:45:34]

On 2017/03/30 22:44:59, Devlin wrote:
> extensions lgtm.
> 
> I'm still a little nervous that this is an excellent footgun, since items can
be
> invalidated as the list is modified.  Is there a timeline/roadmap for all the
> stages of this revamp?

tools/json_schema_compiler also lg.

[brettw, 2017-03-30 22:55:39]

On 2017/03/30 22:44:59, Devlin wrote:
> extensions lgtm.
> 
> I'm still a little nervous that this is an excellent footgun, since items can
be
> invalidated as the list is modified.  Is there a timeline/roadmap for all the
> stages of this revamp?

No, this will take a while (I'm guessing some months).

[jdoerrie, 2017-04-07 11:51:34]

The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-07 11:51:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-07 13:34:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-07 13:34:17]

Dry run: This issue passed the CQ dry run.

[jdoerrie, 2017-04-07 13:56:17]

Next Round. The Android issue is still very puzzling to me, though. 

Here's what I have done so far:
- Reached out to clank-{dev,team}@ in the meantime, but haven't heard much back
yet. 
- Filed http://crbug.com/707751 in response to the waiting-for-debugger flag
being ignored for instrumentation tests. This makes using gdb to debug this
currently impracticable.


Using printf debugging I found out the following:
- The segfault is a use-after-free that gets hit during ListValue::Append when
this does a reallocation followed by a deep copy. This means currently noexcept
needs to removed from Value's move constructor in order to reproduce this. 
- What's even more puzzling, this crash doesn't occur when the nodes from
ModelTypeDebugInfo::MergeDataWithMetadata are ignored, see
http://crrev.com/2802193002 as an example.
- Just clearing the list of nodes and sending empty lists does trigger the
crash, however: http://crrev.com/2806493003

My current assumption is that this invalidating of internal nodes was present
before, just that it didn't resurface due to moving around unique pointers
before and not doing a deep copy. I created http://crrev.com/2804263002 to test
this assumption.

However, I am still very much looking forward to hearing other's ideas or
suggestions what could be going on here.

https://codereview.chromium.org/2740143002/diff/220001/chrome/browser/extensi...
File chrome/browser/extensions/api/content_settings/content_settings_store.cc
(right):

https://codereview.chromium.org/2740143002/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/api/content_settings/content_settings_store.cc:297:
const base::DictionaryValue* dict;
On 2017/03/30 22:44:59, Devlin wrote:
> nit: init to nullptr

Done.

https://codereview.chromium.org/2740143002/diff/220001/chrome/browser/extensi...
File chrome/browser/extensions/menu_manager.cc (right):

https://codereview.chromium.org/2740143002/diff/220001/chrome/browser/extensi...
chrome/browser/extensions/menu_manager.cc:667: std::unique_ptr<base::ListValue>
args(new base::ListValue());
On 2017/03/30 22:44:59, Devlin wrote:
> nit: prefer base::MakeUnique<>.

Done.

https://codereview.chromium.org/2740143002/diff/220001/extensions/browser/api...
File extensions/browser/api/networking_private/networking_private_linux.cc
(right):

https://codereview.chromium.org/2740143002/diff/220001/extensions/browser/api...
extensions/browser/api/networking_private/networking_private_linux.cc:645: const
base::DictionaryValue* dict;
On 2017/03/30 22:44:59, Devlin wrote:
> nit: init to nullptr

Done.

[brettw, 2017-04-10 04:53:57]

Will look tomorrow.

[skym, 2017-04-10 15:44:51]

Hey, looking back over the fetch all nodes sync code here, it doesn't actually
look thread safe... It looks to me like the ModelTypeDebugInfo::GetAllNodes
posts to the "model" thread, and never gets back to the UI thread like it's
supposed to. On Android, the only model type that should be going down this code
path (and where model thread != UI thread) is AUTOFILL. I'm curious, if you
remove
https://cs.chromium.org/chromium/src/testing/variations/fieldtrial_testing_co...
does this pass?

Sorry I didn't notice this earlier. I'll work on a CL to fix this today.

[skym, 2017-04-10 16:35:56]

On 2017/04/10 15:44:51, skym wrote:
> Hey, looking back over the fetch all nodes sync code here, it doesn't actually
> look thread safe... It looks to me like the ModelTypeDebugInfo::GetAllNodes
> posts to the "model" thread, and never gets back to the UI thread like it's
> supposed to. On Android, the only model type that should be going down this
code
> path (and where model thread != UI thread) is AUTOFILL. I'm curious, if you
> remove
>
https://cs.chromium.org/chromium/src/testing/variations/fieldtrial_testing_co...
> does this pass?
> 
> Sorry I didn't notice this earlier. I'll work on a CL to fix this today.

I've verified on desktop at least, AUTOFILL (and only AUTOFILL) is running
OnReceivedNodesForType on the wrong thread. iOS/ChromeOS will likely have more
model types on the wrong thread, but Android should be the same as desktop. I
think this is very likely the cause of your segfault. I'm sorry to have wasted
so much of your time because of our mistake :(

[skym, 2017-04-10 16:57:37]

On 2017/04/10 16:35:56, skym wrote:
> On 2017/04/10 15:44:51, skym wrote:
> > Hey, looking back over the fetch all nodes sync code here, it doesn't
actually
> > look thread safe... It looks to me like the ModelTypeDebugInfo::GetAllNodes
> > posts to the "model" thread, and never gets back to the UI thread like it's
> > supposed to. On Android, the only model type that should be going down this
> code
> > path (and where model thread != UI thread) is AUTOFILL. I'm curious, if you
> > remove
> >
>
https://cs.chromium.org/chromium/src/testing/variations/fieldtrial_testing_co...
> > does this pass?
> > 
> > Sorry I didn't notice this earlier. I'll work on a CL to fix this today.
> 
> I've verified on desktop at least, AUTOFILL (and only AUTOFILL) is running
> OnReceivedNodesForType on the wrong thread. iOS/ChromeOS will likely have more
> model types on the wrong thread, but Android should be the same as desktop. I
> think this is very likely the cause of your segfault. I'm sorry to have wasted
> so much of your time because of our mistake :(

*my mistake. Looks like I accidentally dropped the BindToCurrentThread in
https://codereview.chromium.org/2672493002/diff/80001/components/sync/driver/...
. CL to fix this posted https://codereview.chromium.org/2808113003/

[brettw, 2017-04-10 18:07:16]

Yay! I'm glad we tracked this down. LGTM

[jdoerrie, 2017-04-11 09:18:40]

The CQ bit was checked by jdoerrie@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 09:19:10]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jdoerrie, 2017-04-11 10:49:54]

Description was changed from

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

This CL is a first step to inlining base::ListValue. It is proposed to use an
std::vector<base::Value> as the underlying ListStorage. This CL implements the
change and updates the code accordingly.

BUG=646113
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

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

This CL is a first step to inlining base::ListValue. It is proposed to use an
std::vector<base::Value> as the underlying ListStorage. This CL implements the
change and updates the code accordingly.

TBR=bajones@chromium.org, dbeam@chromium.org, stevenjb@chromium.org
BUG=646113
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[jdoerrie, 2017-04-11 10:49:54]

jdoerrie@chromium.org changed reviewers:
+ bajones@chromium.org, dbeam@chromium.org, rsesek@chromium.org,
stevenjb@chromium.org

[commit-bot: I haz the power, 2017-04-11 10:50:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-11 10:50:12]

Dry run: This issue passed the CQ dry run.

[jdoerrie, 2017-04-11 10:55:27]

skym@, Thank you for looking into this again and fixing the issue, the segfault
is gone after rebasing on your changes: http://crrev.com/2806493003

rsesek@, please review:
- chrome/browser/devtools/devtools_embedder_message_dispatcher.cc
- mojo/common/values_struct_traits.h

For the rest I am using TBR, because this change is largely mechanical, the
other reviewers already gave their LGTM for the non-trivial changes and the
effort was published on chromium-dev in
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/qWyLUZklfCk...

bajones@, TBR for:
- chrome/browser/android/vr_shell/*

dbeam@, TBR for:
- chrome/browser/ui/webui/browsing_history_handler.cc
- chrome/browser/ui/webui/options/*

stevenjb@, TBR for:
- chromeos/network/*

[Robert Sesek, 2017-04-11 13:48:37]

IPC LGTM

[jdoerrie, 2017-04-11 14:08:24]

The CQ bit was checked by jdoerrie@chromium.org

[jdoerrie, 2017-04-11 14:08:26]

The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org,
rdevlin.cronin@chromium.org, skym@chromium.org, brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/2740143002/#ps260001
(title: "Rebase")

[commit-bot: I haz the power, 2017-04-11 14:08:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 14:20:25]

CQ is committing da patch.

Bot data: {"patchset_id": 260001, "attempt_start_ts": 1491919704869050,
"parent_rev": "f52866b40aca4279164a4415b74b71806dd669b8", "commit_rev":
"ebab0defaea3aed024283a64088ebc0cd352b47f"}

[commit-bot: I haz the power, 2017-04-11 14:21:25]

Description was changed from

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

This CL is a first step to inlining base::ListValue. It is proposed to use an
std::vector<base::Value> as the underlying ListStorage. This CL implements the
change and updates the code accordingly.

TBR=bajones@chromium.org, dbeam@chromium.org, stevenjb@chromium.org
BUG=646113
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

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

This CL is a first step to inlining base::ListValue. It is proposed to use an
std::vector<base::Value> as the underlying ListStorage. This CL implements the
change and updates the code accordingly.

TBR=bajones@chromium.org, dbeam@chromium.org, stevenjb@chromium.org
BUG=646113
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2740143002
Cr-Commit-Position: refs/heads/master@{#463618}
Committed:
https://chromium.googlesource.com/chromium/src/+/ebab0defaea3aed024283a64088e...
==========

[commit-bot: I haz the power, 2017-04-11 14:21:31]

Committed patchset #14 (id:260001) as
https://chromium.googlesource.com/chromium/src/+/ebab0defaea3aed024283a64088e...

[please use gerrit instead, 2017-04-11 14:52:46]

A revert of this CL (patchset #14 id:260001) has been created in
https://codereview.chromium.org/2816513002/ by rouslan@chromium.org.

The reason for reverting is: Appears to have broken compile on ChromeOS. Needs a
rebase?

FAILED:
obj/extensions/browser/api/networking_private/networking_private/networking_private_chromeos.o
In file included from
../../extensions/browser/api/networking_private/networking_private_chromeos.cc:21:
In file included from ../../chromeos/network/network_connection_handler.h:18:
In file included from ../../chromeos/cert_loader.h:18:
In file included from ../../net/cert/cert_database.h:13:
../../net/cert/x509_certificate.h:22:10: fatal error: 'net/net_features.h' file
not found
#include "net/net_features.h"
         ^~~~~~~~~~~~~~~~~~~~
1 error generated.

https://build.chromium.org/p/chromium.chrome/builders/Google%20Chrome%20Chrom....