Make URLFetcherCore::ReleaseRequest() release |buffer_|.

Make URLFetcherCore::ReleaseRequest() release |buffer_|.

Consumer might hold on to URLFetcher while net::URLRequest is
gone. URLFetcherCore::|buffer_| consume 4KiB each if not
deallocated. This CL makes ReleaseRequest() null out |buffer_|
too and moves the initialization of |buffer_| to
OnResponseStart() because URLFetcherCore can retry on 5xx and
on network change.

R=mmenke@chromium.org

BUG=685205
Review-Url: https://codereview.chromium.org/2653183006
Cr-Commit-Position: refs/heads/master@{#446078}
Committed: https://chromium.googlesource.com/chromium/src/+/3c5b7638f0662fb0a8fc1e3ef1ec556232dee0b6

[xunjieli, 2017-01-25 16:09:32]

Description was changed from

==========
Make URLFetcherCore::ReleaseRequest() release |buffer_|.

BUG=685205
==========

to

==========
Make URLFetcherCore::ReleaseRequest() release |buffer_|.

Consumer might hold on to URLFetcher while net::URLRequest is
gone. URLFetcherCore::|buffer_| consume 4KiB each if not
deallocated. This CL makes ReleaseRequest() null out |buffer_|
too.

BUG=685205
==========

[xunjieli, 2017-01-25 16:09:32]

xunjieli@chromium.org changed reviewers:
+ mmenke@chromium.org

[xunjieli, 2017-01-25 16:09:51]

Matt: could you take a look? Thanks.

[xunjieli, 2017-01-25 16:09:58]

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

[commit-bot: I haz the power, 2017-01-25 16:10:21]

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

[mmenke, 2017-01-25 16:12:30]

https://codereview.chromium.org/2653183006/diff/1/net/url_request/url_fetcher...
File net/url_request/url_fetcher_core.cc (right):

https://codereview.chromium.org/2653183006/diff/1/net/url_request/url_fetcher...
net/url_request/url_fetcher_core.cc:819: buffer_ = nullptr;
BUG:  In some cases, URLFetcherCores retry requests.  They create the buffer in
the constructor, so this will result in crashes.

[xunjieli, 2017-01-25 16:28:16]

Thanks! PTAL

https://codereview.chromium.org/2653183006/diff/1/net/url_request/url_fetcher...
File net/url_request/url_fetcher_core.cc (right):

https://codereview.chromium.org/2653183006/diff/1/net/url_request/url_fetcher...
net/url_request/url_fetcher_core.cc:819: buffer_ = nullptr;
On 2017/01/25 16:12:30, mmenke wrote:
> BUG:  In some cases, URLFetcherCores retry requests.  They create the buffer
in
> the constructor, so this will result in crashes.

Done. Thanks! I forgot about the retry on 5xx and network change.
I changed to initialize |buffer_| in ReadResponse(). If the retry scenario is
infrequent, then lazily initializing it is probably okay?

[mmenke, 2017-01-25 16:32:16]

https://codereview.chromium.org/2653183006/diff/20001/net/url_request/url_fet...
File net/url_request/url_fetcher_core.cc (right):

https://codereview.chromium.org/2653183006/diff/20001/net/url_request/url_fet...
net/url_request/url_fetcher_core.cc:894: buffer_ = new IOBuffer(kBufferSize);
Just do this unconditionally in OnResponseStarted, where we know it's nullptr
(Head requests are sufficiently rare that I don't think we care about them, and
we'll clean it up immediately in OnReadCompleted, anyways - actually, I think
the HEAD check would make more sense in there, too).

[xunjieli, 2017-01-25 16:41:54]

Thanks, PTAL.

https://codereview.chromium.org/2653183006/diff/20001/net/url_request/url_fet...
File net/url_request/url_fetcher_core.cc (right):

https://codereview.chromium.org/2653183006/diff/20001/net/url_request/url_fet...
net/url_request/url_fetcher_core.cc:894: buffer_ = new IOBuffer(kBufferSize);
On 2017/01/25 16:32:16, mmenke wrote:
> Just do this unconditionally in OnResponseStarted, where we know it's nullptr
> (Head requests are sufficiently rare that I don't think we care about them,
and
> we'll clean it up immediately in OnReadCompleted, anyways - actually, I think
> the HEAD check would make more sense in there, too).

Done.

[mmenke, 2017-01-25 16:42:24]

On 2017/01/25 16:41:54, xunjieli wrote:
> Thanks, PTAL.
> 
>
https://codereview.chromium.org/2653183006/diff/20001/net/url_request/url_fet...
> File net/url_request/url_fetcher_core.cc (right):
> 
>
https://codereview.chromium.org/2653183006/diff/20001/net/url_request/url_fet...
> net/url_request/url_fetcher_core.cc:894: buffer_ = new IOBuffer(kBufferSize);
> On 2017/01/25 16:32:16, mmenke wrote:
> > Just do this unconditionally in OnResponseStarted, where we know it's
nullptr
> > (Head requests are sufficiently rare that I don't think we care about them,
> and
> > we'll clean it up immediately in OnReadCompleted, anyways - actually, I
think
> > the HEAD check would make more sense in there, too).
> 
> Done.

LGTM

[xunjieli, 2017-01-25 16:45:06]

Description was changed from

==========
Make URLFetcherCore::ReleaseRequest() release |buffer_|.

Consumer might hold on to URLFetcher while net::URLRequest is
gone. URLFetcherCore::|buffer_| consume 4KiB each if not
deallocated. This CL makes ReleaseRequest() null out |buffer_|
too.

BUG=685205
==========

to

==========
Make URLFetcherCore::ReleaseRequest() release |buffer_|.

Consumer might hold on to URLFetcher while net::URLRequest is
gone. URLFetcherCore::|buffer_| consume 4KiB each if not
deallocated. This CL makes ReleaseRequest() null out |buffer_|
too and moves the initialization of |buffer_| to
OnResponseStart() because URLFetcherCore can retry on 5xx and
on network change.

R=mmenke@chromium.org

BUG=685205
==========

[xunjieli, 2017-01-25 16:45:11]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2017-01-25 16:45:31]

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-01-25 18:13:30]

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

[commit-bot: I haz the power, 2017-01-25 18:13:31]

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

[xunjieli, 2017-01-25 18:27:37]

The CQ bit was checked by xunjieli@chromium.org

[commit-bot: I haz the power, 2017-01-25 18:28:06]

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-01-25 19:13:38]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485368857283030,
"parent_rev": "8e3c814a822afe01b3893d48cb0dd7cb4e132b74", "commit_rev":
"3c5b7638f0662fb0a8fc1e3ef1ec556232dee0b6"}

[commit-bot: I haz the power, 2017-01-25 19:14:12]

Description was changed from

==========
Make URLFetcherCore::ReleaseRequest() release |buffer_|.

Consumer might hold on to URLFetcher while net::URLRequest is
gone. URLFetcherCore::|buffer_| consume 4KiB each if not
deallocated. This CL makes ReleaseRequest() null out |buffer_|
too and moves the initialization of |buffer_| to
OnResponseStart() because URLFetcherCore can retry on 5xx and
on network change.

R=mmenke@chromium.org

BUG=685205
==========

to

==========
Make URLFetcherCore::ReleaseRequest() release |buffer_|.

Consumer might hold on to URLFetcher while net::URLRequest is
gone. URLFetcherCore::|buffer_| consume 4KiB each if not
deallocated. This CL makes ReleaseRequest() null out |buffer_|
too and moves the initialization of |buffer_| to
OnResponseStart() because URLFetcherCore can retry on 5xx and
on network change.

R=mmenke@chromium.org

BUG=685205

Review-Url: https://codereview.chromium.org/2653183006
Cr-Commit-Position: refs/heads/master@{#446078}
Committed:
https://chromium.googlesource.com/chromium/src/+/3c5b7638f0662fb0a8fc1e3ef1ec...
==========

[commit-bot: I haz the power, 2017-01-25 19:14:15]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/3c5b7638f0662fb0a8fc1e3ef1ec...