New issue
Advanced search Search tips

Issue 670529 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2016
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug
M57



Sign in to add a comment

[Offline pages] Break apart PRERENDERING_FAILED

Project Member Reported by petewil@chromium.org, Dec 2 2016

Issue description

Today, when we get PRERENDERING_FAILED, we do not try another request.  However, PRERENDERING_FAILED is a combination of causes, some are continuable, some are not.  We should break this apart into continuable and non-continuable cases, and allow the next request to proceed on a continuable case.


From the mail thread:
----------------------------------
The PRERENDERING_FAILED_NO_RETRY code is defined just for that request - experienced a hard-ish failure so we should not retry that specific request. 
But we can/should try another one.

The PRERENDERING_CANCELED code is meant to indicate that maybe now is not a good time to try any more prerendering so should not try another request yet.
This happens for the UNSUPPORTED_SCHEME status and if the prerenderer cancels the operation (which can be for resource pressure).

So, I think the open issue is with the PRERENDERING_FAILED code. Currently we will not try another request but maybe we should. However, there is a potential
concern with trying another request if the failed request is the only available request as it could loop through all the attempts while some transient condition was causing
the failure. This was a more realistic concern when we could get the RECENTLY_VISITED code as it would fail very fast. There are some codes that I wonder if
we should still stop processing - MEMORY_LIMIT_EXCEEDED and RENDERED_CRASHED. There is another one I am curious about how it happens and so how
we should handle continuing: NEW_NAVIGATION_ENTRY.

So I wonder if we should add a new enum code for MEMORY_LIMIT_EXCEEDED and RENDERER_CRASHED at least where we do not continue processing but
we do for other FAILED cases. PRERENDERING_FAILED_STOP_PROCESSING?
 
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 5 2016

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

commit f9a803de56a74d1f92a07d2ee0934ecd0ca8ace4
Author: dougarnett <dougarnett@chromium.org>
Date: Mon Dec 05 23:53:09 2016

[OfflinePages] Classifies PRERENDERING_FAILED cases whether to TryNext

Defines new PRERENDERING_FAILED_NO_NEXT for failure types where the
Coordinator should not continue process the next request and now will
process the next request for PRERENDERING_FAILED failures.

I took a code inspection pass on the prerender code and identified
most of the FinalStatus codes we see in UMA into whether they are
FAILED, FAILED_NO_RETRY, or FAILED_NO_NEXT. There we a couple I
wasn't confident about that I left to default as FAILED.

BUG= 670529 

Review-Url: https://codereview.chromium.org/2548903002
Cr-Commit-Position: refs/heads/master@{#436461}

[modify] https://crrev.com/f9a803de56a74d1f92a07d2ee0934ecd0ca8ace4/chrome/browser/android/offline_pages/prerendering_loader.cc
[modify] https://crrev.com/f9a803de56a74d1f92a07d2ee0934ecd0ca8ace4/chrome/browser/android/offline_pages/prerendering_loader_unittest.cc
[modify] https://crrev.com/f9a803de56a74d1f92a07d2ee0934ecd0ca8ace4/components/offline_pages/background/offliner.h
[modify] https://crrev.com/f9a803de56a74d1f92a07d2ee0934ecd0ca8ace4/components/offline_pages/background/request_coordinator.cc
[modify] https://crrev.com/f9a803de56a74d1f92a07d2ee0934ecd0ca8ace4/components/offline_pages/background/request_coordinator_event_logger.cc
[modify] https://crrev.com/f9a803de56a74d1f92a07d2ee0934ecd0ca8ace4/components/offline_pages/background/request_coordinator_unittest.cc
[modify] https://crrev.com/f9a803de56a74d1f92a07d2ee0934ecd0ca8ace4/tools/metrics/histograms/histograms.xml

Project Member

Comment 2 by bugdroid1@chromium.org, Dec 8 2016

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

commit 926b579529f4f67ab2a22364ac1d9ea1eb29a475
Author: dougarnett <dougarnett@chromium.org>
Date: Thu Dec 08 21:41:46 2016

[OfflinePages] Rename PRERENDERING_* result terminology to LOADING_*

As we prepare for making new background loader an option over the
prerendering loader, we need to either generalize Offliner and
BackgroundSavePageResult result terminology or create a new set
of results for the new loader. This CL generalizes that terminology.

BUG= 670529 

Review-Url: https://codereview.chromium.org/2566453002
Cr-Commit-Position: refs/heads/master@{#437355}

[modify] https://crrev.com/926b579529f4f67ab2a22364ac1d9ea1eb29a475/chrome/android/javatests/src/org/chromium/chrome/browser/offlinepages/OfflinePageSavePageLaterEvaluationTest.java
[modify] https://crrev.com/926b579529f4f67ab2a22364ac1d9ea1eb29a475/chrome/browser/android/offline_pages/prerendering_loader.cc
[modify] https://crrev.com/926b579529f4f67ab2a22364ac1d9ea1eb29a475/chrome/browser/android/offline_pages/prerendering_loader_unittest.cc
[modify] https://crrev.com/926b579529f4f67ab2a22364ac1d9ea1eb29a475/chrome/browser/android/offline_pages/prerendering_offliner_unittest.cc
[modify] https://crrev.com/926b579529f4f67ab2a22364ac1d9ea1eb29a475/components/offline_pages/core/background/offliner.h
[modify] https://crrev.com/926b579529f4f67ab2a22364ac1d9ea1eb29a475/components/offline_pages/core/background/request_coordinator.cc
[modify] https://crrev.com/926b579529f4f67ab2a22364ac1d9ea1eb29a475/components/offline_pages/core/background/request_coordinator_event_logger.cc
[modify] https://crrev.com/926b579529f4f67ab2a22364ac1d9ea1eb29a475/components/offline_pages/core/background/request_coordinator_unittest.cc
[modify] https://crrev.com/926b579529f4f67ab2a22364ac1d9ea1eb29a475/components/offline_pages/core/background/request_notifier.h
[modify] https://crrev.com/926b579529f4f67ab2a22364ac1d9ea1eb29a475/components/offline_pages/core/downloads/download_notifying_observer_unittest.cc
[modify] https://crrev.com/926b579529f4f67ab2a22364ac1d9ea1eb29a475/tools/metrics/histograms/histograms.xml

Status: Fixed (was: Untriaged)

Sign in to add a comment