Fixed RLZTracker::GetAccessPointRlz to route IO calls to the FILE thread.

chrome/browser/rlz/rlz.cc

-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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.
 //
 // This code glues the RLZ library DLL with Chrome. It allows Chrome to work
 // with or without the DLL being present. If the DLL is not present the
 // functions do nothing and just return false.
 
 #include "chrome/browser/rlz/rlz.h"
 
 #include <windows.h>
 #include <process.h>
 
 #include <algorithm>
 
 #include "base/file_path.h"
 #include "base/message_loop.h"
 #include "base/path_service.h"
 #include "base/string_util.h"
 #include "base/task.h"
 #include "base/threading/thread.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/browser_thread.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/search_engines/template_url.h"
 #include "chrome/browser/search_engines/template_url_model.h"
 #include "chrome/common/chrome_paths.h"
 #include "chrome/common/env_vars.h"
 #include "chrome/common/notification_registrar.h"
 #include "chrome/common/notification_service.h"
 #include "chrome/installer/util/google_update_settings.h"
 
@@ skipping 145 matching lines @@
     // For organic brandcodes do not use rlz at all. Empty brandcode usually
     // means a chromium install. This is ok.
     std::wstring brand;
     if (!GoogleUpdateSettings::GetBrand(&brand) || brand.empty() ||
         GoogleUpdateSettings::IsOrganic(brand))
       return;
 
     // Do the initial event recording if is the first run or if we have an
     // empty rlz which means we haven't got a chance to do it.
     std::wstring omnibox_rlz;
     RLZTracker::GetAccessPointRlz(rlz_lib::CHROME_OMNIBOX, &omnibox_rlz);

[Roger Tawa OOO till Jul 10th, 2011/01/12 15:46:52]

This call may return an empty string, since we're not on the file thread, if
this is the first time its called.  If the user does not perform at least one
omnibox search before the init delay, the code below will try record rlz events.

I think its OK though since install events are stateful.

[jochen (gone - plz use gerrit), 2011/01/13 09:25:25]

an alternative would be to add a callback parameter to GetAccessPointRlz. If the
callback is non-NULL it will be invoked with the correct result. Otherwise, an
empty string is returned if no access point is cached.

[Roger Tawa OOO till Jul 10th, 2011/01/13 15:09:55]

Thinking about it some, I don't think this is a problem.  RLZ will take care of
not recording this more than once.  The code is fine as is.

[pastarmovj, 2011/01/13 15:34:02]

In general I am not happy leaving this function like that because it still does
IO on the UI thread but given that it is one time event it shouldn't be so bad. 
We can do as you propose and add a Callback option to the call or alternatively
we could make this call repost itself second time with a slight delay if the
GetAccessPointRlz returns false the first time. However RLZ does statefully keep
the INSTALL event than this is unnecessary move.

[Roger Tawa OOO till Jul 10th, 2011/01/13 15:42:17]

Is there a need to post DelayedInitTask to the UI thread?  Looking at the Run()
it does not seem like there is any dependency.  Could the DelayedInitTask be
posted to the IO thread instead?  See line 262 below.

 
     if (first_run_ || omnibox_rlz.empty()) {
       // Record the installation of chrome.
       RLZTracker::RecordProductEvent(rlz_lib::CHROME,
                                      rlz_lib::CHROME_OMNIBOX,
                                      rlz_lib::INSTALL);
       RLZTracker::RecordProductEvent(rlz_lib::CHROME,
                                      rlz_lib::CHROME_HOME_PAGE,
                                      rlz_lib::INSTALL);
       // Record if google is the initial search provider.
@@ skipping 77 matching lines @@
 // cached value.
 
 bool RLZTracker::GetAccessPointRlz(rlz_lib::AccessPoint point,
                                    std::wstring* rlz) {
   static std::wstring cached_ommibox_rlz;
   if ((rlz_lib::CHROME_OMNIBOX == point) &&
       (access_values_state == ACCESS_VALUES_FRESH)) {
     *rlz = cached_ommibox_rlz;
     return true;
   }
+
+  // Make sure we don't access disk outside of the file context.
+  // In such case we repost the task on the right thread and return error.
+  if (!BrowserThread::CurrentlyOn(BrowserThread::FILE)) {
+    // Caching of access points is now only implemented for the CHROME_OMNIBOX.
+    // Thus it is not possible to call this function on another thread for
+    // other access points until proper caching for these has been implemented
+    // and the code that calls this function can handle synchronous fetching
+    // of the access point.
+    DCHECK_EQ(rlz_lib::CHROME_OMNIBOX, point);
+
+    BrowserThread::PostTask(
+        BrowserThread::FILE, FROM_HERE,
+        NewRunnableFunction(&RLZTracker::GetAccessPointRlz,
+                            point, &cached_ommibox_rlz));
+      rlz->erase();
+      return false;
+  }
+
   char str_rlz[kMaxRlzLength + 1];
   if (!rlz_lib::GetAccessPointRlz(point, str_rlz, rlz_lib::kMaxRlzLength, NULL))
     return false;
   *rlz = ASCIIToWide(std::string(str_rlz));
   if (rlz_lib::CHROME_OMNIBOX == point) {
     access_values_state = ACCESS_VALUES_FRESH;
     cached_ommibox_rlz.assign(*rlz);
   }
   return true;
 }
 
 // static
 void RLZTracker::CleanupRlz() {
   OmniBoxUsageObserver::DeleteInstance();
 }