Fix thread safety issues with //components/upload_list.

Fix thread safety issues with //components/upload_list.

The uploads_ vector can be accessed simultaneously from multiple threads,
which can lead to heap corruption. This change ensures access to uploads_
only on the creator task runner. Updates occur by currying the new data
vector in a Closure across threads.

BUG=597384
Committed: https://crrev.com/6221751958a5f073b9557cfb89ba7a4197a5369d
Cr-Commit-Position: refs/heads/master@{#383495}

[Robert Sesek, 2016-03-24 23:48:58]

Patchset #1 (id:1) has been deleted

[Robert Sesek, 2016-03-25 03:38:05]

Patchset #2 (id:40001) has been deleted

[Robert Sesek, 2016-03-25 03:41:49]

Patchset #2 (id:60001) has been deleted

[Robert Sesek, 2016-03-25 03:53:54]

Description was changed from

==========
Fix thread safety issues with //components/upload_list.

If the UploadList is loaded while it is simultaneously being retrieved,
heap corruption can occur. This happens in WebUI where the initial load
is not complete and another load is requested.

BUG=597384
==========

to

==========
Fix thread safety issues with //components/upload_list.

The uploads_ vector can be accessed simultaneously from multiple threads,
which can lead to heap corruption. This change ensures access to uploads_
only on the creator task runner. Updates occur by currying the new data
vector in a Closure across threads.

BUG=597384
==========

[Robert Sesek, 2016-03-25 16:32:41]

rsesek@chromium.org changed reviewers:
+ mark@chromium.org

[Mark Mentovai, 2016-03-25 18:26:56]

LGTM. Nice work.

https://codereview.chromium.org/1830383002/diff/100001/components/upload_list...
File components/upload_list/upload_list.cc (right):

https://codereview.chromium.org/1830383002/diff/100001/components/upload_list...
components/upload_list/upload_list.cc:64: std::move(uploads)));
#include <utility>

https://codereview.chromium.org/1830383002/diff/100001/components/upload_list...
components/upload_list/upload_list.cc:119: void UploadList::GetUploads(size_t
max_count,
This max_count thing seems ill-advised, certainly with the Crashpad
implementation, but whatever.

https://codereview.chromium.org/1830383002/diff/100001/components/upload_list...
components/upload_list/upload_list.cc:123: uploads_.begin() +
std::min<size_t>(uploads_.size(), max_count),
The explicit <size_t> is no longer necessary!

https://codereview.chromium.org/1830383002/diff/100001/components/upload_list...
File components/upload_list/upload_list.h (right):

https://codereview.chromium.org/1830383002/diff/100001/components/upload_list...
components/upload_list/upload_list.h:77: void GetUploads(size_t max_count,
std::vector<UploadInfo>* uploads);
#include <stddef.h> here now (and remove it from upload_list.cc).

https://codereview.chromium.org/1830383002/diff/100001/components/upload_list...
File components/upload_list/upload_list_unittest.cc (right):

https://codereview.chromium.org/1830383002/diff/100001/components/upload_list...
components/upload_list/upload_list_unittest.cc:84:
upload_list->LoadUploadListAsynchronously();
This test is much better now. Thanks!

[Robert Sesek, 2016-03-25 18:45:56]

https://codereview.chromium.org/1830383002/diff/100001/components/upload_list...
File components/upload_list/upload_list.cc (right):

https://codereview.chromium.org/1830383002/diff/100001/components/upload_list...
components/upload_list/upload_list.cc:64: std::move(uploads)));
On 2016/03/25 18:26:56, Mark Mentovai wrote:
> #include <utility>

Done.

https://codereview.chromium.org/1830383002/diff/100001/components/upload_list...
components/upload_list/upload_list.cc:119: void UploadList::GetUploads(size_t
max_count,
On 2016/03/25 18:26:56, Mark Mentovai wrote:
> This max_count thing seems ill-advised, certainly with the Crashpad
> implementation, but whatever.

Right. There's some more cleanup I'd like to do here, but this is a good first
cut.

https://codereview.chromium.org/1830383002/diff/100001/components/upload_list...
components/upload_list/upload_list.cc:123: uploads_.begin() +
std::min<size_t>(uploads_.size(), max_count),
On 2016/03/25 18:26:56, Mark Mentovai wrote:
> The explicit <size_t> is no longer necessary!

Done.

https://codereview.chromium.org/1830383002/diff/100001/components/upload_list...
File components/upload_list/upload_list.h (right):

https://codereview.chromium.org/1830383002/diff/100001/components/upload_list...
components/upload_list/upload_list.h:77: void GetUploads(size_t max_count,
std::vector<UploadInfo>* uploads);
On 2016/03/25 18:26:56, Mark Mentovai wrote:
> #include <stddef.h> here now (and remove it from upload_list.cc).

Done.

[Mark Mentovai, 2016-03-25 18:48:40]

LGTM

P.S. Thanks for giving the methods more rational names too.

[Robert Sesek, 2016-03-25 21:30:31]

rsesek@chromium.org changed reviewers:
+ jochen@chromium.org

[Robert Sesek, 2016-03-25 21:30:32]

+jochen for OWNERS

[Robert Sesek, 2016-03-28 14:52:15]

rsesek@chromium.org changed reviewers:
+ thakis@chromium.org
- jochen@chromium.org

[Robert Sesek, 2016-03-28 14:52:16]

Since jochen is slow, +thakis for //chrome OWNERS

[Nico, 2016-03-28 14:53:42]

INSTANT lgtm for chrome/

[Robert Sesek, 2016-03-28 15:01:54]

The CQ bit was checked by rsesek@chromium.org

[commit-bot: I haz the power, 2016-03-28 15:02:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1830383002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1830383002/120001

[commit-bot: I haz the power, 2016-03-28 15:41:51]

Committed patchset #4 (id:120001)

[commit-bot: I haz the power, 2016-03-28 15:43:28]

Description was changed from

==========
Fix thread safety issues with //components/upload_list.

The uploads_ vector can be accessed simultaneously from multiple threads,
which can lead to heap corruption. This change ensures access to uploads_
only on the creator task runner. Updates occur by currying the new data
vector in a Closure across threads.

BUG=597384
==========

to

==========
Fix thread safety issues with //components/upload_list.

The uploads_ vector can be accessed simultaneously from multiple threads,
which can lead to heap corruption. This change ensures access to uploads_
only on the creator task runner. Updates occur by currying the new data
vector in a Closure across threads.

BUG=597384

Committed: https://crrev.com/6221751958a5f073b9557cfb89ba7a4197a5369d
Cr-Commit-Position: refs/heads/master@{#383495}
==========

[commit-bot: I haz the power, 2016-03-28 15:43:30]

Patchset 4 (id:??) landed as
https://crrev.com/6221751958a5f073b9557cfb89ba7a4197a5369d
Cr-Commit-Position: refs/heads/master@{#383495}