Bug 599969 - Do not use steps for async visit adding

Part 4 - use one event (ran on the background thread) for setting the page
title, and one event (ran on the main thread) for notifying.
r=mak

toolkit/components/places/src/History.cpp

@@ -43,11 +43,6 @@
 #include "nsXULAppAPI.h"
 #endif
 
-#ifdef MOZ_IPC
-#include "mozilla/dom/ContentChild.h"
-#include "nsXULAppAPI.h"
-#endif
-
 #include "History.h"
 #include "nsNavHistory.h"
 #include "nsNavBookmarks.h"
@@ -73,87 +68,6 @@ namespace places {
 // Observer event fired after a visit has been registered in the DB.
 #define URI_VISIT_SAVED "uri-visit-saved"
 
-////////////////////////////////////////////////////////////////////////////////
-//// Step
-
-class Step : public AsyncStatementCallback
-{
-public:
-  /**
-   * Executes statement asynchronously using this as a callback.
-   * 
-   * @param aStmt
-   *        Statement to execute asynchronously
-   */
-  NS_IMETHOD ExecuteAsync(mozIStorageStatement* aStmt);
-
-  /**
-   * Called once after query is completed.  If your query has more than one
-   * result set to process, you will want to override HandleResult to process
-   * each one.
-   *
-   * @param aResultSet
-   *        Results from ExecuteAsync
-   *        Unlike HandleResult, this *can be NULL* if there were no results.
-   */
-  NS_IMETHOD Callback(mozIStorageResultSet* aResultSet);
-
-  /**
-   * By default, stores the last result set received in mResultSet.
-   * For queries with only one result set, you don't need to override.
-   *
-   * @param aResultSet
-   *        Results from ExecuteAsync
-   */
-  NS_IMETHOD HandleResult(mozIStorageResultSet* aResultSet);
-
-  /**
-   * By default, this calls Callback with any saved results from HandleResult.
-   * For queries with only one result set, you don't need to override.
-   *
-   * @param aReason
-   *        SQL status code
-   */
-  NS_IMETHOD HandleCompletion(PRUint16 aReason);
-
-private:
-  // Used by HandleResult to cache results until HandleCompletion is called.
-  nsCOMPtr<mozIStorageResultSet> mResultSet;
-};
-
-NS_IMETHODIMP
-Step::ExecuteAsync(mozIStorageStatement* aStmt)
-{
-  nsCOMPtr<mozIStoragePendingStatement> handle;
-  nsresult rv = aStmt->ExecuteAsync(this, getter_AddRefs(handle));
-  NS_ENSURE_SUCCESS(rv, rv);
-  return NS_OK;
-}
-
-NS_IMETHODIMP
-Step::Callback(mozIStorageResultSet* aResultSet)
-{
-  return NS_OK;
-}
-
-NS_IMETHODIMP
-Step::HandleResult(mozIStorageResultSet* aResultSet)
-{
-  mResultSet = aResultSet;
-  return NS_OK;
-}
-
-NS_IMETHODIMP
-Step::HandleCompletion(PRUint16 aReason)
-{
-  if (aReason == mozIStorageStatementCallback::REASON_FINISHED) {
-    nsCOMPtr<mozIStorageResultSet> resultSet = mResultSet;
-    mResultSet = NULL;
-    Callback(resultSet);
-  }
-  return NS_OK;
-}
-
 ////////////////////////////////////////////////////////////////////////////////
 //// Anonymous Helpers
 
@@ -300,6 +214,8 @@ public:
   : mPlace(aPlace)
   , mReferrer(aReferrer)
   {
+    NS_PRECONDITION(!NS_IsMainThread(),
+                    "This should not be called on the main thread");
   }
 
   NS_IMETHOD Run()
@@ -361,7 +277,6 @@ public:
 
     nsRefPtr<InsertVisitedURI> event =
       new InsertVisitedURI(aConnection, aPlace, aReferrer);
-    NS_ENSURE_TRUE(event, NS_ERROR_OUT_OF_MEMORY);
 
     // Speculatively get a new session id for our visit.  While it is true that
     // we will use the session id from the referrer if the visit was "recent"
@@ -476,7 +391,6 @@ public:
 
     // Finally, dispatch an event to the main thread to notify observers.
     nsCOMPtr<nsIRunnable> event = new NotifyVisitObservers(mPlace, mReferrer);
-    NS_ENSURE_TRUE(event, NS_ERROR_OUT_OF_MEMORY);
     rv = NS_DispatchToMainThread(event);
     NS_ENSURE_SUCCESS(rv, rv);
 
@@ -714,172 +628,211 @@ private:
 };
 
 /**
- * Fail-safe mechanism for ensuring that your task completes, no matter what.
+ * Notifies observers about a pages title changing.
- * Pass this around as an nsAutoPtr in your steps to guarantee that when all
- * your steps are finished, your task is finished.
- *
- * Be sure to use AppendTask to add your first step to the queue.
  */
-class FailSafeFinishTask
+class NotifyTitleObservers : public nsRunnable
 {
 public:
-  FailSafeFinishTask()
-  : mAppended(false)
-  {
-  }
-
-  ~FailSafeFinishTask()
-  {
-    if (mAppended) {
-      History::GetService()->CurrentTaskFinished();
-    }
-  }
-
   /**
-   * Appends task to History's queue.  When this object is destroyed, it will
+   * Notifies observers on the main thread if we need to, and releases the
-   * consider the task finished.
+   * URI (necessary to do on the main thread).
+   *
+   * @param aNotify
+   *        True if we should notify, false if not.
+   * @param aURI
+   *        Reference to the nsCOMPtr that owns the nsIURI object describing the
+   *        page we set the title on.  This will be null after this object is
+   *        constructed.
+   * @param aTitle
+   *        The new title to notify about.
    */
-  void AppendTask(Step* step)
+  NotifyTitleObservers(bool aNotify,
+                       nsCOMPtr<nsIURI>& aURI,
+                       const nsString& aTitle)
+  : mNotify(aNotify)
+  , mTitle(aTitle)
   {
-    History::GetService()->AppendTask(step);
+    NS_PRECONDITION(!NS_IsMainThread(),
-    mAppended = true;
+                    "This should not be called on the main thread");
+
+    // Do not want to AddRef and Release on the background thread!
+    mURI.swap(aURI);
+  }
+
+  NS_IMETHOD Run()
+  {
+    NS_PRECONDITION(NS_IsMainThread(),
+                    "This should be called on the main thread");
+
+    if (!mNotify) {
+      return NS_OK;
+    }
+
+    nsNavHistory* navhistory = nsNavHistory::GetHistoryService();
+    NS_ENSURE_TRUE(navhistory, NS_ERROR_OUT_OF_MEMORY);
+    navhistory->NotifyTitleChange(mURI, mTitle);
+
+    return NS_OK;
+  }
+private:
+  const bool mNotify;
+  nsCOMPtr<nsIURI> mURI;
+  const nsString mTitle;
+};
+
+
+/**
+ * Sets the page title for a page in moz_places (if necessary).
+ */
+class SetPageTitle : public nsRunnable
+{
+public:
+  /**
+   * Sets a pages title in the database asynchronously.
+   *
+   * @param aConnection
+   *        The database connection to use for this operation.
+   * @param aURI
+   *        The URI to set the page title on.
+   * @param aTitle
+   *        The title to set for the page, if the page exists.
+   */
+  static nsresult Start(mozIStorageConnection* aConnection,
+                        nsIURI* aURI,
+                        const nsString& aTitle)
+  {
+    NS_PRECONDITION(NS_IsMainThread(),
+                    "This should be called on the main thread");
+
+    nsRefPtr<SetPageTitle> event = new SetPageTitle(aConnection, aURI, aTitle);
+
+    // Get the target thread, and then start the work!
+    nsCOMPtr<nsIEventTarget> target = do_GetInterface(aConnection);
+    NS_ENSURE_TRUE(target, NS_ERROR_UNEXPECTED);
+    nsresult rv = target->Dispatch(event, NS_DISPATCH_NORMAL);
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    return NS_OK;
+  }
+
+  NS_IMETHOD Run()
+  {
+    NS_PRECONDITION(!NS_IsMainThread(),
+                    "This should not be called on the main thread");
+
+    // First, see if the page exists in the database (we'll need its id later).
+    nsCOMPtr<mozIStorageStatement> stmt =
+      mHistory->syncStatements.GetCachedStatement(
+        "SELECT id, title "
+        "FROM moz_places "
+        "WHERE url = :page_url "
+      );
+    NS_ENSURE_STATE(stmt);
+
+    PRInt64 placeId = 0;
+    nsAutoString title;
+    {
+      mozStorageStatementScoper scoper(stmt);
+      nsresult rv = URIBinder::Bind(stmt, NS_LITERAL_CSTRING("page_url"), mURI);
+      NS_ENSURE_SUCCESS(rv, rv);
+
+      PRBool hasResult;
+      rv = stmt->ExecuteStep(&hasResult);
+      NS_ENSURE_SUCCESS(rv, rv);
+      if (!hasResult) {
+        // We have no record of this page, so there is no need to do any further
+        // work.
+        return Finish(false);
+      }
+
+      rv = stmt->GetInt64(0, &placeId);
+      NS_ENSURE_SUCCESS(rv, rv);
+
+      rv = stmt->GetString(1, title);
+      NS_ENSURE_SUCCESS(rv, rv);
+    }
+
+    NS_ASSERTION(placeId > 0, "We somehow have an invalid place id here!");
+
+    // Also, if we have the same title, there is no reason to do another write
+    // or notify our observers, so bail early.
+    if (mTitle.Equals(title) || (mTitle.IsVoid() && title.IsVoid())) {
+      return Finish(false);
+    }
+
+    // Now we can update our database record.
+    stmt = mHistory->syncStatements.GetCachedStatement(
+        "UPDATE moz_places "
+        "SET title = :page_title "
+        "WHERE id = :page_id "
+      );
+    NS_ENSURE_STATE(stmt);
+
+    {
+      mozStorageStatementScoper scoper(stmt);
+      nsresult rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("page_id"),
+                                          placeId);
+      NS_ENSURE_SUCCESS(rv, rv);
+      if (mTitle.IsVoid()) {
+        rv = stmt->BindNullByName(NS_LITERAL_CSTRING("page_title"));
+      }
+      else {
+        rv = stmt->BindStringByName(NS_LITERAL_CSTRING("page_title"),
+                                    StringHead(mTitle, TITLE_LENGTH_MAX));
+      }
+      NS_ENSURE_SUCCESS(rv, rv);
+      rv = stmt->Execute();
+      NS_ENSURE_SUCCESS(rv, rv);
+    }
+
+    nsresult rv = Finish(true);
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    return NS_OK;
   }
 
 private:
-  bool mAppended;
+  SetPageTitle(mozIStorageConnection* aConnection,
-};
+               nsIURI* aURI,
-////////////////////////////////////////////////////////////////////////////////
+               const nsString& aTitle)
-//// Steps for SetURITitle
+  : mDBConn(aConnection)
-
+  , mURI(aURI)
-struct SetTitleData : public FailSafeFinishTask
+  , mTitle(aTitle)
-{
+  , mHistory(History::GetService())
-  nsCOMPtr<nsIURI> uri;
-  nsString title;
-};
-
-/**
- * Step 3: Notify that title has been updated.
- */
-class TitleNotifyStep: public Step
-{
-public:
-  TitleNotifyStep(nsAutoPtr<SetTitleData> aData)
-  : mData(aData)
   {
   }
 
-  NS_IMETHOD Callback(mozIStorageResultSet* aResultSet)
+  /**
+   * Finishes our work by dispatching an event back to the main thread.
+   *
+   * @param aNotify
+   *        True if we should notify observers, false otherwise.
+   */
+  nsresult Finish(bool aNotify)
   {
-    nsNavHistory* history = nsNavHistory::GetHistoryService();
+    // We always dispatch this event because we have to release mURI on the
-    NS_ENSURE_TRUE(history, NS_ERROR_OUT_OF_MEMORY);
+    // main thread.
-    history->NotifyTitleChange(mData->uri, mData->title);
+    nsCOMPtr<nsIRunnable> event =
+      new NotifyTitleObservers(aNotify, mURI, mTitle);
+    nsresult rv = NS_DispatchToMainThread(event);
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    NS_ASSERTION(!mURI,
+                 "We did not let go of our nsIURI reference after notifying!");
 
     return NS_OK;
   }
 
-protected:
+  mozIStorageConnection* mDBConn;
-  nsAutoPtr<SetTitleData> mData;
-};
 
-/**
+  nsCOMPtr<nsIURI> mURI;
- * Step 2: Set title.
+  const nsString mTitle;
- */
-class SetTitleStep : public Step
-{
-public:
-  SetTitleStep(nsAutoPtr<SetTitleData> aData)
-  : mData(aData)
-  {
-  }
 
-  NS_IMETHOD Callback(mozIStorageResultSet* aResultSet)
+  /**
-  {
+   * Strong reference to the History object because we do not want it to
-    if (!aResultSet) {
+   * disappear out from under us.
-      // URI record was not found.
+   */
-      return NS_OK;
+  nsRefPtr<History> mHistory;
-    }
-
-    nsCOMPtr<mozIStorageRow> row;
-    nsresult rv = aResultSet->GetNextRow(getter_AddRefs(row));
-    NS_ENSURE_SUCCESS(rv, rv);
-
-    nsAutoString title;
-    rv = row->GetString(2, title);
-    NS_ENSURE_SUCCESS(rv, rv);
-
-    // It is actually common to set the title to be the same thing it used to
-    // be. For example, going to any web page will always cause a title to be set,
-    // even though it will often be unchanged since the last visit. In these
-    // cases, we can avoid DB writing and observer overhead.
-    if (mData->title.Equals(title) || (mData->title.IsVoid() && title.IsVoid()))
-      return NS_OK;
-
-    nsNavHistory* history = nsNavHistory::GetHistoryService();
-    NS_ENSURE_TRUE(history, NS_ERROR_OUT_OF_MEMORY);
-
-    nsCOMPtr<mozIStorageStatement> stmt =
-      history->GetStatementById(DB_SET_PLACE_TITLE);
-    NS_ENSURE_STATE(stmt);
-
-    if (mData->title.IsVoid()) {
-      rv = stmt->BindNullByName(NS_LITERAL_CSTRING("page_title"));
-    }
-    else {
-      rv = stmt->BindStringByName(
-        NS_LITERAL_CSTRING("page_title"),
-        StringHead(mData->title, TITLE_LENGTH_MAX)
-      );
-    }
-    NS_ENSURE_SUCCESS(rv, rv);
-
-    rv = URIBinder::Bind(stmt, NS_LITERAL_CSTRING("page_url"), mData->uri);
-    NS_ENSURE_SUCCESS(rv, rv);
-
-    nsCOMPtr<Step> step = new TitleNotifyStep(mData);
-    rv = step->ExecuteAsync(stmt);
-    NS_ENSURE_SUCCESS(rv, rv);
-
-    return NS_OK;
-  }
-
-protected:
-  nsAutoPtr<SetTitleData> mData;
-};
-
-/**
- * Step 1: See if there is an existing URI.
- */
-class StartSetURITitleStep : public Step
-{
-public:
-  StartSetURITitleStep(nsAutoPtr<SetTitleData> aData)
-  : mData(aData)
-  {
-    mData->AppendTask(this);
-  }
-
-  NS_IMETHOD Callback(mozIStorageResultSet* aResultSet)
-  {
-    nsNavHistory* history = nsNavHistory::GetHistoryService();
-    NS_ENSURE_TRUE(history, NS_ERROR_OUT_OF_MEMORY);
-
-    // Find existing entry in moz_places table, if any.
-    nsCOMPtr<mozIStorageStatement> stmt =
-      history->GetStatementById(DB_GET_URL_PAGE_INFO);
-    NS_ENSURE_STATE(stmt);
-
-    nsresult rv = URIBinder::Bind(stmt, NS_LITERAL_CSTRING("page_url"), mData->uri);
-    NS_ENSURE_SUCCESS(rv, rv);
-
-    nsCOMPtr<Step> step = new SetTitleStep(mData);
-    rv = step->ExecuteAsync(stmt);
-    NS_ENSURE_SUCCESS(rv, rv);
-
-    return NS_OK;
-  }
-
-protected:
-  nsAutoPtr<SetTitleData> mData;
 };
 
 } // anonymous namespace
@@ -919,38 +872,6 @@ History::~History()
   Shutdown();
 }
 
-void
-History::AppendTask(Step* aTask)
-{
-  NS_PRECONDITION(aTask, "Got NULL task.");
-
-  if (mShuttingDown) {
-    return;
-  }
-
-  NS_ADDREF(aTask);
-  mPendingVisits.Push(aTask);
-
-  if (mPendingVisits.GetSize() == 1) {
-    // There are no other pending tasks.
-    StartNextTask();
-  }
-}
-
-void
-History::CurrentTaskFinished()
-{
-  if (mShuttingDown) {
-    return;
-  }
-
-  NS_ASSERTION(mPendingVisits.PeekFront(), "Tried to finish task not on the queue");
-
-  nsCOMPtr<Step> deadTaskWalking =
-    dont_AddRef(static_cast<Step*>(mPendingVisits.PopFront()));
-  StartNextTask();
-}
-
 void
 History::NotifyVisited(nsIURI* aURI)
 {
@@ -1063,33 +984,11 @@ History::GetDBConn()
   return mDBConn;
 }
 
-void
-History::StartNextTask()
-{
-  if (mShuttingDown) {
-    return;
-  }
-
-  nsCOMPtr<Step> nextTask =
-    static_cast<Step*>(mPendingVisits.PeekFront());
-  if (!nextTask) {
-    // No more pending visits left to process.
-    return;
-  }
-  nsresult rv = nextTask->Callback(NULL);
-  NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "Beginning a task failed.");
-}
-
 void
 History::Shutdown()
 {
   mShuttingDown = true;
 
-  while (mPendingVisits.PeekFront()) {
-    nsCOMPtr<Step> deadTaskWalking =
-      dont_AddRef(static_cast<Step*>(mPendingVisits.PopFront()));
-  }
-
   // Clean up our statements and connection.
   syncStatements.FinalizeStatements();
 
@@ -1340,19 +1239,19 @@ History::SetURITitle(nsIURI* aURI, const nsAString& aTitle)
     return NS_OK;
   }
 
-  nsAutoPtr<SetTitleData> data(new SetTitleData());
+  nsAutoString title;
-  NS_ENSURE_STATE(data);
-
-  data->uri = aURI;
-
   if (aTitle.IsEmpty()) {
-    data->title.SetIsVoid(PR_TRUE);
+    title.SetIsVoid(PR_TRUE);
   }
   else {
-    data->title.Assign(aTitle);
+    title.Assign(aTitle);
   }
 
-  nsCOMPtr<Step> task(new StartSetURITitleStep(data));
+  mozIStorageConnection* dbConn = GetDBConn();
+  NS_ENSURE_STATE(dbConn);
+
+  rv = SetPageTitle::Start(dbConn, aURI, title);
+  NS_ENSURE_SUCCESS(rv, rv);
 
   return NS_OK;
 }

toolkit/components/places/src/History.h

@@ -75,26 +75,6 @@ public:
    */
   void NotifyVisited(nsIURI* aURI);
 
-  /**
-   * Append a task to the queue for SQL queries that need to happen
-   * atomically.
-   *
-   * @pre aTask is not null
-   *
-   * @param aTask
-   *        Task that needs to be completed atomically
-   */
-  void AppendTask(class Step* aTask);
-
-  /**
-   * Call when all steps of the current running task are finished.  Each task
-   * should be responsible for calling this when it is finished (even if there
-   * are errors).
-   *
-   * Do not call this twice for the same visit.
-   */
-  void CurrentTaskFinished();
-
   /**
    * Obtains the statement to use to check if a URI is visited or not.
    */
@@ -147,26 +127,6 @@ private:
    */
   nsCOMPtr<mozIStorageAsyncStatement> mIsVisitedStatement;
 
-  /**
-   * Since visits rapidly fire at once, it's very likely to have race
-   * conditions for SQL queries.  We often need to see if a row exists
-   * or peek at values, and by the time we have retrieved them they could
-   * be different.
-   *
-   * We guarantee an ordering of our SQL statements so that a set of
-   * callbacks for one visit are guaranteed to be atomic.  Each visit consists
-   * of a data structure that sits in this queue.
-   *
-   * The front of the queue always has the current visit we are processing.
-   */
-  nsDeque mPendingVisits;
-
-  /**
-   * Begins next task at the front of the queue.  The task remains in the queue
-   * until it is done and calls CurrentTaskFinished.
-   */
-  void StartNextTask();
-
   /**
    * Remove any memory references to tasks and do not take on any more.
    */