Fix Android strict-mode violation in GeoLocation info bar.

chrome/browser/geolocation/chrome_geolocation_permission_context.cc

Index: chrome/browser/geolocation/chrome_geolocation_permission_context.cc
diff --git a/chrome/browser/geolocation/chrome_geolocation_permission_context.cc b/chrome/browser/geolocation/chrome_geolocation_permission_context.cc
index e3bfc924f2cb2f1abdae72804f1179212c514c76..61c23cccf3d37e9d31135be316c8d0746c902495 100644
--- a/chrome/browser/geolocation/chrome_geolocation_permission_context.cc
+++ b/chrome/browser/geolocation/chrome_geolocation_permission_context.cc
@@ -10,6 +10,7 @@
 
 #include "base/bind.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/threading/worker_pool.h"
 #include "chrome/browser/content_settings/host_content_settings_map.h"
 #include "chrome/browser/content_settings/permission_request_id.h"
 #include "chrome/browser/content_settings/tab_specific_content_settings.h"
@@ -103,7 +104,13 @@ void ChromeGeolocationPermissionContext::RequestGeolocationPermission(
     return;
   }
 
-  DecidePermission(id, requesting_frame, embedder, callback);
+  // DecidePermission is a bit heavy. Avoid hogging the UI thread
+  // now that we are done with extension related stuff.
+  base::WorkerPool::PostTask(FROM_HERE,

[joth, 2013/08/20 23:07:21]

WorkerPool is a bit funny about non-joinable threads and what you can do on it.

It think the general preference is to use
BrowserThread::PostBlockingPoolTaskAndReply() these days.

[acleung1, 2013/08/23 23:13:09]

Ok. I removed this and keep the execution of these in the UI thread until later
to avoid the possible race conditions.

I'll use PostBlockingPoolTask later in the code.

+      base::Bind(
+          &ChromeGeolocationPermissionContext::DecidePermission,
+          this, id, requesting_frame, embedder, callback), true);
+  return;
 }
 
 void ChromeGeolocationPermissionContext::CancelGeolocationPermissionRequest(
@@ -120,7 +127,6 @@ void ChromeGeolocationPermissionContext::DecidePermission(
     const GURL& requesting_frame,
     const GURL& embedder,
     base::Callback<void(bool)> callback) {
-  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
 
   ContentSetting content_setting =
      profile_->GetHostContentSettingsMap()->GetContentSetting(

[joth, 2013/08/20 23:07:21]

This looks bad. Pretty sure profile_ is not thread safe, and I don't think
content settings map is either.

[acleung1, 2013/08/23 23:13:09]

Fixed. Kept in UI.

@@ -134,12 +140,24 @@ void ChromeGeolocationPermissionContext::DecidePermission(
       PermissionDecided(id, requesting_frame, embedder, callback, true);
       break;
     default:
-      // setting == ask. Prompt the user.
+      // setting == ask. Prompt the user in the UI thread now.
+      content::BrowserThread::PostTask(
+          content::BrowserThread::UI, FROM_HERE,
+          base::Bind(
+              &ChromeGeolocationPermissionContext::CreateInfoBarRequest,
+              base::Unretained(this), id, requesting_frame, embedder,callback));

[joth, 2013/08/20 23:07:21]

why use Unretained() here but not on line 112?

[acleung1, 2013/08/23 23:13:09]

Removed the early call and kept using unretained here as well.

+  }
+}
+
+void ChromeGeolocationPermissionContext::CreateInfoBarRequest(
+    const PermissionRequestID& id,
+    const GURL& requesting_frame,
+    const GURL& embedder,
+    base::Callback<void(bool)> callback) {
       QueueController()->CreateInfoBarRequest(
           id, requesting_frame, embedder, base::Bind(
               &ChromeGeolocationPermissionContext::NotifyPermissionSet,
               base::Unretained(this), id, requesting_frame, callback));
-  }
 }
 
 void ChromeGeolocationPermissionContext::ShutdownOnUIThread() {
@@ -178,7 +196,6 @@ void ChromeGeolocationPermissionContext::NotifyPermissionSet(
 
 PermissionQueueController*
     ChromeGeolocationPermissionContext::QueueController() {
-  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));

[Michael van Ouwerkerk, 2013/08/21 10:24:45]

Instead of deleting, could you modify such checks to check for the thread you
are actually expecting? Or at least assert which thread it is not on?

[acleung1, 2013/08/23 23:13:09]

I am moving this back to the UI thread as well.

As for asserting which thread this is not on, I am not sure if we should that
after Jonathan's comment on that being anti-pattern.

   DCHECK(!shutting_down_);
   if (!permission_queue_controller_)
     permission_queue_controller_.reset(CreateQueueController());
@@ -187,7 +204,6 @@ PermissionQueueController*
 
 PermissionQueueController*
     ChromeGeolocationPermissionContext::CreateQueueController() {
-  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   return new PermissionQueueController(profile(),

[joth, 2013/08/20 23:07:21]

again using profile on non-UI thread?

[acleung1, 2013/08/23 23:13:09]

fixed.

                                        CONTENT_SETTINGS_TYPE_GEOLOCATION);
 }