Fix geolocation crashing bug

- only inform arbitrator of permission being set if thread is started and arbitrator created.
- Also ensure any cached permission is passed though to the arbitrator when it is started.

BUG=59377
TEST=GeolocationProviderTest.OnPermissionGrantedWithoutObservers

Review URL: http://codereview.chromium.org/3872001

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@63194 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 62 insertions, 8 deletions

diff --git a/chrome/browser/geolocation/geolocation_provider.cc b/chrome/browser/geolocation/geolocation_provider.cc
index 5104d6a..97e9049 100644
--- a/chrome/browser/geolocation/geolocation_provider.cc
+++ b/chrome/browser/geolocation/geolocation_provider.cc
@@ -49,8 +49,11 @@ void GeolocationProvider::OnObserversChanged() {
   if (observers_.empty()) {
     Stop();
   } else {
-    if (!IsRunning())
+    if (!IsRunning()) {
       Start();
+      if (HasPermissionBeenGranted())
+        InformProvidersPermissionGranted(most_recent_authorized_frame_);
+    }
     // The high accuracy requirement may have changed.
     Task* task = NewRunnableMethod(this,
         &GeolocationProvider::SetObserverOptions,
@@ -75,26 +78,38 @@ void GeolocationProvider::NotifyObservers(const Geoposition& position) {
 void GeolocationProvider::SetObserverOptions(
     const GeolocationObserverOptions& options) {
   DCHECK(OnGeolocationThread());
-  if (!arbitrator_)
-    arbitrator_ = GeolocationArbitrator::Create(this);
+  DCHECK(arbitrator_);
   arbitrator_->SetObserverOptions(options);
 }
 
 void GeolocationProvider::OnPermissionGranted(const GURL& requesting_frame) {
+  DCHECK(OnClientThread());
+  most_recent_authorized_frame_ = requesting_frame;
+  if (IsRunning())
+    InformProvidersPermissionGranted(requesting_frame);
+}
+
+void GeolocationProvider::InformProvidersPermissionGranted(
+    const GURL& requesting_frame) {
+  DCHECK(IsRunning());
+  DCHECK(requesting_frame.is_valid());
   if (!OnGeolocationThread()) {
-    DCHECK(OnClientThread());
-    most_recent_authorized_frame_ = requesting_frame;
-    Task* task = NewRunnableMethod(this,
-        &GeolocationProvider::OnPermissionGranted, requesting_frame);
+    Task* task = NewRunnableMethod(
+        this,
+        &GeolocationProvider::InformProvidersPermissionGranted,
+        requesting_frame);
     message_loop()->PostTask(FROM_HERE, task);
     return;
   }
   DCHECK(OnGeolocationThread());
+  DCHECK(arbitrator_);
   arbitrator_->OnPermissionGranted(requesting_frame);
 }
 
 void GeolocationProvider::Init() {
   DCHECK(OnGeolocationThread());
+  DCHECK(!arbitrator_);
+  arbitrator_ = GeolocationArbitrator::Create(this);
 }
 
 void GeolocationProvider::CleanUp() {
diff --git a/chrome/browser/geolocation/geolocation_provider.h b/chrome/browser/geolocation/geolocation_provider.h
index ae10093..9420f57 100644
--- a/chrome/browser/geolocation/geolocation_provider.h
+++ b/chrome/browser/geolocation/geolocation_provider.h
@@ -62,6 +62,8 @@ class GeolocationProvider : public base::Thread, public GeolocationObserver {
   void OnObserversChanged();
   // Passes the observers' geolocation options through to the arbitrator.
   void SetObserverOptions(const GeolocationObserverOptions& options);
+  // Update the providers on the geolocation thread, which must be running.
+  void InformProvidersPermissionGranted(const GURL& requesting_frame);
   // Notifies observers when a new position fix is available.
   void NotifyObservers(const Geoposition& position);
 
diff --git a/chrome/browser/geolocation/geolocation_provider_unittest.cc b/chrome/browser/geolocation/geolocation_provider_unittest.cc
new file mode 100644
index 0000000..ec9cc96
--- /dev/null
+++ b/chrome/browser/geolocation/geolocation_provider_unittest.cc
@@ -0,0 +1,38 @@
+// Copyright (c) 2010 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/geolocation_provider.h"
+
+#include "base/singleton.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+class GeolocationProviderTest : public testing::Test {
+ protected:
+  // TODO(joth): This test should inject a mock arbitrator rather than cause
+  // the real one to be instantiated. For now, this is not an issue as the
+  // arbitrator is never started.
+  GeolocationProviderTest()
+      : provider_(new GeolocationProvider) {}
+
+  ~GeolocationProviderTest() {
+    DefaultSingletonTraits<GeolocationProvider>::Delete(provider_);
+  }
+  // Message loop for main thread, as provider depends on it existing.
+  MessageLoop message_loop_;
+
+  // Object under tests. Owned, but not a scoped_ptr due to specialized
+  // destruction protocol.
+  GeolocationProvider* provider_;
+};
+
+// Regression test for http://crbug.com/59377
+TEST_F(GeolocationProviderTest, OnPermissionGrantedWithoutObservers) {
+  EXPECT_FALSE(provider_->HasPermissionBeenGranted());
+  provider_->OnPermissionGranted(GURL("http://example.com"));
+  EXPECT_TRUE(provider_->HasPermissionBeenGranted());
+}
+
+}  // namespace
diff --git a/chrome/browser/geolocation/location_arbitrator.h b/chrome/browser/geolocation/location_arbitrator.h
index 590264f..6957cc2 100644
--- a/chrome/browser/geolocation/location_arbitrator.h
+++ b/chrome/browser/geolocation/location_arbitrator.h
@@ -22,7 +22,6 @@ class LocationProviderBase;
 class URLRequestContextGetter;
 struct Geoposition;
 
-
 // This class is responsible for handling updates from multiple underlying
 // providers and resolving them to a single 'best' location fix at any given
 // moment.