Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2294933003:
[Offline Pages] Handle prerender FinalStatus UnsupportedScheme as a cancel rather than failure. (Closed)
|
Created:
4 years, 3 months ago by dougarnett Modified:
4 years, 3 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, dewittj+watch_chromium.org, tburkard+watch_chromium.org, fgorski+watch_chromium.org, romax+watch_chromium.org, petewil+watch_chromium.org, gavinp+prer_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org, mmenke, dewittj Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Offline Pages] Handle prerender FinalStatus UnsupportedScheme as a cancel rather than failure.
Losing network connection while prerendering can show up as an Unsupported Scheme failure
currently as it sees a special data URL in path toward rendering an error page. We don't want
a flakey network connection fail a background load request on just a single try, so for now,
we will treat this specific error type as a cancel rather than a failure so that it is subject
some limited retrying. The down side, is that for some page that has a repeatable redirect
to an unsupported scheme, we will waste some retries on it that we otherwise could skip.
Ideally, we could get a different final status code from the prerenderer if it could clearly distinguish a network failure condition but it does not currently have such knowledge.
BUG=642025
Committed: https://crrev.com/27e86c877f293dec2693281e7848e4d61a95ba4c
Cr-Commit-Position: refs/heads/master@{#415673}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Revises handling of PRERENDERING_CANCELED to not treat as an abort and not try another request in p… #Patch Set 3 : Revert started try count change from this cl and added TODO with followup bug id. #
Messages
Total messages: 30 (18 generated)
The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Offline Pages] Handle prerender FinalStatus UnsupportedScheme as a cancel rather than failure. Losing network connection while prerendering can show up as an Unsupported Scheme failure currently as it sees a special data URL in path toward rendering an error page. We don't want a flakey network connection fail a background load request on just a single try, so for now, we will treat this specific error type as a cancel rather than a failure so that it is subject some limited retrying. The down side, is that for some page that has a repeatable redirect to an unsupported scheme, we will waste some retries on it that we otherwise could skip. BUG=642025 ========== to ========== [Offline Pages] Handle prerender FinalStatus UnsupportedScheme as a cancel rather than failure. Losing network connection while prerendering can show up as an Unsupported Scheme failure currently as it sees a special data URL in path toward rendering an error page. We don't want a flakey network connection fail a background load request on just a single try, so for now, we will treat this specific error type as a cancel rather than a failure so that it is subject some limited retrying. The down side, is that for some page that has a repeatable redirect to an unsupported scheme, we will waste some retries on it that we otherwise could skip. Ideally, we could get a different final status code from the prerenderer if it could clearly distinguish a network failure condition but it does not currently have such knowledge. BUG=642025 ==========
dougarnett@chromium.org changed reviewers: + dimich@chromium.org, pasko@chromium.org, petewil@chromium.org
https://codereview.chromium.org/2294933003/diff/1/components/offline_pages/ba... File components/offline_pages/background/offliner_policy.h (right): https://codereview.chromium.org/2294933003/diff/1/components/offline_pages/ba... components/offline_pages/background/offliner_policy.h:9: const int kMaxStartedTries = 3; Per dimich request to reduce tries from 5 to 3. On GIN-2g I had a few pages take 3 tries to succesfully load (2 x 2 minutes deadbeat timeouts and then success) so I would not want to go lower than 3 tries.
https://codereview.chromium.org/2294933003/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/2294933003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:173: // unsupported scheme error as a cancel. See issue 642025. If we are closing bug 642025 after this change, we need to open a new bug to fix this hack the right way in the future. https://codereview.chromium.org/2294933003/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/prerendering_loader_unittest.cc (right): https://codereview.chromium.org/2294933003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader_unittest.cc:150: callback_called_ = false; Nice catch! https://codereview.chromium.org/2294933003/diff/1/components/offline_pages/ba... File components/offline_pages/background/offliner_policy.h (right): https://codereview.chromium.org/2294933003/diff/1/components/offline_pages/ba... components/offline_pages/background/offliner_policy.h:9: const int kMaxStartedTries = 3; On 2016/08/30 20:28:15, dougarnett wrote: > Per dimich request to reduce tries from 5 to 3. On GIN-2g I had a few pages take > 3 tries to succesfully load (2 x 2 minutes deadbeat timeouts and then success) > so I would not want to go lower than 3 tries. This might be better as an unrelated changelist.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with a note: https://codereview.chromium.org/2294933003/diff/1/components/offline_pages/ba... File components/offline_pages/background/offliner_policy.h (right): https://codereview.chromium.org/2294933003/diff/1/components/offline_pages/ba... components/offline_pages/background/offliner_policy.h:9: const int kMaxStartedTries = 3; On 2016/08/30 20:28:15, dougarnett wrote: > Per dimich request to reduce tries from 5 to 3. On GIN-2g I had a few pages take > 3 tries to succesfully load (2 x 2 minutes deadbeat timeouts and then success) > so I would not want to go lower than 3 tries. Note 5 or 3 are kinda theoretical w/o actual measurement. I meant that in general if we use simple heuristics and retry on potentially wide bucket of outcomes, some of which could be permanent in nature, it makes sense to limit the number of retries. I don't have specific pref to a particular number. Do we have enough UMA to see outcomes like "took N retries to load"?
Note: this CL is not sufficient - will be uploading some changes to handling PRERENDERING_CANCEL result. And also should decide to add UMA in this CL or a separate one. https://codereview.chromium.org/2294933003/diff/1/components/offline_pages/ba... File components/offline_pages/background/offliner_policy.h (right): https://codereview.chromium.org/2294933003/diff/1/components/offline_pages/ba... components/offline_pages/background/offliner_policy.h:9: const int kMaxStartedTries = 3; On 2016/08/30 23:15:38, Dmitry Titov wrote: > On 2016/08/30 20:28:15, dougarnett wrote: > > Per dimich request to reduce tries from 5 to 3. On GIN-2g I had a few pages > take > > 3 tries to succesfully load (2 x 2 minutes deadbeat timeouts and then success) > > so I would not want to go lower than 3 tries. > > Note 5 or 3 are kinda theoretical w/o actual measurement. I meant that in > general if we use simple heuristics and retry on potentially wide bucket of > outcomes, some of which could be permanent in nature, it makes sense to limit > the number of retries. I don't have specific pref to a particular number. > > Do we have enough UMA to see outcomes like "took N retries to load"? We don't yet have that UMA and agree we need it. I could add as part of this CL tomorrow (or as separate CL).
The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL wrt coordinator changes
RequestCoordinator changes look good, still waiting for answers to other questions.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2294933003/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/prerendering_loader.cc (right): https://codereview.chromium.org/2294933003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader.cc:173: // unsupported scheme error as a cancel. See issue 642025. On 2016/08/30 20:42:47, Pete Williamson wrote: > If we are closing bug 642025 after this change, we need to open a new bug to fix > this hack the right way in the future. Acknowledged. https://codereview.chromium.org/2294933003/diff/1/chrome/browser/android/offl... File chrome/browser/android/offline_pages/prerendering_loader_unittest.cc (right): https://codereview.chromium.org/2294933003/diff/1/chrome/browser/android/offl... chrome/browser/android/offline_pages/prerendering_loader_unittest.cc:150: callback_called_ = false; On 2016/08/30 20:42:47, Pete Williamson wrote: > Nice catch! Acknowledged. https://codereview.chromium.org/2294933003/diff/1/components/offline_pages/ba... File components/offline_pages/background/offliner_policy.h (right): https://codereview.chromium.org/2294933003/diff/1/components/offline_pages/ba... components/offline_pages/background/offliner_policy.h:9: const int kMaxStartedTries = 3; On 2016/08/30 23:27:47, dougarnett wrote: > On 2016/08/30 23:15:38, Dmitry Titov wrote: > > On 2016/08/30 20:28:15, dougarnett wrote: > > > Per dimich request to reduce tries from 5 to 3. On GIN-2g I had a few pages > > take > > > 3 tries to succesfully load (2 x 2 minutes deadbeat timeouts and then > success) > > > so I would not want to go lower than 3 tries. > > > > Note 5 or 3 are kinda theoretical w/o actual measurement. I meant that in > > general if we use simple heuristics and retry on potentially wide bucket of > > outcomes, some of which could be permanent in nature, it makes sense to limit > > the number of retries. I don't have specific pref to a particular number. > > > > Do we have enough UMA to see outcomes like "took N retries to load"? > > We don't yet have that UMA and agree we need it. I could add as part of this CL > tomorrow (or as separate CL). Per Pete's original request, reverting this change from this CL and will add UMA in follow-up CL (in which can decide whether to speculatively adjust this lower). I would like to try some of the EM urls on GIN-2gpoor to help decide that.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by dougarnett@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dimich@chromium.org, pasko@chromium.org Link to the patchset: https://codereview.chromium.org/2294933003/#ps40001 (title: "Revert started try count change from this cl and added TODO with followup bug id.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Handle prerender FinalStatus UnsupportedScheme as a cancel rather than failure. Losing network connection while prerendering can show up as an Unsupported Scheme failure currently as it sees a special data URL in path toward rendering an error page. We don't want a flakey network connection fail a background load request on just a single try, so for now, we will treat this specific error type as a cancel rather than a failure so that it is subject some limited retrying. The down side, is that for some page that has a repeatable redirect to an unsupported scheme, we will waste some retries on it that we otherwise could skip. Ideally, we could get a different final status code from the prerenderer if it could clearly distinguish a network failure condition but it does not currently have such knowledge. BUG=642025 ========== to ========== [Offline Pages] Handle prerender FinalStatus UnsupportedScheme as a cancel rather than failure. Losing network connection while prerendering can show up as an Unsupported Scheme failure currently as it sees a special data URL in path toward rendering an error page. We don't want a flakey network connection fail a background load request on just a single try, so for now, we will treat this specific error type as a cancel rather than a failure so that it is subject some limited retrying. The down side, is that for some page that has a repeatable redirect to an unsupported scheme, we will waste some retries on it that we otherwise could skip. Ideally, we could get a different final status code from the prerenderer if it could clearly distinguish a network failure condition but it does not currently have such knowledge. BUG=642025 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Offline Pages] Handle prerender FinalStatus UnsupportedScheme as a cancel rather than failure. Losing network connection while prerendering can show up as an Unsupported Scheme failure currently as it sees a special data URL in path toward rendering an error page. We don't want a flakey network connection fail a background load request on just a single try, so for now, we will treat this specific error type as a cancel rather than a failure so that it is subject some limited retrying. The down side, is that for some page that has a repeatable redirect to an unsupported scheme, we will waste some retries on it that we otherwise could skip. Ideally, we could get a different final status code from the prerenderer if it could clearly distinguish a network failure condition but it does not currently have such knowledge. BUG=642025 ========== to ========== [Offline Pages] Handle prerender FinalStatus UnsupportedScheme as a cancel rather than failure. Losing network connection while prerendering can show up as an Unsupported Scheme failure currently as it sees a special data URL in path toward rendering an error page. We don't want a flakey network connection fail a background load request on just a single try, so for now, we will treat this specific error type as a cancel rather than a failure so that it is subject some limited retrying. The down side, is that for some page that has a repeatable redirect to an unsupported scheme, we will waste some retries on it that we otherwise could skip. Ideally, we could get a different final status code from the prerenderer if it could clearly distinguish a network failure condition but it does not currently have such knowledge. BUG=642025 Committed: https://crrev.com/27e86c877f293dec2693281e7848e4d61a95ba4c Cr-Commit-Position: refs/heads/master@{#415673} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/27e86c877f293dec2693281e7848e4d61a95ba4c Cr-Commit-Position: refs/heads/master@{#415673} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
