glom r2051 - in trunk: . glom glom/libglom glom/libglom/connectionpool_backends

[murrayc svn gnome org]

Author: murrayc
Date: Thu Apr  9 10:04:22 2009
New Revision: 2051
URL: http://svn.gnome.org/viewvc/glom?rev=2051&view=rev

Log:
2009-04-09  Michael Hasselmann <michaelh openismus com>

* glom/libglom/connectionpool_backends/backend.h:
Introduced new error code FAILURE_NO_BACKEND which shall be used to
detect logical errors in the backend implementations.
* glom/libglom/connectionpool_backends/postgres_self.cc (connect):
Set error correctly if no self-hosted instance is active.
* glom/libglom/connectionpool.cc (connect, cleanup):
Added a g_return_val_if_fail to check whether there is a backend to
connect to. Also reset m_ready_to_connect flag to false during
cleanup, which stops Glom from behaving funnily if the user supplies
wrong credentials (see bug #577821):
- The user enters wrong credentials, gets a 'Connection failed' dialog
  the first time. Then, enters wrong credentials a second time, gets no
  dialog (but gets dialog for next try and so on).
- Glom would crash if the user tried to open another document, then decides
  to abort and only opens another document (much) later (which would
  look like a random crash then).
The ConnectionPool class could perhaps be made more robust concerning
that flag since it currently allows inconsistent states.

Modified:
   trunk/ChangeLog
   trunk/glom/base_db.cc
   trunk/glom/libglom/connectionpool.cc
   trunk/glom/libglom/connectionpool_backends/backend.h
   trunk/glom/libglom/connectionpool_backends/postgres_self.cc

Modified: trunk/glom/base_db.cc
==============================================================================
--- trunk/glom/base_db.cc	(original)
+++ trunk/glom/base_db.cc	Thu Apr  9 10:04:22 2009
@@ -358,6 +358,7 @@
 {
   if(get_document())
   {
+    // TODO: When is it *ever* correct to call fill_from_database() from here?
     if(ConnectionPool::get_instance()->get_ready_to_connect())
       fill_from_database(); //virtual.
 

Modified: trunk/glom/libglom/connectionpool.cc
==============================================================================
--- trunk/glom/libglom/connectionpool.cc	(original)
+++ trunk/glom/libglom/connectionpool.cc	Thu Apr  9 10:04:22 2009
@@ -324,6 +324,9 @@
 sharedptr<SharedConnection> ConnectionPool::connect(std::auto_ptr<ExceptionConnection>& error)
 #endif
 {
+  //Don't try to connect if we don't have a backend to connect to.
+  g_return_val_if_fail(m_backend.get(), sharedptr<SharedConnection>(0));
+
   if(get_ready_to_connect())
   {
     //If the connection is already open (because it is being used by somebody):
@@ -355,8 +358,7 @@
 #ifdef GLIBMM_EXCEPTIONS_ENABLED
       std::auto_ptr<ExceptionConnection> error;
 #endif
-      if(m_backend.get())
-        m_refGdaConnection = m_backend->connect(m_database, get_user(), get_password(), error);
+      m_refGdaConnection = m_backend->connect(m_database, get_user(), get_password(), error);
 
       if(!m_refGdaConnection)
       {
@@ -610,12 +612,22 @@
 
 void ConnectionPool::cleanup(const SlotProgress& slot_progress)
 {
+  // Without a valid backend instance we should not state that we are ready to
+  // connect. Fixes crash described in #577821.
+
+  // TODO: Implement checks in set_ready_to_connect() to only set the flag to
+  // "true" when the ConnectionPool instance is in a consistent state
+  // afterwards. Or, have a private set_ready_to_connect() and a public
+  // set_not_ready_to_connect()?
+  set_ready_to_connect(false);
+
   if(m_backend.get())
     m_backend->cleanup(slot_progress);
 
   //Make sure that connect() makes a new connection:
   connection_cached.clear();
 
+
 #ifndef G_OS_WIN32
   /* Stop advertising the self-hosting database server via avahi: */
   //avahi_stop_publishing();

Modified: trunk/glom/libglom/connectionpool_backends/backend.h
==============================================================================
--- trunk/glom/libglom/connectionpool_backends/backend.h	(original)
+++ trunk/glom/libglom/connectionpool_backends/backend.h	Thu Apr  9 10:04:22 2009
@@ -39,7 +39,8 @@
   enum failure_type
   {
     FAILURE_NO_SERVER, //Either there was no attempt to connect to a specific database, or the connection failed both with and without specifying the database.
-    FAILURE_NO_DATABASE //Connection without specifying the database was possible.
+    FAILURE_NO_DATABASE, //Connection without specifying the database was possible.
+    FAILURE_NO_BACKEND //No backend instance available. Should never happen.
   };
 
   ExceptionConnection(failure_type failure);
@@ -142,7 +143,9 @@
    * so we don't need #ifdefs all over the code. This part of the API is only
    * used by the ConnectionPool which will translate the error back into
    * an exception in case exceptions are enabled.
-    */
+   * If this method doesn't return a connection handle then error will be
+   * non-zero (and vice versa).
+   */
   virtual Glib::RefPtr<Gnome::Gda::Connection> connect(const Glib::ustring& database, const Glib::ustring& username, const Glib::ustring& password, std::auto_ptr<ExceptionConnection>& error) = 0;
 
 #ifndef GLOM_ENABLE_CLIENT_ONLY

Modified: trunk/glom/libglom/connectionpool_backends/postgres_self.cc
==============================================================================
--- trunk/glom/libglom/connectionpool_backends/postgres_self.cc	(original)
+++ trunk/glom/libglom/connectionpool_backends/postgres_self.cc	Thu Apr  9 10:04:22 2009
@@ -402,7 +402,10 @@
 Glib::RefPtr<Gnome::Gda::Connection> PostgresSelfHosted::connect(const Glib::ustring& database, const Glib::ustring& username, const Glib::ustring& password, std::auto_ptr<ExceptionConnection>& error)
 {
   if(!get_self_hosting_active())
+  {
+    error.reset(new ExceptionConnection(ExceptionConnection::FAILURE_NO_BACKEND));
     return Glib::RefPtr<Gnome::Gda::Connection>();
+  }
 
   return attempt_connect("localhost", port_as_string(m_port), database, username, password, error);