Bug 599973 (Part 2) - Don't use steps for async favicons.
authorMarco Bonardo <mbonardo@mozilla.com>
Tue, 16 Nov 2010 02:22:01 +0100
changeset 59336 a4bd5f0ad56902e0e570af141bca72b46c7b3b78
parent 59335 999385f355a3a70f263c21b55be3bc89d678012c
child 59337 6617dcb0d48a98ceb139674d2542c6901b09192e
push id1
push usershaver@mozilla.com
push dateTue, 04 Jan 2011 17:58:04 +0000
bugs599973
milestone2.0b8pre
Bug 599973 (Part 2) - Don't use steps for async favicons. r=sdwilsh a=blocking
toolkit/components/places/src/AsyncFaviconHelpers.cpp
toolkit/components/places/src/Helpers.h
toolkit/components/places/src/nsFaviconService.cpp
toolkit/components/places/src/nsFaviconService.h
--- a/toolkit/components/places/src/AsyncFaviconHelpers.cpp
+++ b/toolkit/components/places/src/AsyncFaviconHelpers.cpp
@@ -63,29 +63,30 @@
  * The maximum time we will keep a favicon around.  We always ask the cache, if
  * we can, but default to this value if we do not get a time back, or the time
  * is more in the future than this.
  * Currently set to one week from now.
  */
 #define MAX_FAVICON_EXPIRATION ((PRTime)7 * 24 * 60 * 60 * PR_USEC_PER_SEC)
 
 using namespace mozilla::places;
+using namespace mozilla::storage;
 
 namespace {
 
 /**
  * Fetches information on a page from the Places database.
  *
  * @param aDBConn
  *        Database connection to history tables.
  * @param _page
  *        Page that should be fetched.
  */
 nsresult
-FetchPageInfo(nsCOMPtr<mozIStorageConnection>& aDBConn,
+FetchPageInfo(StatementCache<mozIStorageStatement>& aStmtCache,
               PageData& _page)
 {
   NS_PRECONDITION(_page.spec.Length(), "Must have a non-empty spec!");
   NS_PRECONDITION(!NS_IsMainThread(),
                   "This should not be called on the main thread");
 
   // This query fragment finds the bookmarked uri we want to set the icon for,
   // walking up to three redirect levels.  It is borrowed from
@@ -112,23 +113,24 @@ FetchPageInfo(nsCOMPtr<mozIStorageConnec
       nsINavHistoryService::TRANSITION_REDIRECT_PERMANENT,
       nsINavHistoryService::TRANSITION_REDIRECT_TEMPORARY,
       nsINavHistoryService::TRANSITION_REDIRECT_PERMANENT,
       nsINavHistoryService::TRANSITION_REDIRECT_TEMPORARY,
       nsINavHistoryService::TRANSITION_REDIRECT_PERMANENT,
       nsINavHistoryService::TRANSITION_REDIRECT_TEMPORARY
     );
 
-  nsCOMPtr<mozIStorageStatement> stmt;
-  aDBConn->CreateStatement(NS_LITERAL_CSTRING(
-    "SELECT h.id, h.favicon_id, "
-           "(") + redirectedBookmarksFragment + NS_LITERAL_CSTRING(") "
-    "FROM moz_places h WHERE h.url = :page_url"
-  ), getter_AddRefs(stmt));
+  nsCOMPtr<mozIStorageStatement> stmt =
+    aStmtCache.GetCachedStatement(NS_LITERAL_CSTRING(
+      "SELECT h.id, h.favicon_id, "
+             "(") + redirectedBookmarksFragment + NS_LITERAL_CSTRING(") "
+      "FROM moz_places h WHERE h.url = :page_url"
+    ));
   NS_ENSURE_STATE(stmt);
+  mozStorageStatementScoper scoper(stmt);
 
   nsresult rv = URIBinder::Bind(stmt, NS_LITERAL_CSTRING("page_url"),
                                 _page.spec);
   NS_ENSURE_SUCCESS(rv, rv);
 
   PRBool hasResult;
   rv = stmt->ExecuteStep(&hasResult);
   NS_ENSURE_SUCCESS(rv, rv);
@@ -165,17 +167,17 @@ FetchPageInfo(nsCOMPtr<mozIStorageConnec
       // history would be a privacy leak, bail out as if the page did not exist.
       return NS_ERROR_NOT_AVAILABLE;
     }
     else {
       // The page, or a redirect to it, is bookmarked.  If the bookmarked spec
       // is different from the requested one, use it.
       if (!_page.bookmarkedSpec.Equals(_page.spec)) {
         _page.spec = _page.bookmarkedSpec;
-        rv = FetchPageInfo(aDBConn, _page);
+        rv = FetchPageInfo(aStmtCache, _page);
         NS_ENSURE_SUCCESS(rv, rv);
       }
     }
   }
 
   return NS_OK;
 }
 
@@ -183,29 +185,30 @@ FetchPageInfo(nsCOMPtr<mozIStorageConnec
  * Fetches information on a icon from the Places database.
  *
  * @param aDBConn
  *        Database connection to history tables.
  * @param _icon
  *        Icon that should be fetched.
  */
 nsresult
-FetchIconInfo(nsCOMPtr<mozIStorageConnection>& aDBConn,
+FetchIconInfo(StatementCache<mozIStorageStatement>& aStmtCache,
               IconData& _icon)
 {
   NS_PRECONDITION(_icon.spec.Length(), "Must have a non-empty spec!");
   NS_PRECONDITION(!NS_IsMainThread(),
                   "This should not be called on the main thread");
 
-  nsCOMPtr<mozIStorageStatement> stmt;
-  aDBConn->CreateStatement(NS_LITERAL_CSTRING(
-    "SELECT id, expiration, data, mime_type "
-    "FROM moz_favicons WHERE url = :icon_url"
-  ), getter_AddRefs(stmt));
+  nsCOMPtr<mozIStorageStatement> stmt =
+    aStmtCache.GetCachedStatement(NS_LITERAL_CSTRING(
+      "SELECT id, expiration, data, mime_type "
+      "FROM moz_favicons WHERE url = :icon_url"
+    ));
   NS_ENSURE_STATE(stmt);
+  mozStorageStatementScoper scoper(stmt);
 
   nsresult rv = URIBinder::Bind(stmt, NS_LITERAL_CSTRING("icon_url"),
                                 _icon.spec);
   NS_ENSURE_SUCCESS(rv, rv);
 
   PRBool hasResult;
   rv = stmt->ExecuteStep(&hasResult);
   NS_ENSURE_SUCCESS(rv, rv);
@@ -454,17 +457,17 @@ AsyncFetchAndSetIconForPage::~AsyncFetch
 
 NS_IMETHODIMP
 AsyncFetchAndSetIconForPage::Run()
 {
   NS_PRECONDITION(!NS_IsMainThread(),
                   "This should not be called on the main thread");
 
   // Try to fetch the icon from the database.
-  nsresult rv = FetchIconInfo(mDBConn, mIcon);
+  nsresult rv = FetchIconInfo(mFaviconSvc->mSyncStatements, mIcon);
   NS_ENSURE_SUCCESS(rv, rv);
 
   bool isInvalidIcon = mIcon.data.IsEmpty() ||
                        (mIcon.expiration && PR_Now() > mIcon.expiration);
   bool fetchIconFromNetwork = mIcon.fetchMode == FETCH_ALWAYS ||
                               (mIcon.fetchMode == FETCH_IF_MISSING && isInvalidIcon);
 
   if (!fetchIconFromNetwork) {
@@ -695,98 +698,101 @@ AsyncAssociateIconToPage::~AsyncAssociat
 }
 
 NS_IMETHODIMP
 AsyncAssociateIconToPage::Run()
 {
   NS_PRECONDITION(!NS_IsMainThread(),
                   "This should not be called on the main thread");
 
-  nsresult rv = FetchPageInfo(mDBConn, mPage);
+  nsresult rv = FetchPageInfo(mFaviconSvc->mSyncStatements, mPage);
   if (rv == NS_ERROR_NOT_AVAILABLE){
     // We have never seen this page.  If we can add the page to history,
     // we will try to do it later, otherwise just bail out.
     if (!mPage.canAddToHistory) {
       return NS_OK;
     }
   }
   else {
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
   mozStorageTransaction transaction(mDBConn, PR_FALSE,
                                     mozIStorageConnection::TRANSACTION_IMMEDIATE);
 
   // If there is no entry for this icon, or the entry is obsolete, replace it.
   if (mIcon.id == 0 || (mIcon.status & ICON_STATUS_CHANGED)) {
-    nsCOMPtr<mozIStorageStatement> stmt;
-    mDBConn->CreateStatement(NS_LITERAL_CSTRING(
-      "INSERT OR REPLACE INTO moz_favicons "
-        "(id, url, data, mime_type, expiration) "
-      "VALUES ((SELECT id FROM moz_favicons WHERE url = :icon_url), "
-              ":icon_url, :data, :mime_type, :expiration) "
-    ), getter_AddRefs(stmt));
+    nsCOMPtr<mozIStorageStatement> stmt =
+      mFaviconSvc->mSyncStatements.GetCachedStatement(NS_LITERAL_CSTRING(
+        "INSERT OR REPLACE INTO moz_favicons "
+          "(id, url, data, mime_type, expiration) "
+        "VALUES ((SELECT id FROM moz_favicons WHERE url = :icon_url), "
+                ":icon_url, :data, :mime_type, :expiration) "
+      ));
     NS_ENSURE_STATE(stmt);
+    mozStorageStatementScoper scoper(stmt);
     rv = URIBinder::Bind(stmt, NS_LITERAL_CSTRING("icon_url"), mIcon.spec);
     NS_ENSURE_SUCCESS(rv, rv);
     rv = stmt->BindBlobByName(NS_LITERAL_CSTRING("data"),
                               TO_INTBUFFER(mIcon.data), mIcon.data.Length());
     NS_ENSURE_SUCCESS(rv, rv);
     rv = stmt->BindUTF8StringByName(NS_LITERAL_CSTRING("mime_type"), mIcon.mimeType);
     NS_ENSURE_SUCCESS(rv, rv);
     rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("expiration"), mIcon.expiration);
     NS_ENSURE_SUCCESS(rv, rv);
     rv = stmt->Execute();
     NS_ENSURE_SUCCESS(rv, rv);
 
     // Get the new icon id.  Do this regardless mIcon.id, since other code
     // could have added a entry before us.  Indeed we interrupted the thread
     // after the previous call to FetchIconInfo.
-    rv = FetchIconInfo(mDBConn, mIcon);
+    rv = FetchIconInfo(mFaviconSvc->mSyncStatements, mIcon);
     NS_ENSURE_SUCCESS(rv, rv);
 
     mIcon.status |= ICON_STATUS_SAVED;
   }
 
   // If the page does not have an id, try to insert a new one.
   if (mPage.id == 0) {
-    nsCOMPtr<mozIStorageStatement> stmt;
-    mDBConn->CreateStatement(NS_LITERAL_CSTRING(
-      "INSERT INTO moz_places (url, rev_host, favicon_id) "
-      "VALUES (:page_url, :rev_host, :favicon_id) "
-    ), getter_AddRefs(stmt));
+    nsCOMPtr<mozIStorageStatement> stmt =
+      mFaviconSvc->mSyncStatements.GetCachedStatement(NS_LITERAL_CSTRING(
+        "INSERT INTO moz_places (url, rev_host, favicon_id) "
+        "VALUES (:page_url, :rev_host, :favicon_id) "
+      ));
     NS_ENSURE_STATE(stmt);
+    mozStorageStatementScoper scoper(stmt);
     rv = URIBinder::Bind(stmt, NS_LITERAL_CSTRING("page_url"), mPage.spec);
     NS_ENSURE_SUCCESS(rv, rv);
     // The rev_host can be null.
     if (mPage.revHost.IsEmpty()) {
       rv = stmt->BindNullByName(NS_LITERAL_CSTRING("rev_host"));
     }
     else {
       rv = stmt->BindStringByName(NS_LITERAL_CSTRING("rev_host"), mPage.revHost);
     }
     NS_ENSURE_SUCCESS(rv, rv);
     rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("favicon_id"), mIcon.id);
     NS_ENSURE_SUCCESS(rv, rv);
     rv = stmt->Execute();
     NS_ENSURE_SUCCESS(rv, rv);
 
     // Get the new page id.
-    rv = FetchPageInfo(mDBConn, mPage);
+    rv = FetchPageInfo(mFaviconSvc->mSyncStatements, mPage);
     NS_ENSURE_SUCCESS(rv, rv);
 
     mIcon.status |= ICON_STATUS_ASSOCIATED;
   }
   // Otherwise just associate the icon to the page, if needed.
   else if (mPage.iconId != mIcon.id) {
-    nsCOMPtr<mozIStorageStatement> stmt;
-    mDBConn->CreateStatement(NS_LITERAL_CSTRING(
-      "UPDATE moz_places SET favicon_id = :icon_id WHERE id = :page_id"
-    ), getter_AddRefs(stmt));
+    nsCOMPtr<mozIStorageStatement> stmt =
+      mFaviconSvc->mSyncStatements.GetCachedStatement(NS_LITERAL_CSTRING(
+        "UPDATE moz_places SET favicon_id = :icon_id WHERE id = :page_id"
+      ));
     NS_ENSURE_STATE(stmt);
+    mozStorageStatementScoper scoper(stmt);
     rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("icon_id"), mIcon.id);
     NS_ENSURE_SUCCESS(rv, rv);
     rv = stmt->BindInt64ByName(NS_LITERAL_CSTRING("page_id"), mPage.id);
     NS_ENSURE_SUCCESS(rv, rv);
     rv = stmt->Execute();
     NS_ENSURE_SUCCESS(rv, rv);
 
     mIcon.status |= ICON_STATUS_ASSOCIATED;
--- a/toolkit/components/places/src/Helpers.h
+++ b/toolkit/components/places/src/Helpers.h
@@ -40,16 +40,17 @@
 #define mozilla_places_Helpers_h_
 
 /**
  * This file contains helper classes used by various bits of Places code.
  */
 
 #include "mozilla/storage.h"
 #include "nsIURI.h"
+#include "nsThreadUtils.h"
 
 namespace mozilla {
 namespace places {
 
 ////////////////////////////////////////////////////////////////////////////////
 //// Asynchronous Statement Callback Helper
 
 class AsyncStatementCallback : public mozIStorageStatementCallback
@@ -193,12 +194,43 @@ void GetReversedHostname(const nsString&
  *
  * @param aInput
  *        The string to be reversed
  * @param aReversed
  *        Ouput parameter will contain the reversed string
  */
 void ReverseString(const nsString& aInput, nsString& aReversed);
 
+/**
+ * Used to finalize a statementCache on a specified thread.
+ */
+template<typename StatementType>
+class FinalizeStatementCacheProxy : public nsRunnable
+{
+public:
+  /**
+   * Constructor.
+   *
+   * @param aStatementCache
+   *        The statementCache that should be finalized.
+   */
+  FinalizeStatementCacheProxy(
+    mozilla::storage::StatementCache<StatementType>& aStatementCache
+  )
+  : mStatementCache(aStatementCache)
+  {
+  }
+
+  NS_IMETHOD
+  Run()
+  {
+    mStatementCache.FinalizeStatements();
+    return NS_OK;
+  }
+
+protected:
+  mozilla::storage::StatementCache<StatementType>& mStatementCache;
+};
+
 } // namespace places
 } // namespace mozilla
 
 #endif // mozilla_places_Helpers_h_
--- a/toolkit/components/places/src/nsFaviconService.cpp
+++ b/toolkit/components/places/src/nsFaviconService.cpp
@@ -107,17 +107,18 @@ private:
 PLACES_FACTORY_SINGLETON_IMPLEMENTATION(nsFaviconService, gFaviconService)
 
 NS_IMPL_ISUPPORTS1(
   nsFaviconService
 , nsIFaviconService
 )
 
 nsFaviconService::nsFaviconService()
-: mFaviconsExpirationRunning(false)
+: mSyncStatements(mDBConn)
+, mFaviconsExpirationRunning(false)
 , mOptimizedIconDimension(OPTIMIZED_FAVICON_DIMENSION)
 , mFailedFaviconSerial(0)
 , mShuttingDown(false)
 {
   NS_ASSERTION(!gFaviconService,
                "Attempting to create two instances of the service!");
   gFaviconService = this;
 }
@@ -934,16 +935,24 @@ nsFaviconService::FinalizeStatements() {
     mDBRemoveAllFavicons,
   };
 
   for (PRUint32 i = 0; i < NS_ARRAY_LENGTH(stmts); i++) {
     nsresult rv = nsNavHistory::FinalizeStatement(stmts[i]);
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
+  // Finalize the statementCache on the correct thread.
+  nsRefPtr<FinalizeStatementCacheProxy<mozIStorageStatement>> event =
+    new FinalizeStatementCacheProxy<mozIStorageStatement>(mSyncStatements);
+  nsCOMPtr<nsIEventTarget> target = do_GetInterface(mDBConn);
+  NS_ENSURE_TRUE(target, NS_ERROR_OUT_OF_MEMORY);
+  nsresult rv = target->Dispatch(event, NS_DISPATCH_NORMAL);
+  NS_ENSURE_SUCCESS(rv, rv);
+
   return NS_OK;
 }
 
 
 nsresult
 nsFaviconService::GetFaviconDataAsync(nsIURI* aFaviconURI,
                                       mozIStorageStatementCallback *aCallback)
 {
--- a/toolkit/components/places/src/nsFaviconService.h
+++ b/toolkit/components/places/src/nsFaviconService.h
@@ -43,16 +43,17 @@
 #include "nsDataHashtable.h"
 #include "nsIFaviconService.h"
 #include "nsServiceManagerUtils.h"
 #include "nsString.h"
 
 #include "nsToolkitCompsCID.h"
 
 #include "mozilla/storage.h"
+#include "mozilla/storage/StatementCache.h"
 
 // Favicons bigger than this size should not be saved to the db to avoid
 // bloating it with large image blobs.
 // This still allows us to accept a favicon even if we cannot optimize it.
 #define MAX_FAVICON_SIZE 10240
 
 // Most icons will be smaller than this rough estimate of the size of an
 // uncompressed 16x16 RGBA image of the same dimensions.
@@ -137,16 +138,23 @@ public:
 
   /**
    * Finalize all internal statements.
    */
   nsresult FinalizeStatements();
 
   void SendFaviconNotifications(nsIURI* aPage, nsIURI* aFaviconURI);
 
+  /**
+   * This cache should be used only for background thread statements.
+   *
+   * @pre must be running on the background thread of mDBConn.
+   */
+  mozilla::storage::StatementCache<mozIStorageStatement> mSyncStatements;
+
   NS_DECL_ISUPPORTS
   NS_DECL_NSIFAVICONSERVICE
 
 private:
   ~nsFaviconService();
 
   nsCOMPtr<mozIStorageConnection> mDBConn; // from history service