Fix a use-after-free of SaveFileCreateInfo in SaveFileManager.

Fix a use-after-free of SaveFileCreateInfo in SaveFileManager.

For details about the problem see crbug.com/569258.
This CL fixes the problem, by avoiding to post a raw pointer
and instead posting the SaveFileCreateInfo struct by value.
Posting by value is ok, because the target (UI thread) doesn't
modify SaveFileCreateInfo (and I tried to make that explicit
by retaining the "const" qualifier on the parameter declaration).

BUG=569258
Committed: https://crrev.com/5669529aa87b0d38985b592ec08affed7dc6cfdb
Cr-Commit-Position: refs/heads/master@{#365053}

[Łukasz Anforowicz, 2015-12-12 01:39:54]

lukasza@chromium.org changed reviewers:
+ rdsmith@chromium.org

[Łukasz Anforowicz, 2015-12-12 01:39:54]

Randy, could you please take a look?

[Randy Smith (Not in Mondays), 2015-12-12 20:32:11]

Nice catch; thanks.  LGTM.

I'll note that it's frustrating reading the code without the call path including
scoped_ptrs to indicate ownership transfer.  I mention it in the hopes you feel
inspired to add them :-}, but you're welcome to land without that.

https://codereview.chromium.org/1519283002/diff/1/content/browser/download/sa...
File content/browser/download/save_file_manager.cc (right):

https://codereview.chromium.org/1519283002/diff/1/content/browser/download/sa...
content/browser/download/save_file_manager.cc:223: void
SaveFileManager::OnStartSave(const SaveFileCreateInfo info) {
I'd make this a const&.  I believe the Bind & PostTask stuff handles that
properly and that it'll reduce copies.  And it's consistent with the rest of the
code base.

[Łukasz Anforowicz, 2015-12-14 17:40:12]

Thanks for reviewing.

On 2015/12/12 20:32:11, rdsmith wrote:
> Nice catch; thanks.  LGTM.

The first suspect was my recent change that introduced save_item_id and started
using it instead of save_id (crrev.com/1484093002).  But I think that the
use-after-free race was there even before the save_item_id change.
> 
> I'll note that it's frustrating reading the code without the call path
including
> scoped_ptrs to indicate ownership transfer.  I mention it in the hopes you
feel
> inspired to add them :-}, but you're welcome to land without that.

This is a good idea - this will either make ownership clearer or uncover bugs. 
I'll add this to the ideas of what to do during the Holidays downtime.
>

https://codereview.chromium.org/1519283002/diff/1/content/browser/download/sa...
File content/browser/download/save_file_manager.cc (right):

https://codereview.chromium.org/1519283002/diff/1/content/browser/download/sa...
content/browser/download/save_file_manager.cc:223: void
SaveFileManager::OnStartSave(const SaveFileCreateInfo info) {
On 2015/12/12 20:32:11, rdsmith wrote:
> I'd make this a const&.  I believe the Bind & PostTask stuff handles that
> properly and that it'll reduce copies.  And it's consistent with the rest of
the
> code base.

You're right, const-ref reads better and is more consistent with the rest of the
code and the style guide.  And yes - it still binds to the callback by value (an
explicit base::ConstRef from bind_helpers.h is needed to bind by
reference/pointer).

Done.

[Łukasz Anforowicz, 2015-12-14 17:40:18]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2015-12-14 17:40:18]

The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/1519283002/#ps20001
(title: "Changing parameter declaration to const-ref.")

[commit-bot: I haz the power, 2015-12-14 17:41:28]

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

[commit-bot: I haz the power, 2015-12-14 18:49:27]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-12-14 18:50:14]

Description was changed from

==========
Fix a use-after-free of SaveFileCreateInfo in SaveFileManager.

For details about the problem see crbug.com/569258.
This CL fixes the problem, by avoiding to post a raw pointer
and instead posting the SaveFileCreateInfo struct by value.
Posting by value is ok, because the target (UI thread) doesn't
modify SaveFileCreateInfo (and I tried to make that explicit
by retaining the "const" qualifier on the parameter declaration).

BUG=569258
==========

to

==========
Fix a use-after-free of SaveFileCreateInfo in SaveFileManager.

For details about the problem see crbug.com/569258.
This CL fixes the problem, by avoiding to post a raw pointer
and instead posting the SaveFileCreateInfo struct by value.
Posting by value is ok, because the target (UI thread) doesn't
modify SaveFileCreateInfo (and I tried to make that explicit
by retaining the "const" qualifier on the parameter declaration).

BUG=569258

Committed: https://crrev.com/5669529aa87b0d38985b592ec08affed7dc6cfdb
Cr-Commit-Position: refs/heads/master@{#365053}
==========

[commit-bot: I haz the power, 2015-12-14 18:50:15]

Patchset 2 (id:??) landed as
https://crrev.com/5669529aa87b0d38985b592ec08affed7dc6cfdb
Cr-Commit-Position: refs/heads/master@{#365053}