Remove base::ListValue::Set(size_t, base::Value*)

Remove base::ListValue::Set(size_t, base::Value*)

This change removes the raw pointer version of base::ListValue::Set,
replacing it with the unique_ptr version or base::ListValue::Append*
where appropriate.

BUG=646113
Review-Url: https://codereview.chromium.org/2838893002
Cr-Commit-Position: refs/heads/master@{#467340}
Committed: https://chromium.googlesource.com/chromium/src/+/f1e72e36770eb7fc01f6b19b58ff645b7ae0368a

[jdoerrie, 2017-04-25 14:05:17]

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

[commit-bot: I haz the power, 2017-04-25 14:05:26]

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-25 14:08:12]

jdoerrie@chromium.org changed reviewers:
+ brettw@chromium.org, rsesek@chromium.org, vabr@chromium.org

[jdoerrie, 2017-04-25 14:08:12]

brettw@chromium.org: Please review changes in //base

rsesek@chromium.org: Please review changes in
//content/common/common_param_traits_unittest.cc

vabr@chromium.org: Please review the CL in general.

[Robert Sesek, 2017-04-25 14:11:21]

LGTM

[commit-bot: I haz the power, 2017-04-25 14:19:02]

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

[commit-bot: I haz the power, 2017-04-25 14:19:03]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[jdoerrie, 2017-04-25 14:28:26]

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

[commit-bot: I haz the power, 2017-04-25 14:28:42]

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-25 15:01:23]

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

[commit-bot: I haz the power, 2017-04-25 15:01:24]

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-04-25 15:45:13]

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

[commit-bot: I haz the power, 2017-04-25 15:45:33]

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-25 17:11:49]

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

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

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

[vabr (Chromium), 2017-04-25 17:13:16]

Hi Jan,

My comments are mostly optional, with the exception of the one in fuzzer.cc --
there I'd like to ensure we know what's happening.

On the condition that that's clarified, the CL LGTM. Good to see another
dangerous part of the API gone!

Vaclav

https://codereview.chromium.org/2838893002/diff/60001/chrome/test/chromedrive...
File chrome/test/chromedriver/chrome/web_view_impl.cc (right):

https://codereview.chromium.org/2838893002/diff/60001/chrome/test/chromedrive...
chrome/test/chromedriver/chrome/web_view_impl.cc:374:
std::unique_ptr<base::ListValue> point_list(new base::ListValue);
optional: Move point_list definition immediately prior to its first usage.

https://codereview.chromium.org/2838893002/diff/60001/content/common/common_p...
File content/common/common_param_traits_unittest.cc (right):

https://codereview.chromium.org/2838893002/diff/60001/content/common/common_p...
content/common/common_param_traits_unittest.cc:125: subdict->Set("list",
sublist.release());
optional: While here, you might as well change the Set calls to get unique_ptr
arguments. While arguably this is not the main focus of the CL, it is in its
spirit, and it will avoid another CL touching this file soon.

https://codereview.chromium.org/2838893002/diff/60001/ipc/ipc_message_utils.cc
File ipc/ipc_message_utils.cc (right):

https://codereview.chromium.org/2838893002/diff/60001/ipc/ipc_message_utils.c...
ipc/ipc_message_utils.cc:243: if (!ReadValue(m, iter, &subval, recursion + 1))
I suggest fixing ReadValue to take a pointer to unique_ptr<base::Value> instead.
It only has two callsites. By doing that you can also fix the
SetWithoutPathExpansion at the other callsite.

https://codereview.chromium.org/2838893002/diff/60001/tools/ipc_fuzzer/fuzzer...
File tools/ipc_fuzzer/fuzzer/fuzzer.cc (right):

https://codereview.chromium.org/2838893002/diff/60001/tools/ipc_fuzzer/fuzzer...
tools/ipc_fuzzer/fuzzer/fuzzer.cc:547: p->GetDictionary(index, &tmp);
This seems wrong: GetDictionary fills |tmp| with a pointer to a DictionaryValue
owned by |p|. Note that the target of |tmp| after line 547 was not allocated
through new, it is part of a vector. Then line 549 wraps |tmp| in a unique_ptr,
which should attempt to destroy it after line 549 completes. And the value
assigned on line 546 is never deleted.

I'm probably wrong, because the sanitizers would have to found this already. But
if I'm correct, then the same issue is also below for the LIST case.

[brettw, 2017-04-25 19:55:45]

lgtm

https://codereview.chromium.org/2838893002/diff/60001/tools/ipc_fuzzer/fuzzer...
File tools/ipc_fuzzer/fuzzer/fuzzer.cc (right):

https://codereview.chromium.org/2838893002/diff/60001/tools/ipc_fuzzer/fuzzer...
tools/ipc_fuzzer/fuzzer/fuzzer.cc:547: p->GetDictionary(index, &tmp);
This looks wrong to me too.

[jdoerrie, 2017-04-26 09:03:20]

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

[commit-bot: I haz the power, 2017-04-26 09:03:28]

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-26 09:13:19]

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

[commit-bot: I haz the power, 2017-04-26 09:13:20]

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...)
  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-04-26 09:32:15]

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

[commit-bot: I haz the power, 2017-04-26 09:32:31]

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-26 11:30:59]

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

[commit-bot: I haz the power, 2017-04-26 11:31:00]

Dry run: This issue passed the CQ dry run.

[jdoerrie, 2017-04-26 11:34:36]

jdoerrie@chromium.org changed reviewers:
+ mkwst@chromium.org

[jdoerrie, 2017-04-26 11:34:37]

Hi Mike, do you mind reviewing the Patch Set 2 to Patch Set 6 delta for
common_param_traits_unittest.cc
(https://codereview.chromium.org/2838893002/diff2/20001:100001/content/common/...

Robert already gave his LGTM for an earlier version, but I changed the file
afterwards to address Vaclav's comments.

https://codereview.chromium.org/2838893002/diff/60001/chrome/test/chromedrive...
File chrome/test/chromedriver/chrome/web_view_impl.cc (right):

https://codereview.chromium.org/2838893002/diff/60001/chrome/test/chromedrive...
chrome/test/chromedriver/chrome/web_view_impl.cc:374:
std::unique_ptr<base::ListValue> point_list(new base::ListValue);
On 2017/04/25 17:13:15, vabr (Chromium) wrote:
> optional: Move point_list definition immediately prior to its first usage.

Done.

https://codereview.chromium.org/2838893002/diff/60001/content/common/common_p...
File content/common/common_param_traits_unittest.cc (right):

https://codereview.chromium.org/2838893002/diff/60001/content/common/common_p...
content/common/common_param_traits_unittest.cc:125: subdict->Set("list",
sublist.release());
On 2017/04/25 17:13:15, vabr (Chromium) wrote:
> optional: While here, you might as well change the Set calls to get unique_ptr
> arguments. While arguably this is not the main focus of the CL, it is in its
> spirit, and it will avoid another CL touching this file soon.

Done.

https://codereview.chromium.org/2838893002/diff/60001/ipc/ipc_message_utils.cc
File ipc/ipc_message_utils.cc (right):

https://codereview.chromium.org/2838893002/diff/60001/ipc/ipc_message_utils.c...
ipc/ipc_message_utils.cc:243: if (!ReadValue(m, iter, &subval, recursion + 1))
On 2017/04/25 17:13:15, vabr (Chromium) wrote:
> I suggest fixing ReadValue to take a pointer to unique_ptr<base::Value>
instead.
> It only has two callsites. By doing that you can also fix the
> SetWithoutPathExpansion at the other callsite.

Done.

https://codereview.chromium.org/2838893002/diff/60001/tools/ipc_fuzzer/fuzzer...
File tools/ipc_fuzzer/fuzzer/fuzzer.cc (right):

https://codereview.chromium.org/2838893002/diff/60001/tools/ipc_fuzzer/fuzzer...
tools/ipc_fuzzer/fuzzer/fuzzer.cc:547: p->GetDictionary(index, &tmp);
On 2017/04/25 19:55:45, brettw (plz ping after 24h) wrote:
> This looks wrong to me too.

I agree, looking at this more closely this seems wrong. This code would be fine
if line 547 didn't exist or there is no dictionary at p[index], in which case
GetDictionary is a no-op. However, if p[index] contains a dictionary already,
this leaks the memory allocated in line 546. Furthermore, line 549 is then a
self move-assignment, which in the current implementation clears the contents of
the Value, rendering the Fuzzing in line 548 useless. Lastly, when |Set|
concludes, the passed in unique ptr will attempt to destruct p[index], while p
is still alive. This should result in all sort of errors. Given that this was
never caught by the sanitizers, it is possible that p[index] never contained a
previously existing dictionary.

I changed it now for both the Dict and the List, let me know what you think.

[vabr (Chromium), 2017-04-26 12:51:46]

Still LGTM.
Thanks!
Vaclav

[Mike West, 2017-04-26 15:23:46]

LGTM.

[jdoerrie, 2017-04-26 16:17:45]

The CQ bit was checked by jdoerrie@chromium.org

[jdoerrie, 2017-04-26 16:17:45]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/2838893002/#ps100001
(title: "Fix Compilation Error")

[commit-bot: I haz the power, 2017-04-26 16:18:15]

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-26 16:23:59]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1493223465026310,
"parent_rev": "a90291ca648d7aca95315f697b49a9758a3dd054", "commit_rev":
"f1e72e36770eb7fc01f6b19b58ff645b7ae0368a"}

[commit-bot: I haz the power, 2017-04-26 16:24:11]

Description was changed from

==========
Remove base::ListValue::Set(size_t, base::Value*)

This change removes the raw pointer version of base::ListValue::Set,
replacing it with the unique_ptr version or base::ListValue::Append*
where appropriate.

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

to

==========
Remove base::ListValue::Set(size_t, base::Value*)

This change removes the raw pointer version of base::ListValue::Set,
replacing it with the unique_ptr version or base::ListValue::Append*
where appropriate.

BUG=646113

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

[commit-bot: I haz the power, 2017-04-26 16:24:14]

Committed patchset #6 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/f1e72e36770eb7fc01f6b19b58ff...