[Offline pages] Check errors and invalidate pointers only for mainframe for navigation

[Offline pages] Check errors and invalidate pointers only for mainframe for navigation

BUG=704311
Review-Url: https://codereview.chromium.org/2764373004
Cr-Commit-Position: refs/heads/master@{#459263}
Committed: https://chromium.googlesource.com/chromium/src/+/69ad9a9d95cdf1ca2e7ffafb03a6e7ffdd74106d

[chili, 2017-03-22 23:53:56]

chili@chromium.org changed reviewers:
+ dewittj@chromium.org, dimich@chromium.org, fgorski@chromium.org

[chili, 2017-03-22 23:58:18]

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

[commit-bot: I haz the power, 2017-03-22 23:58:50]

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

[Dmitry Titov, 2017-03-23 01:07:25]

lgtm!

As discussed offline, the check for save_state_==NONE might not be needed...

[fgorski, 2017-03-23 16:18:00]

lgtm mod comment

https://codereview.chromium.org/2764373004/diff/40001/chrome/browser/android/...
File chrome/browser/android/offline_pages/background_loader_offliner.cc (right):

https://codereview.chromium.org/2764373004/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_loader_offliner.cc:279: if
(navigation_handle->HasCommitted() &&
Is there a chance at this point that we have already started the save?

[chili, 2017-03-23 17:31:06]

https://codereview.chromium.org/2764373004/diff/40001/chrome/browser/android/...
File chrome/browser/android/offline_pages/background_loader_offliner.cc (right):

https://codereview.chromium.org/2764373004/diff/40001/chrome/browser/android/...
chrome/browser/android/offline_pages/background_loader_offliner.cc:279: if
(navigation_handle->HasCommitted() &&
On 2017/03/23 16:18:00, fgorski wrote:
> Is there a chance at this point that we have already started the save?

yes, but it should not matter.

Before, we also have the issue of subframe navigations invalidating the weak
ptrs, and subframe navigations do not result in a second OnLoad to kick off
another save, resulting in hanging state.

If we invalidate the pointers here, we run the risk of the second save operation
returning as ALREADY_EXISTS

If we don't invalidate the pointers here, we run the risk of the first save
operation returning as ARCHIVE_CREATION_FAILED, since a navigation commit clears
the dom tree.  In addition, if OnLoad happens immediately afterwards, the
pointers will be invalidated anyway.

Ideally, we remove this pointer invalidation and check if we're already in a
save in DidStopLoading.  I'd prefer to hold off and think about that particular
change a bit more to make sure we're not simply trading one set of race
conditions for another

[dewittj, 2017-03-23 17:32:22]

lgtm

[chili, 2017-03-23 20:58:26]

The CQ bit was checked by chili@chromium.org

[commit-bot: I haz the power, 2017-03-23 20:59:20]

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

[commit-bot: I haz the power, 2017-03-23 22:41:31]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1490302706874650,
"parent_rev": "5e29167b03005b3bb45fe76649d597967ab05dc2", "commit_rev":
"69ad9a9d95cdf1ca2e7ffafb03a6e7ffdd74106d"}

[commit-bot: I haz the power, 2017-03-23 22:42:18]

Description was changed from

==========
[Offline pages] Check errors and invalidate pointers only for mainframe for
navigation

BUG=704311
==========

to

==========
[Offline pages] Check errors and invalidate pointers only for mainframe for
navigation

BUG=704311

Review-Url: https://codereview.chromium.org/2764373004
Cr-Commit-Position: refs/heads/master@{#459263}
Committed:
https://chromium.googlesource.com/chromium/src/+/69ad9a9d95cdf1ca2e7ffafb03a6...
==========

[commit-bot: I haz the power, 2017-03-23 22:42:20]

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