[New Tab Page] Change the mechanism to intercept online NTP errors

chrome/browser/search/instant_service.cc

Index: chrome/browser/search/instant_service.cc
diff --git a/chrome/browser/search/instant_service.cc b/chrome/browser/search/instant_service.cc
index 1144e405f7885dd2572749e7fd40575caf337fa9..c1d486c0bac6ab7a970ba2669c325b29501ec466 100644
--- a/chrome/browser/search/instant_service.cc
+++ b/chrome/browser/search/instant_service.cc
@@ -19,6 +19,8 @@
 #include "chrome/browser/themes/theme_service_factory.h"
 #include "chrome/browser/thumbnails/thumbnail_list_source.h"
 #include "chrome/browser/ui/search/instant_search_prerenderer.h"
+#include "chrome/browser/ui/search/new_tab_page_interceptor_service.h"
+#include "chrome/browser/ui/search/new_tab_page_interceptor_service_factory.h"
 #include "chrome/browser/ui/webui/favicon_source.h"
 #include "chrome/browser/ui/webui/ntp/thumbnail_source.h"
 #include "chrome/browser/ui/webui/theme_source.h"
@@ -420,6 +422,14 @@ void InstantService::OnTemplateURLServiceChanged() {
     ResetInstantSearchPrerenderer();
     FOR_EACH_OBSERVER(InstantServiceObserver, observers_,
                       DefaultSearchProviderChanged());
+
+    // Update the New Tab Page interceptor with the new URL.
+    NewTabPageInterceptorService* service =
+        NewTabPageInterceptorServiceFactory::GetForProfile(profile_);
+    if (service) {
+      GURL new_tab_page_url(chrome::GetNewTabPageURL(profile_));
+      service->UpdateDefaultSearchProvider(new_tab_page_url);
+    }

[mmenke, 2015/01/15 22:35:30]

Can't the service just sign up as an InstantServiceObserver instead?  Makes the
new service more modular.

[Mathieu, 2015/01/15 22:42:09]

I tried for a long time to make that work. It turns out that if the Interceptor
depends on the InstantService, the latter seems to be constructed too early in
the Chrome lifecycle. The whole thing crashes and burns with a thread_checker
later on, while it shouldn't because we're on UI thread here, same as when
InstantService usually gets created. Anyhow, I've spoken to various Chrome
engineers and this way is a solution to that. I know it's not 100% satisfactory
but it's not totally out of place here either.

[mmenke, 2015/01/15 23:22:21]

Did you have all those "DCHECK_CURRENTLY_ON(content::BrowserThread::UI);" in the
NewTabPageInterceptorServiceFactory when you ran into the issue?  Any
explanation I can think of for that problem should trigger those.

Should at least move the GetNewTabPageURL into the new service - no reason for
the InstantService to what the service actually needs to know.

[kmadhusu, 2015/01/16 00:13:12]

(Just curious) Did you get a chance to find the culprit CL that is causing this
to crash now? When I wrote the initial code it was working fine.

[mmenke, 2015/01/16 17:54:04]

Currently, the InstantService is created once profile creation is complete, when
we create the BrowserInstantController.

If the new service calls into it on construction, we'd now be creating the
InstantService before profile creation completes, before we even have a
URLRequestContext.

I assume that somewhere down the line, that results in calling SetUserData on
the ResourceContext on the IO thread, before we've passed the ResourceContext
from the IO thread over to the UI thread...Which causes a DCHECK when we
continue setting up the ResourceContext on the UI thread.

So...How could we fix this?  On option would be to create the interceptor on the
IO thread, and then have it set up the service, instead of the other way around,
though that's still a little ugly.  Open to other ideas.

Currently, having the new service "depend" on a service that has to be created
*AFTER* the new service seems really weird.

[mmenke, 2015/01/16 22:04:16]

Shouldn't we just be watching the TemplateURLService, like the InstantService
is?

[Mathieu, 2015/01/21 21:30:02]

Newest patch does that, have a look.

   }
 }