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

Issue 763589 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug

Blocking:
issue 764406



Sign in to add a comment

Change the update timeout for fetching Safe Browsing updates to reduce false timeouts

Project Member Reported by vakh@chromium.org, Sep 9 2017

Issue description

We have received reports from some users that machines on slow networks are unable to download Safe Browsing updates.

Based on the information we have, it appears that this is happening due to time outs.

Safe Browsing hash prefix update fetching code currently has a 30-second time out.
https://cs.chromium.org/chromium/src/components/safe_browsing_db/v4_update_protocol_manager.cc?l=75&rcl=43c3390d15ce27cf490c682d9b286c672d543440

The reason for this time out is to break out of any network state where we are not making download progress, but the time out doesn't have to be this low.

Reports:
1. https://goto.google.com/zajez
2. https://goto.google.com/kjzyf
 
Project Member

Comment 1 by bugdroid1@chromium.org, Sep 9 2017

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

commit 6e0bc1c5d7f0b2b14430523a4189939243c7f53e
Author: Varun Khaneja <vakh@chromium.org>
Date: Sat Sep 09 01:55:24 2017

Change the update timeout to 15 mins to prevent timeouts on slow connections

Bug:  763589 
Change-Id: I1d8a25701b5136f5b757e9093eb8253f9792bf9d
Reviewed-on: https://chromium-review.googlesource.com/658559
Commit-Queue: Nathan Parker <nparker@chromium.org>
Reviewed-by: Nathan Parker <nparker@chromium.org>
Cr-Commit-Position: refs/heads/master@{#500778}
[modify] https://crrev.com/6e0bc1c5d7f0b2b14430523a4189939243c7f53e/components/safe_browsing_db/v4_update_protocol_manager.cc

Comment 2 by vakh@chromium.org, Sep 12 2017

Blocking: 764406

Comment 3 by vakh@chromium.org, Sep 12 2017

Cc: dacton@google.com
Status: Started (was: Assigned)
This landed in 63.0.3212.0. I'm still waiting for the UMA to show that it reduced the time outs. http://shortn/_znsoih4NmC

Comment 4 by vakh@chromium.org, Sep 13 2017

Labels: Merge-Request-62
This seems to have done the expected i.e. drop the number of timeouts to an all-time low. See shortn link above.

Requesting merge approval for this trivial change because without this change, desktop users on very slow networks are unable to use Safe Browsing protection.

Diff is just this:
-static const int kV4TimerUpdateWaitSecMax = 30;
+static const int kV4TimerUpdateWaitSecMax = 15 * 60;  // 15 minutes

i.e change timeout interval from 30 seconds to 15 minutes.

Comment 5 by vakh@chromium.org, Sep 14 2017

Status: Fixed (was: Started)
Project Member

Comment 6 by sheriffbot@chromium.org, Sep 14 2017

Labels: -Merge-Request-62 Merge-Review-62 Hotlist-Merge-Review
This bug requires manual review: M62 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Review-62 Merge-Approved-62
Thanks for the fix - approving merge to M62 (branch: 3202). 
Project Member

Comment 8 by bugdroid1@chromium.org, Sep 14 2017

Labels: -merge-approved-62 merge-merged-3202
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/853e16f977dcbf97875f9c41532fde5a6f3c9b87

commit 853e16f977dcbf97875f9c41532fde5a6f3c9b87
Author: Varun Khaneja <vakh@chromium.org>
Date: Thu Sep 14 20:58:53 2017

[62] Change update timeout to 15min to reduce timeouts on slow networks

TBR=vakh@chromium.org

(cherry picked from commit 6e0bc1c5d7f0b2b14430523a4189939243c7f53e)

Bug:  763589 
Change-Id: I1d8a25701b5136f5b757e9093eb8253f9792bf9d
Reviewed-on: https://chromium-review.googlesource.com/658559
Commit-Queue: Nathan Parker <nparker@chromium.org>
Reviewed-by: Nathan Parker <nparker@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#500778}
Reviewed-on: https://chromium-review.googlesource.com/667959
Reviewed-by: Varun Khaneja <vakh@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#231}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/853e16f977dcbf97875f9c41532fde5a6f3c9b87/components/safe_browsing_db/v4_update_protocol_manager.cc

Comment 9 by vakh@chromium.org, Sep 21 2017

This change went into Beta today. I'm tracking the metrics for all channels here: http://shortn/_m8N5qhQrBo

Comment 10 by vakh@chromium.org, Sep 21 2017

Went into M62 at: 62.0.3202.24
Current Beta: 62.0.3202.29

Comment 11 by vakh@chromium.org, Sep 25 2017

Cc: franc...@mozilla.com
+francois FYI, though it may not apply to Firefox since it is a Chrome implementation bug.

Sign in to add a comment