New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 809137 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

Child report crash reports do not showing locally enabled experiments

Project Member Reported by jam@chromium.org, Feb 5 2018

Issue description

In investigating bug 807980, it looks like that crash only happens if a user enables a feature that's not enabled by finch through any experiment. However it doesn't appear that the crash report includes finch experiments that the user enabled locally, which makes this hard to see.

From https://bugs.chromium.org/p/chromium/issues/detail?id=807980#c17 as suggested by Alexei:

"Ok I did a local experiment on Chrome Canary: I enabled network service & network service in process in chrome:flags, then visited chrome://crash and looked at the crash report in crash: https://crash/browse?stbtiq=f95318c4a26612e4%20#2

Neither "Experiments" field, or taking the hashes from "product data" and putting it in https://uma.googleplex.com/p/chrome/variations/hashes shows network service."
 
Components: Internals>Metrics>Variations
Summary: Child report crash reports do not showing locally enabled experiments (was: Crash report not showing locally enabled experiments)
If I enable network service and do chrome://inducebrowsercrashforrealz, then I get this report:

crash/d948b20d553ac04d

And there, I can clearly see the line:
  --enable-features=NetworkService

Under "Switches".

So, I think this is already working fine for browser process. I think the issue is with child processes, because feature state is propagated via shared memory and not command-line, so the --enable-features flag doesn't get shown for those crashes.

I'm thinking the right way to support this is to have manually-enabled feature flags be propagated via the command-line (since there should be few of these) while ones coming from the server should continue to use shared memory (since server ones are reported via Experiments field already).

Comment 2 by jam@chromium.org, Feb 7 2018

"If I enable network service ": did you run chrome with -enable-features=NetworkService? If so:
1) this is still broken when users enable a feature in about:flags, i.e. see the crash link in the first comment
2) we don't get switches from non-canary? i.e. see the crash link in bug 807980 which just says "0 of 2 switches were uploaded"
I enabled it from about:flags on canary. about:flags uses --enable-features as the underlying mechanism.

Seems like 2) is a separate bug that should be fixed?


Cc: rsesek@chromium.org scottmg@chromium.org
+scottmg +rsesek +ivanpe

Scott/Robert/Ivan, would you know why we'd see "0 of 2 switches were uploaded" on a crash report? (e.g. https://crash.corp.google.com/feec25050ed6097d)
Cc: ivanpe@chromium.org
I don't actually know, but I always assumed that the 2/2 were "boring" switches per https://cs.chromium.org/chromium/src/chrome/common/crash_keys.cc?rcl=4653fb7432bff1ed03d6ec7cf207dcb4e20c43ad&l=29. I can investigate later if no one actually knows for sure.
Hmm, if that's the case, that would indicate network service isn't getting enabled by about:flags for that report...

Comment 8 by jam@chromium.org, Feb 7 2018

@asvitkine: these code paths are only run with network service enabled
One thing I noticed is that --force-fieldtrials is marked as a boring flag on Windows. So it could be that it's that flag that's being used here. I'll send a CL to remove it from boring flags.

It could be a conjunction of that and  crbug.com/309729  that could be the case in those originally report. Or could be something else (e.g. memory corruption flipping the boolean state of the feature. ;)
I'm wrong, I don't think IsBoringSwitch() explains the "0 of 2 switches were uploaded". Looking into that now.
OK, I took a look at the crash server code and raw protobufs for f95318c4a26612e4 vs. feec25050ed6097d.

In feec25... I believe that 0/2 switches literally means there's a crash key that says "num-switches = 2", but there's not switch-1 and switch-2 in the protobuf.

Conversely in f95318... num-switches = 16, and there's values for switch-1 .. switch-14.

Looking again at https://cs.chromium.org/chromium/src/components/crash/core/common/crash_keys.cc?l=84 I believe my initial guess was actually right.

It sets num-switches based on argv size, but then sets switch-N based on skipping "boring" keys, so it is indeed things in this list https://cs.chromium.org/chromium/src/chrome/common/crash_keys.cc?l=29 that will get dropped.

Also, if there's more than 15 switches they won't be uploaded either.

Having said all that, now that we don't have an arbitrary size limit and preregistration requirements on keys (that's correct Robert?) could we just drop this confusing business and always upload all switches?


Yes, I do suspect that the "0 of 2 switches" is because of IsBoringSwitch().

Regarding uploading all the switches: we still do need slots for each switch (see https://cs.chromium.org/chromium/src/components/crash/core/common/crash_keys.cc?type=cs&q=switchesfromcommandline&sq=package:chromium&l=63), but there's no reason to not expand the count. Alternatively, we could upload the entire command line as a single string, but that would then require processing it into individual switches on the server side.
Project Member

Comment 13 by bugdroid1@chromium.org, Feb 8 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/223d22856887469d46f88aea49259a76a490f272

commit 223d22856887469d46f88aea49259a76a490f272
Author: Alexei Svitkine <asvitkine@chromium.org>
Date: Thu Feb 08 00:18:35 2018

Make user-enabled features be added to child process command lines.

Previously, they'd only be passed using the normal shared memory
mechanism, but this makes them hidden which is counterproductive
for crash reports. This change makes them be explicitly added,
so that they appear in a consistent manner in child processes and
browser process.

Should not affect any functional behavior other than the command
line and crash reports showing the extra flags.

Also cleans up field_trial_unittest.cc by removing redundant
base:: prefixes.

Bug:  809137 
Change-Id: I12065cd74d6d830593453a67cd6c59dbffada718
Reviewed-on: https://chromium-review.googlesource.com/907593
Commit-Queue: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#535207}
[modify] https://crrev.com/223d22856887469d46f88aea49259a76a490f272/base/feature_list.cc
[modify] https://crrev.com/223d22856887469d46f88aea49259a76a490f272/base/feature_list.h
[modify] https://crrev.com/223d22856887469d46f88aea49259a76a490f272/base/feature_list_unittest.cc
[modify] https://crrev.com/223d22856887469d46f88aea49259a76a490f272/base/metrics/field_trial.cc
[modify] https://crrev.com/223d22856887469d46f88aea49259a76a490f272/base/metrics/field_trial_unittest.cc

Status: Fixed (was: Assigned)
Marking as Fixed.

https://chromium-review.googlesource.com/907593 makes child processes' command line contain manually (e.g. about:flags) enabled flags via --enable-features and --disable-features switches.

I also landed https://chromium-review.googlesource.com/906911 which removed --force-fieldtrials from the boring switches list, which might have been the case for the crashes reported under bug 807980.

So this the trial reporting issue should be resolved. We should see if such crash reports are tagged correctly now or not. If there's still issues, we can open follow-up bugs to investigate.

Comment 15 by jam@chromium.org, Feb 8 2018

Thank you!
FYI in  issue 814128 , we will start reporting all switches except uninteresting ones.

Sign in to add a comment