[OfflinePages] RequestCoordinatorFactory return null for incognito

chrome/browser/net/net_error_tab_helper.cc

Index: chrome/browser/net/net_error_tab_helper.cc
diff --git a/chrome/browser/net/net_error_tab_helper.cc b/chrome/browser/net/net_error_tab_helper.cc
index 823d96fd9a59b8d287529321bd40321bb0be47ea..22a6c2ac20aebcec36f09c2798dc485878a2e1da 100644
--- a/chrome/browser/net/net_error_tab_helper.cc
+++ b/chrome/browser/net/net_error_tab_helper.cc
@@ -281,6 +281,7 @@ void NetErrorTabHelper::DownloadPageLaterHelper(const GURL& page_url) {
   offline_pages::RequestCoordinator* request_coordinator =
       offline_pages::RequestCoordinatorFactory::GetForBrowserContext(
           web_contents()->GetBrowserContext());
+  DCHECK(request_coordinator);

[fgorski, 2016/11/22 22:46:33]

Doug, if request_coordinator is null here, you will still crash in 287 (in
production)/

A common pattern to deal with that is to check returned service:

if (!request_coordinator)
  return;

compare with offline_page_utils or pretty much any other usage of keyed
services.

Having DCHECK here is OK (e.g. just before return;), but please handle non debug
case appropriately too.

Use this as a place to start.
https://cs.chromium.org/chromium/src/chrome/browser/android/offline_pages/dow...

[Pete Williamson, 2016/11/22 22:59:32]

Actually, what I had in mind was not a DCHECK, but 
if (request_coordinator) {
  request_coordinator->SavePageLater(...)
}

So we don't crash retail chrome if this path is somehow executed with a null
request_coordinator.

It would also be fine to do something like Filip suggests:
if (!request_coordinator)
  return;

[dougarnett, 2016/11/23 18:19:52]

Done.

   offline_pages::ClientId client_id(
       offline_pages::kAsyncNamespace, base::GenerateGUID());
   request_coordinator->SavePageLater(