[OfflinePages] RequestCoordinatorFactory return null for incognito

components/offline_pages/background/request_coordinator.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 "components/offline_pages/background/request_coordinator.h"
 
 #include <limits>
 #include <utility>
 
 #include "base/bind.h"
@@ skipping 176 matching lines @@
 }
 
 RequestCoordinator::~RequestCoordinator() {}
 
 int64_t RequestCoordinator::SavePageLater(const GURL& url,
                                           const ClientId& client_id,
                                           bool user_requested,
                                           RequestAvailability availability) {
   DVLOG(2) << "URL is " << url << " " << __func__;
 
+  // Make sure this is called on actual instance (as the factory
+  // may not return one for incognito profile).
+  CHECK(this);

[fgorski, 2016/11/22 21:43:37]

I don't think this is a good strategy. (It does not contribute.)

If you fix the factory, this is not necessary.

[dougarnett, 2016/11/22 22:01:16]

I found the Factory returning null was not sufficient, this method was getting
called with null this pointer. We can fix the caller to check the Factory result
but that doesn't stop a new caller from hitting the same problem if they don't
check the factory result.

+
   if (!OfflinePageModel::CanSaveURL(url)) {
     DVLOG(1) << "Not able to save page for requested url: " << url;
     return 0L;
   }
 
   int64_t id = GenerateOfflineId();
 
   // Build a SavePageRequest.
   offline_pages::SavePageRequest request(id, url, client_id, base::Time::Now(),
                                          user_requested);
@@ skipping 747 matching lines @@
 
 ClientPolicyController* RequestCoordinator::GetPolicyController() {
   return policy_controller_.get();
 }
 
 void RequestCoordinator::Shutdown() {
   network_quality_estimator_ = nullptr;
 }
 
 }  // namespace offline_pages