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

chrome/browser/android/offline_pages/background_loader_offliner.cc

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/android/offline_pages/background_loader_offliner.h"
 
 #include "base/metrics/histogram_macros.h"
 #include "base/sys_info.h"
 #include "chrome/browser/android/offline_pages/offline_page_mhtml_archiver.h"
 #include "chrome/browser/android/offline_pages/offliner_helper.h"
@@ skipping 210 matching lines @@
 void BackgroundLoaderOffliner::WebContentsDestroyed() {
   if (pending_request_) {
     SavePageRequest request(*pending_request_.get());
     completion_callback_.Run(request, Offliner::RequestStatus::LOADING_FAILED);
     ResetState();
   }
 }
 
 void BackgroundLoaderOffliner::DidFinishNavigation(
     content::NavigationHandle* navigation_handle) {
+  if (!navigation_handle->IsInMainFrame())
+    return;
   // If there was an error of any kind (certificate, client, DNS, etc),
   // Mark as error page. Resetting here causes RecordNavigationMetrics to crash.
   if (navigation_handle->IsErrorPage()) {
     // TODO(chili): we need to UMA this.
     switch (navigation_handle->GetNetErrorCode()) {
       case net::ERR_ACCESS_DENIED:
       case net::ERR_ADDRESS_INVALID:
       case net::ERR_ADDRESS_UNREACHABLE:
       case net::ERR_CERT_COMMON_NAME_INVALID:
       case net::ERR_CERT_AUTHORITY_INVALID:
@@ skipping 19 matching lines @@
         page_load_state_ = NONRETRIABLE;
         break;
       case net::ERR_INTERNET_DISCONNECTED:
         page_load_state_ = DELAY_RETRY;
         break;
       default:
         page_load_state_ = RETRIABLE;
     }
   }
 
-  // If the document is not the same, invalidate any pending save tasks.
+  // If the document is not the same invalidate any pending save tasks.
   //
   // Downloads or 204/205 response codes do not commit (no new navigation)
   // Same-Document (committed) navigations are:
   // - reference fragment navigations
   // - pushState/replaceState
   // - same document history navigation
   if (navigation_handle->HasCommitted() &&

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

Is there a chance at this point that we have already started the save?

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

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

       !navigation_handle->IsSameDocument()) {
     weak_ptr_factory_.InvalidateWeakPtrs();
   }
 }
 
 void BackgroundLoaderOffliner::SetPageDelayForTest(long delay_ms) {
   page_delay_ms_ = delay_ms;
 }
 
 void BackgroundLoaderOffliner::OnNetworkBytesChanged(int64_t bytes) {
@@ skipping 120 matching lines @@
     int64_t offline_id) {
   // If for some reason the request was reset during while waiting for callback
   // ignore the completion callback.
   if (pending_request_ && pending_request_->request_id() != offline_id)
     return;
   completion_callback_.Run(request, RequestStatus::FOREGROUND_CANCELED);
 }
 }  // namespace offline_pages
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(offline_pages::OfflinerData);