Safe Browsing metrics for when user opt-out is unknown |
||||||
Issue descriptionIt's hypothetically possible for the following situation to happen: 1. WebView starts up. We asynchronously check for user consent (via GMS). 2. The app loads a URL. We perform a Safe Browsing check in parallel. a. We asynchronously check the URL against the database (via GMS). 3. We receive a result from GMS about the safety of the URL. a. We don't know yet if the user has opted in. Our logic treats this as an opt-out, so we treat this URL as if it were safe. 4. We later find out the user's opt-in preference. This is a case we explicitly check for (hence the logic mentioned in 3a). However, it's not clear how often we hit this case in practice.
,
Apr 24 2018
Well, didn't mean to post that yet... Was also going to say: It would be interesting to also know how often: * we whitelist an unsafe URL because we haven't heard back from GMS * we whitelist a safe URL (so it doesn't matter either way) * we start checking a URL because we don't know user opt-in, however we find this out before the URL check finishes It would also be nice to get these metrics in for M67, because we're thinking about changing this code in M68 (and would like to compare data before/after to determine if we've regressed).
,
May 3 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4d79a307d38653862fd0401923a71f198c90fb04 commit 4d79a307d38653862fd0401923a71f198c90fb04 Author: Nate Fischer <ntfschr@chromium.org> Date: Thu May 03 06:09:05 2018 AW: add UMA metric for user-consent vs url check race The other CL in this series: http://crrev/i/616309 This adds the entries in histograms.xml and enums.xml for the SafeBrowsing.WebView.UserConsentVsUrlCheckRace histogram, which will be logged downstream, for the first Safe Browsing check. Bug: 836064 Test: check that metric is logged under chrome://histograms/SafeBrowsing Change-Id: Ief746b6c03044edc7becbcbf1664bfd08e2a2843 Reviewed-on: https://chromium-review.googlesource.com/1032255 Reviewed-by: Ilya Sherman <isherman@chromium.org> Reviewed-by: Bo <boliu@chromium.org> Commit-Queue: Nate Fischer <ntfschr@chromium.org> Cr-Commit-Position: refs/heads/master@{#555671} [modify] https://crrev.com/4d79a307d38653862fd0401923a71f198c90fb04/tools/metrics/histograms/enums.xml [modify] https://crrev.com/4d79a307d38653862fd0401923a71f198c90fb04/tools/metrics/histograms/histograms.xml
,
May 3 2018
The following revision refers to this bug: https://chrome-internal.googlesource.com/clank/internal/apps/+/5edc54894ebd79d7fc0a413539b429024350ed09 commit 5edc54894ebd79d7fc0a413539b429024350ed09 Author: Nate Fischer <ntfschr@google.com> Date: Thu May 03 22:52:27 2018
,
May 4 2018
Changwan, do you think it's worth it to cherry pick at this point? This unfortunately won't be a clean cherry pick due to some other refactorings.
,
May 7 2018
Hmm... I'm still worried about #0. Not every cherrypick has to be super clean anyways, so I hope that you can find a way.
,
May 7 2018
Requesting merge so that we can have a better understanding of a potential impact from issue 835497 . I believe we need to cherry pick both CLs (comment #3 and #4).
,
May 7 2018
This bug requires manual review: M67 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
May 8 2018
Do we absolutely need this in M67? How bad will it be to only have it in M68?
,
May 10 2018
Ping!
,
May 10 2018
I would characterize this as a "want," not a "need." But, here's my thoughts on pros/cons: Pros: * There's a chance that issue 835497 regressed correctness. If it did so significantly, we would want to revert that other change. Merging this to M67 is an easy way to understand any potential regression * These code changes are limited to WebView, no affect on chrome * Low risk for bugs: this only adds logs, doesn't change webview behavior Anticipated risk: * I thought the cherry pick for comment#4 wouldn't be clean--git pleasantly surprised me (yay)! Cherry pick uploaded to http://crrev/i/623215 * The CL from comment#3 had conflict, but that's to be expected. These conflicts are trivial to resolve, it's just XML. Cherry pick is http://crrev/c/1050963 * I'm trying to test this locally, but I'm having trouble syncing to the branch (I don't remember the steps for getting the buildspecs, but I'm happy to compile if I can get pointers)
,
May 14 2018
Nate, did you find a way to handle this ?
,
May 14 2018
I am leaning towards not merging this into M67.
,
May 15 2018
I'm not familiar with syncing to branch--I'll need a link to docs on using buildspecs. If we don't merge this, we can wait until issue 835497 rolls out and look at these metrics. If we're OK with the results as-is, we won't take any action. If things look problematic, we can revert issue 835497 to see if it improves these metrics. We'll get our answer, but it might take 1-2 milestones longer.
,
May 17 2018
Thanks. I didn't get to this today, but I'll try to compile tomorrow (unless you vote against merging).
,
May 17 2018
If this is just a want and not a need then let's not take the risk of merging it given that it is not going to be a clean merge
,
May 17 2018
Ack. Marking this as fixed then. Let's evaluate the metrics results in M68 and decide if we need the backup plan I outlined in comment #14.
,
May 18 2018
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by ntfschr@chromium.org
, Apr 24 2018