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

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug



Sign in to add a comment
link

Issue 902727: Variations.SeedFetchResponseOrErrorCode reports a very high number of ABORTED cases caused by HTTP fallback

Reported by asvitk...@chromium.org, Nov 7 Project Member

Issue description

Variations.SeedFetchResponseOrErrorCode reports a very high number of ABORTED cases caused by HTTP fallback.

See this thread for initial investigation:
https://groups.google.com/a/google.com/forum/#!topic/chrome-metrics-team/O0V2iBQLSrM

Some things we know:
  - It's caused by the HTTP fallback logic (we can see effect from 1% experiment):
    https://uma.googleplex.com/p/chrome/variations?sid=5da9a67b36a91dceee3be859927f3ee0
  - The failures are being logged to HTTPS histogram, not the HTTP histogram.
  - Even though error is logged a lot, it's not proportial to number of users like other buckets are - it seems like some users are reporting this bucket *a lot* with top 50 client ids reporting millions of samples in the bucket. Is there some weird retry loop happening?
 

Comment 1 by asvitk...@chromium.org, Nov 7

It seems w/ the fallback, we're making much more request?

https://uma.googleplex.com/p/chrome/variations?sid=6341b9ca65ba1be808f4b30b6e9cc5e8

Variations.RequestCount shows a 31% increase in total count and 75th percentile from being the 10th request to the 100th request. So a lot more requests are trying to be made....

Comment 2 by asvitk...@chromium.org, Nov 7

And specifically, most samples being in the "overflow" bucket for Variations.RequestCount (e.g. on Histogram Distribution tab).

Comment 3 by asvitk...@chromium.org, Nov 7

So one hypothesis is if |insecure_variations_server_url_| was somehow initialized to an HTTPS URL, then the logic would be faulty. Because it would keep retrying failed HTTPS requests with more HTTPS requests and it would keep trying to fetch a ton.

But I don't see how this could happen for regular users. For developers, they could specify --variations-insecure-server-url and have it be an HTTPS url and cause this to happen. But it seems unlikely that this would cause such a big issue. (Still, we should fix that.)

Given that we see a large percentage of ABORTED buckets reported on canary, we could start with the above fix and see what effect it has. (Where the fix is tracking explicitly if the pending request is a fallback requests rather than checking if the URL is HTTPS).

Comment 4 by asvitk...@chromium.org, Nov 7

The other thing I was thinking - could something behind-the-scenes be converting HTTP urls to HTTPS ones to make the logic not work properly? Perhaps we can add a histogram to try to detect that (i.e. compare the info we get from URL vs. our own explicit tracking if it was fallback) and see what the data looks like on canary?

Comment 5 by carlosil@chromium.org, Nov 7

Thanks for the extra information, yeah anything that's treating a fallback request as a non-fallback (which seems to be happening here, since the metrics are logged to the non .HTTP histogram) would lead to infinite requests if requests are failing. A simple fix for this would be to check that insecure_variations_server_url_ is indeed an HTTP URL, otherwise not retry.

Comment 6 by carlosil@chromium.org, Nov 7

Since we are using the final URL to check this, another possible case (besides the one mentioned in #3 about --variations-insecure-server-url being set), is that the URL is redirecting to HTTPS, in that case the infinite loop would also happen for any failures

Comment 7 by carlosil@chromium.org, Nov 7

An example practical case that would cause this is a Captive portal that redirects everything to "https://login.test", the first HTTPS attempt would fail (since it's HTTPS), then we would correctly fall back to HTTP, but if that gets redirected, the final url would be https://login.test, at that point we'd just keep retrying the http url, which would keep getting redirected to https.

Comment 8 by asvitkine@google.com, Nov 8

Aha, makes sense. Could also be any proxy that tries to helpfully redirect http URLs to equivalent https ones.

I wonder, could we simply disable following redirects for those requests?

Comment 9 by bugdroid1@chromium.org, Nov 9

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/91d246885ead6155c131e027d99be1aa79b9ae3c

commit 91d246885ead6155c131e027d99be1aa79b9ae3c
Author: Carlos IL <carlosil@chromium.org>
Date: Fri Nov 09 23:22:28 2018

Variations service now checks explicitely if previous attempt was HTTP.

Variations service now sets a flag if the last request was an HTTP
retry, and uses it to decide whether to retry again, instead of
relying on the scheme of the final URL. Also, retries are now disabled
if the fallback url is HTTPS.

Bug: 902727
Change-Id: Ibd6df4a22bc6302b231aff0ae32f8e1c8a1f277c
Reviewed-on: https://chromium-review.googlesource.com/c/1325035
Commit-Queue: Carlos IL <carlosil@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607022}
[modify] https://crrev.com/91d246885ead6155c131e027d99be1aa79b9ae3c/components/variations/service/variations_service.cc
[modify] https://crrev.com/91d246885ead6155c131e027d99be1aa79b9ae3c/components/variations/service/variations_service.h
[modify] https://crrev.com/91d246885ead6155c131e027d99be1aa79b9ae3c/components/variations/service/variations_service_unittest.cc

Comment 10 by bugdroid1@chromium.org, Nov 14

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8d41f8c9c5f71f94133e1e557701521b5e464c63

commit 8d41f8c9c5f71f94133e1e557701521b5e464c63
Author: Carlos IL <carlosil@chromium.org>
Date: Wed Nov 14 02:59:49 2018

Revert "Variations service now checks explicitely if previous attempt was HTTP."

This reverts commit 91d246885ead6155c131e027d99be1aa79b9ae3c.

Reason for revert: This CL made ChromeBrowserMainBrowserTest.VariationsServiceStartsRequestOnNetworkChange flaky ( crbug.com/904108 ) reverting while investigating why.

Original change's description:
> Variations service now checks explicitely if previous attempt was HTTP.
> 
> Variations service now sets a flag if the last request was an HTTP
> retry, and uses it to decide whether to retry again, instead of
> relying on the scheme of the final URL. Also, retries are now disabled
> if the fallback url is HTTPS.
> 
> Bug: 902727
> Change-Id: Ibd6df4a22bc6302b231aff0ae32f8e1c8a1f277c
> Reviewed-on: https://chromium-review.googlesource.com/c/1325035
> Commit-Queue: Carlos IL <carlosil@chromium.org>
> Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#607022}

TBR=asvitkine@chromium.org,carlosil@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 902727
Change-Id: I67036ea20097f889e71de5179a2e4510a201388d
Reviewed-on: https://chromium-review.googlesource.com/c/1334421
Reviewed-by: Carlos IL <carlosil@chromium.org>
Commit-Queue: Carlos IL <carlosil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607882}
[modify] https://crrev.com/8d41f8c9c5f71f94133e1e557701521b5e464c63/components/variations/service/variations_service.cc
[modify] https://crrev.com/8d41f8c9c5f71f94133e1e557701521b5e464c63/components/variations/service/variations_service.h
[modify] https://crrev.com/8d41f8c9c5f71f94133e1e557701521b5e464c63/components/variations/service/variations_service_unittest.cc

Comment 11 by bugdroid1@chromium.org, Nov 27

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/871a1ef511b48acdad53f7d8e8fafd6362fc52d2

commit 871a1ef511b48acdad53f7d8e8fafd6362fc52d2
Author: Carlos IL <carlosil@chromium.org>
Date: Tue Nov 27 17:05:29 2018

Reland "Variations service now checks explicitely if previous attempt was HTTP."

This reverts commit 8d41f8c9c5f71f94133e1e557701521b5e464c63.

Reason for revert: This CL reverts the revert, and fixes the flakiness in the test by allowing one or two requests in the test.

Bug:  904108 , 902727

Original change's description:
> Revert "Variations service now checks explicitely if previous attempt was HTTP."
>
> This reverts commit 91d246885ead6155c131e027d99be1aa79b9ae3c.
>
> Reason for revert: This CL made ChromeBrowserMainBrowserTest.VariationsServiceStartsRequestOnNetworkChange flaky ( crbug.com/904108 ) reverting while investigating why.
>
> Original change's description:
> > Variations service now checks explicitely if previous attempt was HTTP.
> >
> > Variations service now sets a flag if the last request was an HTTP
> > retry, and uses it to decide whether to retry again, instead of
> > relying on the scheme of the final URL. Also, retries are now disabled
> > if the fallback url is HTTPS.
> >
> > Bug: 902727
> > Change-Id: Ibd6df4a22bc6302b231aff0ae32f8e1c8a1f277c
> > Reviewed-on: https://chromium-review.googlesource.com/c/1325035
> > Commit-Queue: Carlos IL <carlosil@chromium.org>
> > Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#607022}
>
> TBR=asvitkine@chromium.org,carlosil@chromium.org
>
> # Not skipping CQ checks because original CL landed > 1 day ago.
>
> Bug: 902727
> Change-Id: I67036ea20097f889e71de5179a2e4510a201388d
> Reviewed-on: https://chromium-review.googlesource.com/c/1334421
> Reviewed-by: Carlos IL <carlosil@chromium.org>
> Commit-Queue: Carlos IL <carlosil@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#607882}

Change-Id: I5618dd7c48849ddf1fc8cd8f613ba7d83ebc66c0
Reviewed-on: https://chromium-review.googlesource.com/c/1335666
Commit-Queue: Carlos IL <carlosil@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611152}
[modify] https://crrev.com/871a1ef511b48acdad53f7d8e8fafd6362fc52d2/chrome/browser/chrome_browser_main_browsertest.cc
[modify] https://crrev.com/871a1ef511b48acdad53f7d8e8fafd6362fc52d2/components/variations/service/variations_service.cc
[modify] https://crrev.com/871a1ef511b48acdad53f7d8e8fafd6362fc52d2/components/variations/service/variations_service.h
[modify] https://crrev.com/871a1ef511b48acdad53f7d8e8fafd6362fc52d2/components/variations/service/variations_service_unittest.cc

Sign in to add a comment