Child report crash reports do not showing locally enabled experiments |
||||
Issue descriptionIn 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."
,
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"
,
Feb 7 2018
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?
,
Feb 7 2018
+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)
,
Feb 7 2018
,
Feb 7 2018
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.
,
Feb 7 2018
Hmm, if that's the case, that would indicate network service isn't getting enabled by about:flags for that report...
,
Feb 7 2018
@asvitkine: these code paths are only run with network service enabled
,
Feb 7 2018
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. ;)
,
Feb 7 2018
I'm wrong, I don't think IsBoringSwitch() explains the "0 of 2 switches were uploaded". Looking into that now.
,
Feb 7 2018
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?
,
Feb 7 2018
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.
,
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
,
Feb 8 2018
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.
,
Feb 8 2018
Thank you!
,
Feb 26 2018
FYI in issue 814128 , we will start reporting all switches except uninteresting ones. |
||||
►
Sign in to add a comment |
||||
Comment 1 by asvitk...@chromium.org
, Feb 7 2018Summary: Child report crash reports do not showing locally enabled experiments (was: Crash report not showing locally enabled experiments)