Fix Android strict-mode violation in GeoLocation info bar.

chrome/browser/geolocation/chrome_geolocation_permission_context.cc

 // Copyright (c) 2012 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.
 
 #include "chrome/browser/geolocation/chrome_geolocation_permission_context.h"
 
 #include <functional>
 #include <string>
 #include <vector>
 
 #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"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_system.h"
 #include "chrome/browser/extensions/suggest_permission_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/tab_contents/tab_util.h"
 #include "chrome/common/extensions/extension.h"
 #include "content/public/browser/browser_thread.h"
@@ skipping 73 matching lines @@
 
   GURL embedder = web_contents->GetURL();
   if (!requesting_frame.is_valid() || !embedder.is_valid()) {
     LOG(WARNING) << "Attempt to use geolocation from an invalid URL: "
                  << requesting_frame << "," << embedder
                  << " (geolocation is not supported in popups)";
     NotifyPermissionSet(id, requesting_frame, callback, false);
     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(
     int render_process_id,
     int render_view_id,
     int bridge_id,
     const GURL& requesting_frame) {
   CancelPendingInfoBarRequest(PermissionRequestID(
       render_process_id, render_view_id, bridge_id));
 }
 
 void ChromeGeolocationPermissionContext::DecidePermission(
     const PermissionRequestID& id,
     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.

           requesting_frame, embedder, CONTENT_SETTINGS_TYPE_GEOLOCATION,
           std::string());
   switch (content_setting) {
     case CONTENT_SETTING_BLOCK:
       PermissionDecided(id, requesting_frame, embedder, callback, false);
       break;
     case CONTENT_SETTING_ALLOW:
       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() {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   permission_queue_controller_.reset();
   shutting_down_ = true;
 }
 
 void ChromeGeolocationPermissionContext::PermissionDecided(
     const PermissionRequestID& id,
     const GURL& requesting_frame,
     const GURL& embedder,
     base::Callback<void(bool)> callback,
     bool allowed) {
   NotifyPermissionSet(id, requesting_frame, callback, allowed);
 }
 
 void ChromeGeolocationPermissionContext::NotifyPermissionSet(
     const PermissionRequestID& id,
     const GURL& requesting_frame,
     base::Callback<void(bool)> callback,
     bool allowed) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));

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

you call this from  ChromeGeolocationPermissionContext::PermissionDecided()
which is called from ChromeGeolocationPermissionContext::DecidePermission()
which is not on UI thread. How can this not assert?

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

Good catch! This should have assert if master location is not enabled.

I changed this back to UI thread as well.

 
   // WebContents may have gone away (or not exists for extension).
   TabSpecificContentSettings* content_settings =
       TabSpecificContentSettings::Get(id.render_process_id(),
                                       id.render_view_id());
   if (content_settings) {
     content_settings->OnGeolocationPermissionSet(requesting_frame.GetOrigin(),
                                                  allowed);
   }
 
   callback.Run(allowed);
 }
 
 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());
   return permission_queue_controller_.get();
 }
 
 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);
 }
 
 void ChromeGeolocationPermissionContext::CancelPendingInfoBarRequest(
     const PermissionRequestID& id) {
   if (!content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)) {
     content::BrowserThread::PostTask(
         content::BrowserThread::UI, FROM_HERE,
         base::Bind(
             &ChromeGeolocationPermissionContext::CancelPendingInfoBarRequest,
             this, id));
      return;
   }
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   if (shutting_down_)
     return;
   QueueController()->CancelInfoBarRequest(id);
 }