Move destruction of WebRtcLogUploader to post threads teardown.

Move destruction of WebRtcLogUploader to post threads teardown.

Needs to outlive the file thread. Fixes rare crash.

Previous attempt (https://codereview.chromium.org/174493003) caused an inconsistent leak (https://code.google.com/p/chromium/issues/detail?id=348928).

This CL makes sure pending URLFetchers are deleted before threads teardown and removes the reference to system request context in WebRtcLoggingHandlerHost.

NOTRY=true

BUG=344892
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=260167

[Ami GONE FROM CHROMIUM, 2014-03-25 17:53:17]

Can this be tested?

https://codereview.chromium.org/211033006/diff/30001/chrome/browser/media/web...
File chrome/browser/media/webrtc_log_uploader.cc (right):

https://codereview.chromium.org/211033006/diff/30001/chrome/browser/media/web...
chrome/browser/media/webrtc_log_uploader.cc:45:
WebRtcLogUploader::~WebRtcLogUploader() {
DCHECK the vector is empty.

https://codereview.chromium.org/211033006/diff/30001/chrome/browser/media/web...
chrome/browser/media/webrtc_log_uploader.cc:70: ++it) {
It's a vector, so you could
for (size_t i = 0; i < url_fetchers_.size(); ++i)
and save a couple of lines.

https://codereview.chromium.org/211033006/diff/30001/chrome/browser/media/web...
chrome/browser/media/webrtc_log_uploader.cc:73: found_in_vector = true;
can drop found_in_vector+break in favor of a return, and then LOG+delete
unconditionally outside the loop.

https://codereview.chromium.org/211033006/diff/30001/chrome/browser/media/web...
chrome/browser/media/webrtc_log_uploader.cc:78: DLOG(WARNING) << "URLFetcher not
found in vector.";
Can this happen if there is a fetch in flight while Cancel is called?  If so,
seems unworthy of a warning.
If this reflects a programming error then it should be a DCHECK (and drop the
explicit delete).

[Henrik Grunell, 2014-03-26 06:59:02]

I think it's somewhat complicated to test, we don't want to actually start
URLFetchers with the real URL etc. Needs some refactoring/rewriting.

The crash being fixed is racy, so I don't see that it can be tested.

There's a bug for getting rid of |post_data_| used for testing. I could make it
a general bug for improving the testing for this class, including this case.
Sounds good?

https://codereview.chromium.org/211033006/diff/30001/chrome/browser/media/web...
File chrome/browser/media/webrtc_log_uploader.cc (right):

https://codereview.chromium.org/211033006/diff/30001/chrome/browser/media/web...
chrome/browser/media/webrtc_log_uploader.cc:45:
WebRtcLogUploader::~WebRtcLogUploader() {
On 2014/03/25 17:53:17, Ami Fischman wrote:
> DCHECK the vector is empty.

Done.

https://codereview.chromium.org/211033006/diff/30001/chrome/browser/media/web...
chrome/browser/media/webrtc_log_uploader.cc:70: ++it) {
On 2014/03/25 17:53:17, Ami Fischman wrote:
> It's a vector, so you could
> for (size_t i = 0; i < url_fetchers_.size(); ++i)
> and save a couple of lines.

Done.

https://codereview.chromium.org/211033006/diff/30001/chrome/browser/media/web...
chrome/browser/media/webrtc_log_uploader.cc:73: found_in_vector = true;
On 2014/03/25 17:53:17, Ami Fischman wrote:
> can drop found_in_vector+break in favor of a return, and then LOG+delete
> unconditionally outside the loop.

Done.

https://codereview.chromium.org/211033006/diff/30001/chrome/browser/media/web...
chrome/browser/media/webrtc_log_uploader.cc:78: DLOG(WARNING) << "URLFetcher not
found in vector.";
On 2014/03/25 17:53:17, Ami Fischman wrote:
> Can this happen if there is a fetch in flight while Cancel is called?  If so,
> seems unworthy of a warning.
> If this reflects a programming error then it should be a DCHECK (and drop the
> explicit delete).

Right. This should really not happen if cancelled. Changed it to NOTREACHED.

[Henrik Grunell, 2014-03-26 07:41:34]

Jochen, please review chrome/browser/browser_process_impl.cc.

[jochen (gone - plz use gerrit), 2014-03-26 08:25:17]

chrome/browser/browser_process_impl.cc lgtm

[Ami GONE FROM CHROMIUM, 2014-03-26 17:53:57]

FWIW this should testable using FakeURLFetcherFactory and
ScopedURLFetcherFactory.

LGTM if you're in a hurry to land this and want to follow up with the test in a
separate CL.

[Henrik Grunell, 2014-03-27 09:50:31]

On 2014/03/26 17:53:57, Ami Fischman wrote:
> FWIW this should testable using FakeURLFetcherFactory and
> ScopedURLFetcherFactory.

OK, thanks.

> LGTM if you're in a hurry to land this and want to follow up with the test in
a
> separate CL.

Added this task to https://code.google.com/p/chromium/issues/detail?id=356076

[Henrik Grunell, 2014-03-27 09:55:52]

The CQ bit was checked by grunell@chromium.org

[commit-bot: I haz the power, 2014-03-27 09:56:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grunell@chromium.org/211033006/160001

[commit-bot: I haz the power, 2014-03-27 10:59:20]

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

[commit-bot: I haz the power, 2014-03-27 10:59:21]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_rel

[Henrik Grunell, 2014-03-27 13:38:04]

Ami, there's a compile issue on non-Windows non-clang (afaict) with a vector of
scoped_ptr. I think it's a case that scoped_ptr doesn't support. See e.g.
http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...
and
http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...

I've changed the implementation so that the upload_done_data_ is used instead
for deleting fetchers. It already contains pointers to the fetchers.

I also added a shutting_down_ flag so that no new fetchers are created. And
changed the Cancel...() function to StartShutdown().

PTAL.

[Ami GONE FROM CHROMIUM, 2014-03-27 16:57:18]

I was wondering how the scoped_ptr container was working for you but assumed
technology had advanced while I wasn't looking ;)

LGTM

[Henrik Grunell, 2014-03-28 08:33:05]

The CQ bit was checked by grunell@chromium.org

[commit-bot: I haz the power, 2014-03-28 08:33:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grunell@chromium.org/211033006/190001

[commit-bot: I haz the power, 2014-03-28 13:24:57]

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

[commit-bot: I haz the power, 2014-03-28 13:24:58]

Retried try job too often on ios_dbg_simulator for step(s) components_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...

[Henrik Grunell, 2014-03-28 16:41:34]

The CQ bit was checked by grunell@chromium.org

[commit-bot: I haz the power, 2014-03-28 16:45:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/grunell@chromium.org/211033006/190001

[commit-bot: I haz the power, 2014-03-28 16:50:27]

Change committed as 260167