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 126 matching lines @@
 // Performs late RLZ initialization and RLZ event recording for chrome.
 // This task needs to run on the UI thread.
 class DelayedInitTask : public Task {
  public:
   explicit DelayedInitTask(bool first_run)
       : first_run_(first_run) {
   }
   virtual ~DelayedInitTask() {
   }
   virtual void Run() {
-    // Needs to be evaluated. See http://crbug.com/62328.
-    base::ThreadRestrictions::ScopedAllowIO allow_io;
-
     // For non-interactive tests we don't do the rest of the initialization
     // because sometimes the very act of loading the dll causes QEMU to crash.
     if (::GetEnvironmentVariableW(ASCIIToWide(env_vars::kHeadless).c_str(),
                                   NULL, 0)) {
       return;
     }
     // 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() ||
@@ skipping 21 matching lines @@
       }
     }
     // Record first user interaction with the omnibox. We call this all the
     // time but the rlz lib should ingore all but the first one.
     if (OmniBoxUsageObserver::used()) {
       RLZTracker::RecordProductEvent(rlz_lib::CHROME,
                                      rlz_lib::CHROME_OMNIBOX,
                                      rlz_lib::FIRST_SEARCH);
     }
     // Schedule the daily RLZ ping.
-    base::Thread* thread = g_browser_process->file_thread();
-    if (thread)
-      thread->message_loop()->PostTask(FROM_HERE, new DailyPingTask());
+    MessageLoop::current()->PostTask(FROM_HERE, new DailyPingTask());
   }
 
  private:
   bool IsGoogleDefaultSearch() {
     if (!g_browser_process)
       return false;
     FilePath user_data_dir;
     if (!PathService::Get(chrome::DIR_USER_DATA, &user_data_dir))
       return false;
     ProfileManager* profile_manager = g_browser_process->profile_manager();
@@ skipping 23 matching lines @@
   const int kMinDelay = 20 * 1000;
 
   delay *= 1000;
   delay = (delay < kMinDelay) ? kMinDelay : delay;
   delay = (delay > kMaxDelay) ? kMaxDelay : delay;
 
   if (!OmniBoxUsageObserver::used())
     new OmniBoxUsageObserver();
 
   // Schedule the delayed init items.
-  MessageLoop::current()->PostDelayedTask(FROM_HERE,
-      new DelayedInitTask(first_run), delay);
+  BrowserThread::PostDelayedTask(
+      BrowserThread::FILE, FROM_HERE, new DelayedInitTask(first_run), delay);
   return true;
 }
 
 bool RLZTracker::RecordProductEvent(rlz_lib::Product product,
                                     rlz_lib::AccessPoint point,
                                     rlz_lib::Event event_id) {
   return rlz_lib::RecordProductEvent(product, point, event_id);
 }
 
 bool RLZTracker::ClearAllProductEvents(rlz_lib::Product product) {
   return rlz_lib::ClearAllProductEvents(product);
 }
 
 // We implement caching of the answer of get_access_point() if the request
 // is for CHROME_OMNIBOX. If we had a successful ping, then we update the
 // 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;

[Roger Tawa OOO till Jul 10th, 2011/01/18 21:00:14]

there is now the possibility that multiple threads will touch the
access_values_state and cached_ommibox_rlz variables.  Shouldn't there a lock to
protect them?  (there was a possibility before too but it seems it may happen
more often now)

[pastarmovj, 2011/01/18 21:10:20]

I don't think this is possible. This code is only reached from the FILE thread
because of the new addition. The other case this value is written is the PingNow
function which is also only called through a task post on the FILE thread
because of the last edit where we moved the PostDelayedTask to the file thread
instead of the UI where it used to be before.
Or am I skipping there something?

On 2011/01/18 21:00:14, Roger Tawa wrote:

[Roger Tawa OOO till Jul 10th, 2011/01/19 01:57:52]

The ui thread can access access_values_state and cached_ommibox_rlz at line 280
above, no?  so its possible for the file thread (or pingnow thread) to be
modifying one or both values while the ui thread is trying to read them.

[pastarmovj, 2011/01/19 08:34:18]

Ah you meant between read and write access in the same function. Sure this can
happen. Actually I'd only swap lines 308/309 as we can safely assume that the
int assignment is atomic so we only want to allow reading of the cache value
_after_ it has been initialized.

On 2011/01/19 01:57:52, Roger Tawa wrote:

     cached_ommibox_rlz.assign(*rlz);
   }
   return true;
 }
 
 // static
 void RLZTracker::CleanupRlz() {
   OmniBoxUsageObserver::DeleteInstance();
 }