Fix about:flags information not showing up in chrome://net-export/ logs

Fix about:flags information not showing up in chrome://net-export/ logs

Pass the command-line and channel as dependencies to StartNetLog() rather than saving them in the ChromeNetLog constructor.

The issue is that the CommandLine singleton may be mutated after ChromeNetLog has been initialized (it gets re-written to include synthesized switches for about:flags).

BUG=711432
Review-Url: https://codereview.chromium.org/2965623002
Cr-Commit-Position: refs/heads/master@{#483754}
Committed: https://chromium.googlesource.com/chromium/src/+/30355da69277622a7f6bb2129c0435b1e1223f49

[eroman, 2017-06-29 21:39:31]

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

[commit-bot: I haz the power, 2017-06-29 21:39:56]

Dry run: 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-06-29 21:58:30]

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

[commit-bot: I haz the power, 2017-06-29 21:58:31]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[eroman, 2017-06-29 22:13:35]

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

[commit-bot: I haz the power, 2017-06-29 22:13:56]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eroman, 2017-06-29 22:16:13]

Description was changed from

==========
Have ChromeNetLog access the command line string lazily rather than
eagerly during initialization.

Previously it would take the command line string (to include in log
files) as an explicit parameter to ChromeNetLog::ChromeNetLog(). Whereas
now it accesses it through via the global
base::CommandLine::ForCurrentProcess().

This makes the dependency less explicit (yuck), but fixes a problem
where about:flags values were not being included in logs generated by
chrome://net-export/.

The problem is that the global CommandLine instance is mutated shortly
after startup to include the about:flags values. This was happening
after ChromeNetLog had been constructed (and rememembered the command
line string).

BUG=711432
==========

to

==========
Fix about:flags information not showing up in chrome://net-export/ logs

Pass the command-line and channel as dependencies to StartNetLog() rather than
saving them in the ChromeNetLog constructor.

The issue is that the CommandLine singleton may be mutated after ChromeNetLog
has been initialized (it gets re-written to include synthesized switches for
about:flags).

BUG=711432
==========

[commit-bot: I haz the power, 2017-06-29 22:37:08]

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

[commit-bot: I haz the power, 2017-06-29 22:37:09]

Dry run: Try jobs failed on following builders:
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[eroman, 2017-06-29 23:38:50]

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

[commit-bot: I haz the power, 2017-06-29 23:39:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eroman, 2017-06-29 23:39:28]

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

[eroman, 2017-06-29 23:43:41]

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

[commit-bot: I haz the power, 2017-06-29 23:43:57]

Dry run: 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-06-30 01:06:14]

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

[commit-bot: I haz the power, 2017-06-30 01:06:15]

Dry run: This issue passed the CQ dry run.

[xunjieli, 2017-06-30 13:27:08]

I am confused. "activeFieldTrialGroups" is set by GetNetConstants() which
doesn't depend on |command_line_string| or |channel_string|. This should have no
effect on how "activeFieldTrialGroups" is populated, right?

[xunjieli, 2017-06-30 15:00:15]

LGTM. I confused this with activeFieldTrialGroups. Talked to davidben@ offline.
This makes sense.

[eroman, 2017-06-30 17:53:11]

The CQ bit was checked by eroman@chromium.org

[commit-bot: I haz the power, 2017-06-30 17:53:29]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[eroman, 2017-06-30 17:58:01]

Thanks for the review!

I am happy to chat more about the problem or proposed solution too.

There are other ways to fix this, but passing the input explicitly to
StartNetLog() seemed like a good layering regardless.

[commit-bot: I haz the power, 2017-06-30 17:59:03]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1498845191717340,
"parent_rev": "62a057db405d02f0ae5f775a1b9a081ba7202e1e", "commit_rev":
"30355da69277622a7f6bb2129c0435b1e1223f49"}

[commit-bot: I haz the power, 2017-06-30 17:59:32]

Description was changed from

==========
Fix about:flags information not showing up in chrome://net-export/ logs

Pass the command-line and channel as dependencies to StartNetLog() rather than
saving them in the ChromeNetLog constructor.

The issue is that the CommandLine singleton may be mutated after ChromeNetLog
has been initialized (it gets re-written to include synthesized switches for
about:flags).

BUG=711432
==========

to

==========
Fix about:flags information not showing up in chrome://net-export/ logs

Pass the command-line and channel as dependencies to StartNetLog() rather than
saving them in the ChromeNetLog constructor.

The issue is that the CommandLine singleton may be mutated after ChromeNetLog
has been initialized (it gets re-written to include synthesized switches for
about:flags).

BUG=711432

Review-Url: https://codereview.chromium.org/2965623002
Cr-Commit-Position: refs/heads/master@{#483754}
Committed:
https://chromium.googlesource.com/chromium/src/+/30355da69277622a7f6bb2129c04...
==========

[commit-bot: I haz the power, 2017-06-30 17:59:33]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/30355da69277622a7f6bb2129c04...

[davidben, 2017-06-30 18:03:25]

davidben@chromium.org changed reviewers:
+ davidben@chromium.org

[davidben, 2017-06-30 18:03:26]

https://codereview.chromium.org/2965623002/diff/60001/components/net_log/chro...
File components/net_log/chrome_net_log.cc (right):

https://codereview.chromium.org/2965623002/diff/60001/components/net_log/chro...
components/net_log/chrome_net_log.cc:52: GetConstants(command_line_string,
channel_string));
This bit means we still won't capture about:flags if you use --log-net-log,
right? (Dunno how much we care about that.)

[eroman, 2017-06-30 18:24:37]

https://codereview.chromium.org/2965623002/diff/60001/components/net_log/chro...
File components/net_log/chrome_net_log.cc (right):

https://codereview.chromium.org/2965623002/diff/60001/components/net_log/chro...
components/net_log/chrome_net_log.cc:52: GetConstants(command_line_string,
channel_string));
On 2017/06/30 18:03:26, davidben wrote:
> This bit means we still won't capture about:flags if you use --log-net-log,
> right? (Dunno how much we care about that.)

Correct.
--log-net-log needs some more loving, before it is worth tackling this.

The way I expect this should be sequenced is:

(1) Make --log-net-log use the same code for writing logs as net-export
(https://bugs.chromium.org/p/chromium/issues/detail?id=716570). It is not
feasible to improve things when we have so many variants of log writing. At
least the net-internals version is dead now, but still some left to kill.

(2) Switch the underlying file logging mechanism to support truncation
(https://bugs.chromium.org/p/chromium/issues/detail?id=679030). Should complete
the interns work and enable the bounded logger.

(3) Further change the log format to make things not suck. #profit.
 * Choices like requiring naming all enums complicates things like
https://bugs.chromium.org/p/chromium/issues/detail?id=90674
  * Compress and/or use a binary format instead of the terrible JSON -
https://bugs.chromium.org/p/chromium/issues/detail?id=132160