[Offline pages] Stop observing web contents and reset to null after use. Only create a new web cont…

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/bind.h"
 #include "base/json/json_writer.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/sys_info.h"
@@ skipping 154 matching lines @@
         static_cast<int>(OfflinePagesCctApiPrerenderAllowedStatus::
                              NETWORK_PREDICTION_DISABLED) +
             1);
   }
 
   if (!OfflinePageModel::CanSaveURL(request.url())) {
     DVLOG(1) << "Not able to save page for requested url: " << request.url();
     return false;
   }
 
-  if (!loader_)
-    ResetState();
+  ResetLoader();
+  AttachObservers();
 
   MarkLoadStartTime();
 
   // Track copy of pending request.
   pending_request_.reset(new SavePageRequest(request));
   completion_callback_ = completion_callback;
   progress_callback_ = progress_callback;
 
   // Listen for app foreground/background change.
   app_listener_.reset(new base::android::ApplicationStatusListener(
@@ skipping 92 matching lines @@
                                  Offliner::RequestStatus::LOADING_FAILED);
     }
     ResetState();
   }
 }
 
 void BackgroundLoaderOffliner::WebContentsDestroyed() {
   if (pending_request_) {
     SavePageRequest request(*pending_request_.get());
     completion_callback_.Run(request, Offliner::RequestStatus::LOADING_FAILED);
     ResetState();

[dewittj, 2017/05/05 21:26:16]

nit: ResetState will cause a double-free of the destroyed web contents if called
from here. Do you want to fix this in this CL or in another?

[chili, 2017/05/05 21:34:23]

I still can't think of a good way to fix this. With my latest change, we should
only end up here if something unexpected caused the web contents to be destroyed
from elsewhere.  If it's something unexpected causing the web contents to be
destroyed, then the webcontents we're holding on to (and also the
background_loader_contents) are going to be invalid too, which means we should
reset/clear them?

If we're calling reset on the background_loader_contents, that's going to init
destructor for webcontents again, but since we're holding on to a unique_ptr, we
can't avoid this.....

Alternatively, we can assume that if something unexpected happens, we're going
to be screwed anyway....?

[carlosk, 2017/05/05 21:49:48]

Can you detect when that situation is happening -- maybe even adding some flags
specifically for that -- and then force the crash so that we better understand
what's going on?

[chili, 2017/05/06 00:21:17]

We currently don't have a good way of figuring out *when* or *if* this happens. 
Presumably, a gpu or rendering error can cause the web contents to be destroyed
though I haven't been able to reproduce in a test.

As I understand it, we might not even need this method because something
somewhere would've been terribly wrong if the web contents was destroyed without
us (the offliner) knowing about it first...

   }
 }
 
 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()) {
@@ skipping 146 matching lines @@
   cancel_callback_.Run(request);
 }
 
 void BackgroundLoaderOffliner::ResetState() {
   pending_request_.reset();
   snapshot_controller_.reset();
   page_load_state_ = SUCCESS;
   network_bytes_ = 0LL;
   is_low_bar_met_ = false;
   did_snapshot_on_last_retry_ = false;
-  // TODO(chili): Remove after RequestCoordinator can handle multiple offliners.
-  // We reset the loader and observer after completion so loaders
-  // will not be re-used across different requests/tries. This is a temporary
-  // solution while there exists assumptions about the number of offliners
-  // there are.
+  content::WebContentsObserver::Observe(nullptr);
+  loader_.reset();
+}
+
+void BackgroundLoaderOffliner::ResetLoader() {
   loader_.reset(
       new background_loader::BackgroundLoaderContents(browser_context_));
+}
+
+void BackgroundLoaderOffliner::AttachObservers() {
   content::WebContents* contents = loader_->web_contents();
   content::WebContentsObserver::Observe(contents);
   OfflinerData::AddToWebContents(contents, this);
 }
 
 void BackgroundLoaderOffliner::OnApplicationStateChange(
     base::android::ApplicationState application_state) {
   if (pending_request_ && is_low_end_device_ &&
       application_state ==
           base::android::APPLICATION_STATE_HAS_RUNNING_ACTIVITIES) {
@@ skipping 13 matching lines @@
   // Given the choice between int and double, we choose to implicitly convert to
   // a double since it maintains more precision (we can get a longer time in
   // milliseconds than we can with a 2 bit int, 53 bits vs 32).
   double delay = delay_so_far.InMilliseconds();
   signal_data_.SetDouble(signal_name, delay);
 }
 
 }  // namespace offline_pages
 
 DEFINE_WEB_CONTENTS_USER_DATA_KEY(offline_pages::OfflinerData);