New issue
Advanced search Search tips

Issue 655178 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug



Sign in to add a comment

[Offline Pages] RequestPicker needs to remove requests it detects have had too many attempts

Project Member Reported by dougarnett@chromium.org, Oct 12 2016

Issue description

Currently, the RequestPicker will filter out requests for consideration
that have had too many started attempts or completed attempts but it
does not actually remove them from the queue (not report to observer
that they failed) so a corresponding Download notification will continue
to animate until the request is expired or canceled by user.

Note many cases of too many attempts will be covered by the Coordinator
(if the OfflineDoneCallback is called) but cases where that callback
is not called (crash or killed by android during processing) will have
this problem.

See lines 145 and 149 of Oct 11 version of request_picker.cc.
 
I don't think request picker should be responsible for that, but I agree that such clean up should be done.
Sure, actually I expect to be part of Pete's refactoring into Tasks rather than just fix current RequestPicker logic. 
OK. Got it.
Labels: -Pri-2 M-56 Pri-1
Not cleaning up these requests means the notification remains actively animating. 
We need to clean these up and cause the NotifyComplete to fire for them.
It looks to me like requests with too many completed attempts are removed in OfflinerDoneCallback, and requests with too many started attempts are removed in AbortRequestAttempt.

Yes, but IFF OfflinerDoneCallback is called for that request. 

So if chrome killed or crashes while offliner in progress, that
callback never happens and the picker will never pick it again
and so it will stay until 7 day expiry with animated notification.

This happens will crash loop or resource taxing request where
android kills us where the start count reaches the limit.
Btw, I saw this when Dmitry was trying his svelte device and saw that his download attempts where not completing. In that case, he was hitting the
the background bytes UMA DCHECK crash we were seeing a couple weeks ago.
Labels: -M-56 M-57
Moving to M57, this is not important enough to need to be in M56.
Issue 650480 has been merged into this issue.
Status: Fixed (was: Assigned)

Sign in to add a comment