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

components/browser_sync/profile_sync_service.cc

Index: components/browser_sync/profile_sync_service.cc
diff --git a/components/browser_sync/profile_sync_service.cc b/components/browser_sync/profile_sync_service.cc
index ed837f1b31b2ecfa7f70739d6a4f4de247cb9e3a..21e863c9ff9653e6b570727d5ba259558ec7e3ce 100644
--- a/components/browser_sync/profile_sync_service.cc
+++ b/components/browser_sync/profile_sync_service.cc
@@ -2111,7 +2111,9 @@ GetAllNodesRequestHelper::GetAllNodesRequestHelper(
     const base::Callback<void(std::unique_ptr<base::ListValue>)>& callback)
     : result_accumulator_(new base::ListValue()),
       awaiting_types_(requested_types),
-      callback_(callback) {}
+      callback_(callback) {
+  result_accumulator_->Reserve(awaiting_types_.Size());

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

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&&)|).

[skym, 2017/03/15 16:57:23]

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...

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

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.

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

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.

+}
 
 GetAllNodesRequestHelper::~GetAllNodesRequestHelper() {
   if (!awaiting_types_.Empty()) {