RecentTabHelper won't accept overlapping requests from Downloads anymore.

RecentTabHelper won't accept overlapping requests from Downloads anymore.

This change fixes a free-after-use bug by changing the way RecentTabHelper
handles page snapshot requests from Downloads. The main change is that it now
will ignore overlapping requests from Downloads but a few other changes were
made, many of them required to get this properly fixed:
- Moved SnapshotProgressInfo definition into the CC file.
- Fixed issues with overlapping snapshot requests from Downloads and page
  quality status events.
- Fixed potential issue where last_n and Downloads requests would be mixed up in
  case Background Downloader was disabled.
- Added enforcement of acceptable Offline namespaces for requests.
- Changed the logic for automatically replacing Downloads snapshots once the
  page attains high quality ("fully" loaded).
- Other minor code and comments improvements.

Note: snapshot overlap between last_n and Downloads requests are still accepted.

BUG=678367, 683580
Review-Url: https://codereview.chromium.org/2655673005
Cr-Commit-Position: refs/heads/master@{#446882}
Committed: https://chromium.googlesource.com/chromium/src/+/01fea95b3e06b6b270b816e798e03a7995589d1a

[carlosk, 2017-01-25 01:15:45]

The CQ bit was checked by carlosk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-25 01:16:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[carlosk, 2017-01-25 01:17:32]

carlosk@chromium.org changed reviewers:
+ dimich@chromium.org

[carlosk, 2017-01-25 01:17:33]

dimich@: PTAL.

[carlosk, 2017-01-25 01:19:47]

carlosk@chromium.org changed reviewers:
+ dewittj@chromium.org

[carlosk, 2017-01-25 01:19:47]

dimich@: PTAL.

dewittj@: FYI.

[commit-bot: I haz the power, 2017-01-25 02:05:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-25 02:05:32]

Dry run: This issue passed the CQ dry run.

[Dmitry Titov, 2017-01-27 04:18:22]

Looks awesome, nice comments and structure of code is improved.
Is it possible to add new unittests to check for new behaviors (no overlapped
snapshots etc)?

https://codereview.chromium.org/2655673005/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/recent_tab_helper.cc (right):

https://codereview.chromium.org/2655673005/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:82: // valid if
|page_snapshot_completed| is true.
Not clear which "|page_snapshot_completed|" this comment refers to.

https://codereview.chromium.org/2655673005/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/recent_tab_helper.h (right):

https://codereview.chromium.org/2655673005/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.h:124: // downloads. Null
no successful one has ever completed.
nit: "Null no" -> "Null if no"

[carlosk, 2017-01-27 18:47:14]

Thanks! Indeed a new test was in order and it is now there.

https://codereview.chromium.org/2655673005/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/recent_tab_helper.cc (right):

https://codereview.chromium.org/2655673005/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:82: // valid if
|page_snapshot_completed| is true.
On 2017/01/27 04:18:22, Dmitry Titov wrote:
> Not clear which "|page_snapshot_completed|" this comment refers to.

Done.

https://codereview.chromium.org/2655673005/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/recent_tab_helper.h (right):

https://codereview.chromium.org/2655673005/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.h:124: // downloads. Null
no successful one has ever completed.
On 2017/01/27 04:18:22, Dmitry Titov wrote:
> nit: "Null no" -> "Null if no"

Done.

[dewittj, 2017-01-27 18:48:57]

Publishing drafts from yesterday, will update more based on new patch.

https://codereview.chromium.org/2655673005/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/recent_tab_helper.cc (right):

https://codereview.chromium.org/2655673005/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:106: // that the call
is from Downloads.
Suggestion to remove the DCHECK: change signature to only accept GUID

https://codereview.chromium.org/2655673005/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:121: // TODO(carlosk):
it might be better to make the decision to schedule or not
This TODO is not really actionable.  Is there a bug or doc or another way to
phrase it so that another person could take it on?

https://codereview.chromium.org/2655673005/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:123: // TODO(carlosk):
an edge case happens when the ongoing request was
This should be more explicit about the race condition, and/or should refer to a
bug with more details.  Otherwise it's very mysterious to someone who is not
familiar with the known issue.

https://codereview.chromium.org/2655673005/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:195:
weak_ptr_factory_.InvalidateWeakPtrs();
perhaps the reset() + invalidateweakptrs should be in a "CancelInFlightTasks"
helper method.

[carlosk, 2017-01-27 23:13:30]

Thanks.

https://codereview.chromium.org/2655673005/diff/1/chrome/browser/android/offl...
File chrome/browser/android/offline_pages/recent_tab_helper.cc (right):

https://codereview.chromium.org/2655673005/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:106: // that the call
is from Downloads.
On 2017/01/27 18:48:57, dewittj wrote:
> Suggestion to remove the DCHECK: change signature to only accept GUID

As OfflinePageDownloadBridge also uses the ClientId value provided here it makes
more sense to keep that as is.

https://codereview.chromium.org/2655673005/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:121: // TODO(carlosk):
it might be better to make the decision to schedule or not
On 2017/01/27 18:48:57, dewittj wrote:
> This TODO is not really actionable.  Is there a bug or doc or another way to
> phrase it so that another person could take it on?

Done.

https://codereview.chromium.org/2655673005/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:123: // TODO(carlosk):
an edge case happens when the ongoing request was
On 2017/01/27 18:48:57, dewittj wrote:
> This should be more explicit about the race condition, and/or should refer to
a
> bug with more details.  Otherwise it's very mysterious to someone who is not
> familiar with the known issue.

Added a crbug with further details.

https://codereview.chromium.org/2655673005/diff/1/chrome/browser/android/offl...
chrome/browser/android/offline_pages/recent_tab_helper.cc:195:
weak_ptr_factory_.InvalidateWeakPtrs();
On 2017/01/27 18:48:57, dewittj wrote:
> perhaps the reset() + invalidateweakptrs should be in a "CancelInFlightTasks"
> helper method.

Done.

[Dmitry Titov, 2017-01-27 23:36:38]

lgtm

[dewittj, 2017-01-27 23:52:08]

lgtm, thanks!

Is there any way to rerun the fuzzer on the try bots?

[carlosk, 2017-01-28 00:55:14]

The CQ bit was checked by carlosk@chromium.org

[commit-bot: I haz the power, 2017-01-28 00:55:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[carlosk, 2017-01-28 00:57:36]

On 2017/01/27 23:52:08, dewittj wrote:
> lgtm, thanks!
> 
> Is there any way to rerun the fuzzer on the try bots?

I don't know... I'll try asking at the issue. Otherwise I'd think it should be
rerun once it is marked fixed.

[commit-bot: I haz the power, 2017-01-28 02:07:39]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485564914478590,
"parent_rev": "b34b8896fe3b95563ea13eab767b102a120d464e", "commit_rev":
"01fea95b3e06b6b270b816e798e03a7995589d1a"}

[commit-bot: I haz the power, 2017-01-28 02:08:23]

Description was changed from

==========
RecentTabHelper won't accept overlapping requests from Downloads anymore.

This change fixes a free-after-use bug by changing the way RecentTabHelper
handles page snapshot requests from Downloads. The main change is that it now
will ignore overlapping requests from Downloads but a few other changes were
made, many of them required to get this properly fixed:
- Moved SnapshotProgressInfo definition into the CC file.
- Fixed issues with overlapping snapshot requests from Downloads and page
  quality status events.
- Fixed potential issue where last_n and Downloads requests would be mixed up in
  case Background Downloader was disabled.
- Added enforcement of acceptable Offline namespaces for requests.
- Changed the logic for automatically replacing Downloads snapshots once the
  page attains high quality ("fully" loaded).
- Other minor code and comments improvements.

Note: snapshot overlap between last_n and Downloads requests are still accepted.

BUG=678367,683580
==========

to

==========
RecentTabHelper won't accept overlapping requests from Downloads anymore.

This change fixes a free-after-use bug by changing the way RecentTabHelper
handles page snapshot requests from Downloads. The main change is that it now
will ignore overlapping requests from Downloads but a few other changes were
made, many of them required to get this properly fixed:
- Moved SnapshotProgressInfo definition into the CC file.
- Fixed issues with overlapping snapshot requests from Downloads and page
  quality status events.
- Fixed potential issue where last_n and Downloads requests would be mixed up in
  case Background Downloader was disabled.
- Added enforcement of acceptable Offline namespaces for requests.
- Changed the logic for automatically replacing Downloads snapshots once the
  page attains high quality ("fully" loaded).
- Other minor code and comments improvements.

Note: snapshot overlap between last_n and Downloads requests are still accepted.

BUG=678367,683580

Review-Url: https://codereview.chromium.org/2655673005
Cr-Commit-Position: refs/heads/master@{#446882}
Committed:
https://chromium.googlesource.com/chromium/src/+/01fea95b3e06b6b270b816e798e0...
==========

[commit-bot: I haz the power, 2017-01-28 02:08:24]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/01fea95b3e06b6b270b816e798e0...