Add ability for NetLogLogger to gather data from more than just NetLog

Add ability for NetLogLogger to gather data from more than just NetLog

This includes data from other net-internals tabs and from pending
URLRequests.  This also changes the semantices of NetLogLogger so
that it takes the input file in StartObserving and closes it in
StopObserving, to make thread restrictions when using the new
capability a little simpler.

This CL also fixes a number of cases where StopObserving either wasn't
being called, or was being called incorrectly by consumers.

BUG=426918
Committed: https://crrev.com/853ed15d6d080b116bbc496de3302276f3668f1f
Cr-Commit-Position: refs/heads/master@{#320745}

[mmenke, 2015-03-03 16:52:52]

Patchset #2 (id:20001) has been deleted

[mmenke, 2015-03-03 18:21:32]

Patchset #5 (id:100001) has been deleted

[mmenke, 2015-03-03 18:21:37]

Patchset #4 (id:80001) has been deleted

[mmenke, 2015-03-03 18:27:24]

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

[mmenke, 2015-03-03 18:27:25]

Eric:  PTAL

Misha:  FYI.  This should get us information about hung/active requests, proxy
config, socket pools, DNS cache, etc.

[mmenke, 2015-03-03 18:33:58]

https://codereview.chromium.org/976483002/diff/140001/net/base/net_log_logger.h
File net/base/net_log_logger.h (right):

https://codereview.chromium.org/976483002/diff/140001/net/base/net_log_logger...
net/base/net_log_logger.h:53: net::URLRequestContext* url_request_context);
One downside of this approach is that we can't get anything other than net/
info...  So no prerender information, not DRP information (Though that's
currently pretty useless, and stored in prefs/ anyways), etc.  We also only get
hung requests information from one URLRequestContext, while net-internals
includes several.

So we go from having 2 different types of logs to three.  For most purposes the
new type will be as good as net-internals logs, but not all.

[eroman, 2015-03-05 19:25:58]

lgtm

https://codereview.chromium.org/976483002/diff/140001/net/base/net_log_logger.h
File net/base/net_log_logger.h (right):

https://codereview.chromium.org/976483002/diff/140001/net/base/net_log_logger...
net/base/net_log_logger.h:48: // StopObserving must be called on the context's
thread, while the context is
I don't like this second requirement. How about instead of storing
url_request_context_ in the NetLogger, making it a parameter to StopObserving()
?

[mmenke, 2015-03-05 22:02:57]

Patchset #6 (id:160001) has been deleted

[mmenke, 2015-03-05 22:04:17]

I also noticed that we were using NetLog::RemoveObserver instead of
NetLogLogger::StopObserving in a bunch of places, which I've fixed.

https://codereview.chromium.org/976483002/diff/140001/net/base/net_log_logger.h
File net/base/net_log_logger.h (right):

https://codereview.chromium.org/976483002/diff/140001/net/base/net_log_logger...
net/base/net_log_logger.h:48: // StopObserving must be called on the context's
thread, while the context is
On 2015/03/05 19:25:58, eroman wrote:
> I don't like this second requirement. How about instead of storing
> url_request_context_ in the NetLogger, making it a parameter to
StopObserving()
> ?

Done. May make StartObserving take a list of contexts at some point, to get
closer to feature parity with net-export.

[mmenke, 2015-03-09 14:48:12]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2015-03-09 14:48:12]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/976483002/#ps220001
(title: "Fix Cronet (Really!)")

[commit-bot: I haz the power, 2015-03-09 14:48:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/976483002/220001

[commit-bot: I haz the power, 2015-03-09 15:06:26]

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

[commit-bot: I haz the power, 2015-03-09 15:06:27]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[mmenke, 2015-03-09 15:10:46]

On 2015/03/09 15:06:27, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

Oops...need more signoffs.

[mmenke, 2015-03-09 20:10:16]

mmenke@chromium.org changed reviewers:
+ kalman@chromium.org, pfeldman@chromium.org, zea@chromium.org

[mmenke, 2015-03-09 20:10:18]

[+kalman]:  Please review chrome/browser/extensions/api/log_private/
[+pfeldman]:  Please review content/shell/browser/shell_net_log.cc.
[+zea]:  Please review google_apis/gcm/tools/mcs_probe.cc

I'm just modifying those files to use the new and improved (?) NetLogLogger
interface.  The way they are using the API should result in basically no
functional difference to any of them.

[not at google - send to devlin, 2015-03-09 21:26:25]

lgtm

https://codereview.chromium.org/976483002/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/api/log_private/log_private_api.h (right):

https://codereview.chromium.org/976483002/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/api/log_private/log_private_api.h:94: const
std::string& owner_extension_id, base::ScopedFILE* file);
FILE!!!!!!!!!

[mmenke, 2015-03-09 21:59:30]

https://codereview.chromium.org/976483002/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/api/log_private/log_private_api.h (right):

https://codereview.chromium.org/976483002/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/api/log_private/log_private_api.h:94: const
std::string& owner_extension_id, base::ScopedFILE* file);
On 2015/03/09 21:26:25, kalman wrote:
> FILE!!!!!!!!!

The problem with that is it leaks the FILE* if the task doesn't run on the io
thread for some reason.  I suppose since we're using base::Unretained, we assume
that doesn't happen, or if it does, it only happens when the IO Thread shuts
down?  I'm not that strongly opposed to switching, if you want, but it doesn't
strike me as all that safe.

[not at google - send to devlin, 2015-03-09 22:03:38]

https://codereview.chromium.org/976483002/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/api/log_private/log_private_api.h (right):

https://codereview.chromium.org/976483002/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/api/log_private/log_private_api.h:94: const
std::string& owner_extension_id, base::ScopedFILE* file);
On 2015/03/09 21:59:30, mmenke wrote:
> On 2015/03/09 21:26:25, kalman wrote:
> > FILE!!!!!!!!!
> 
> The problem with that is it leaks the FILE* if the task doesn't run on the io
> thread for some reason.  I suppose since we're using base::Unretained, we
assume
> that doesn't happen, or if it does, it only happens when the IO Thread shuts
> down?  I'm not that strongly opposed to switching, if you want, but it doesn't
> strike me as all that safe.

Oh sorry, that was supposed to be a joke. I'm using to seeing naming like
"ScopedFile" so when I see "ScopedFILE" it looks like somebody shouting "FILE"
(though I do understand the naming convention).

As for the code in question, if CreateFileForNetLogger takes ownership then it
makes sense to be Scoped, otherwise, less so - I was just considering my input
as a rubber stamp.

[mmenke, 2015-03-09 22:21:43]

https://codereview.chromium.org/976483002/diff/220001/chrome/browser/extensio...
File chrome/browser/extensions/api/log_private/log_private_api.h (right):

https://codereview.chromium.org/976483002/diff/220001/chrome/browser/extensio...
chrome/browser/extensions/api/log_private/log_private_api.h:94: const
std::string& owner_extension_id, base::ScopedFILE* file);
On 2015/03/09 22:03:38, kalman wrote:
> On 2015/03/09 21:59:30, mmenke wrote:
> > On 2015/03/09 21:26:25, kalman wrote:
> > > FILE!!!!!!!!!
> > 
> > The problem with that is it leaks the FILE* if the task doesn't run on the
io
> > thread for some reason.  I suppose since we're using base::Unretained, we
> assume
> > that doesn't happen, or if it does, it only happens when the IO Thread shuts
> > down?  I'm not that strongly opposed to switching, if you want, but it
doesn't
> > strike me as all that safe.
> 
> Oh sorry, that was supposed to be a joke. I'm using to seeing naming like
> "ScopedFile" so when I see "ScopedFILE" it looks like somebody shouting "FILE"
> (though I do understand the naming convention).
> 
> As for the code in question, if CreateFileForNetLogger takes ownership then it
> makes sense to be Scoped, otherwise, less so - I was just considering my input
> as a rubber stamp.

Ahh, right.  I completely misunderstood.  :)  And yes, it takes ownership.

I kept on writing File everywhere when writing this CL, and shockingly, every
time I tried, I got a compile failure.  :(

[mmenke, 2015-03-11 17:05:00]

pfeldman:  Ping!
zea:  Ping!

[mmenke, 2015-03-13 16:56:58]

mmenke@chromium.org changed reviewers:
+ mkwst@chromium.org, tim@chromium.org

[mmenke, 2015-03-13 16:56:59]

[+tim]:  Please review google_apis/gcm/tools/mcs_probe.cc.
[+mkwst]:  Please review content/shell/browser/shell_net_log.cc.

[Nicolas Zea, 2015-03-13 16:59:04]

gcm LGTM

[pfeldman, 2015-03-16 13:36:16]

shell lgtm

[mmenke, 2015-03-16 16:05:04]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2015-03-16 16:05:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/976483002/220001

[commit-bot: I haz the power, 2015-03-16 16:08:32]

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

[commit-bot: I haz the power, 2015-03-16 16:08:33]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[mmenke, 2015-03-16 16:41:22]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2015-03-16 16:41:22]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org,
pfeldman@chromium.org, zea@chromium.org, kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/976483002/#ps240001
(title: "Merge")

[commit-bot: I haz the power, 2015-03-16 16:44:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/976483002/240001

[commit-bot: I haz the power, 2015-03-16 17:38:56]

Committed patchset #9 (id:240001)

[commit-bot: I haz the power, 2015-03-16 17:39:22]

Patchset 9 (id:??) landed as
https://crrev.com/853ed15d6d080b116bbc496de3302276f3668f1f
Cr-Commit-Position: refs/heads/master@{#320745}