Bug 405497 - Calling RunInBatchedMode within a RunBatched implementation hangs. r=Mano

browser/components/nsBrowserGlue.js

@@ -538,9 +538,6 @@ BrowserGlue.prototype = {
     var bmsvc = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
                 getService(Ci.nsINavBookmarksService);
 
-    // XXXmano bug 405497: this should be batched even if we're not called from
-    // the import service. However, calling runInBatchedMode from within a
-    // RunBatched implementation hangs the browser.
     var callback = {
       _placesBundle: Cc["@mozilla.org/intl/stringbundle;1"].
                      getService(Ci.nsIStringBundleService).
@@ -612,9 +609,7 @@ BrowserGlue.prototype = {
     };
 
     try {
-      callback.runBatched();
-      // See XXX note above
-      // bmsvc.runInBatchMode(callback, null);
+      bmsvc.runInBatchMode(callback, null);
     }
     catch(ex) {
       Components.utils.reportError(ex);

toolkit/components/places/src/nsNavBookmarks.cpp

@@ -47,7 +47,6 @@
 #include "nsFaviconService.h"
 #include "nsAnnotationService.h"
 #include "nsPrintfCString.h"
-#include "nsAutoLock.h"
 #include "nsIUUIDGenerator.h"
 #include "prprf.h"
 #include "nsILivemarkService.h"
@@ -84,7 +83,7 @@ nsNavBookmarks* nsNavBookmarks::sInstance = nsnull;
 
 nsNavBookmarks::nsNavBookmarks()
   : mItemCount(0), mRoot(0), mBookmarksRoot(0), mTagRoot(0), mToolbarFolder(0), mBatchLevel(0),
-    mLock(nsnull), mBatchHasTransaction(PR_FALSE)
+    mBatchHasTransaction(PR_FALSE)
 {
   NS_ASSERTION(!sInstance, "Multiple nsNavBookmarks instances!");
   sInstance = this;
@@ -94,8 +93,6 @@ nsNavBookmarks::~nsNavBookmarks()
 {
   NS_ASSERTION(sInstance == this, "Expected sInstance == this");
   sInstance = nsnull;
-  if (mLock)
-    PR_DestroyLock(mLock);
 }
 
 NS_IMPL_ISUPPORTS3(nsNavBookmarks,
@@ -288,9 +285,6 @@ nsNavBookmarks::Init()
   rv = transaction.Commit();
   NS_ENSURE_SUCCESS(rv, rv);
 
-  mLock = PR_NewLock();
-  NS_ENSURE_TRUE(mLock, NS_ERROR_OUT_OF_MEMORY);
-
   // Temporary migration code for bug 396300
   nsCOMPtr<mozIStorageStatement> moveUnfiledBookmarks;
   rv = dbConn->CreateStatement(NS_LITERAL_CSTRING("UPDATE moz_bookmarks SET parent = ?1 WHERE type = ?2 AND parent=?3"),
@@ -2530,7 +2524,7 @@ nsNavBookmarks::GetURIForKeyword(const nsAString& aKeyword, nsIURI** aURI)
   return NS_NewURI(aURI, spec);
 }
 
-// See RunInBatchMode, mLock _must_ be set when batching
+// See RunInBatchMode
 nsresult
 nsNavBookmarks::BeginUpdateBatch()
 {
@@ -2545,8 +2539,6 @@ nsNavBookmarks::BeginUpdateBatch()
     ENUMERATE_WEAKARRAY(mObservers, nsINavBookmarkObserver,
                         OnBeginUpdateBatch())
   }
-  mozIStorageConnection *dbConn = DBConn();
-  mozStorageTransaction transaction(dbConn, PR_FALSE);
   return NS_OK;
 }
 
@@ -2566,16 +2558,13 @@ nsNavBookmarks::EndUpdateBatch()
 NS_IMETHODIMP
 nsNavBookmarks::RunInBatchMode(nsINavHistoryBatchCallback* aCallback,
                                nsISupports* aUserData) {
-  NS_ENSURE_STATE(mLock);
   NS_ENSURE_ARG_POINTER(aCallback);
 
-  nsAutoLock lock(mLock);
   BeginUpdateBatch();
   nsresult rv = aCallback->RunBatched(aUserData);
   EndUpdateBatch();
-  NS_ENSURE_SUCCESS(rv, rv);
 
-  return NS_OK;
+  return rv;
 }
 
 NS_IMETHODIMP

toolkit/components/places/src/nsNavBookmarks.h

@@ -147,9 +147,6 @@ private:
   // the level of nesting of batches, 0 when no batches are open
   PRInt32 mBatchLevel;
 
-  // lock for RunInBatchMode
-  PRLock* mLock;
-
   // true if the outermost batch has an associated transaction that should
   // be committed when our batch level reaches 0 again.
   PRBool mBatchHasTransaction;

toolkit/components/places/src/nsNavHistory.cpp

@@ -85,7 +85,6 @@
 #include "mozStorageCID.h"
 #include "mozStorageHelper.h"
 #include "nsAppDirectoryServiceDefs.h"
-#include "nsAutoLock.h"
 #include "nsIIdleService.h"
 #include "nsILivemarkService.h"
 
@@ -316,7 +315,6 @@ nsNavHistory::GetSingleton()
 // nsNavHistory::nsNavHistory
 
 nsNavHistory::nsNavHistory() : mBatchLevel(0),
-                               mLock(nsnull),
                                mBatchHasTransaction(PR_FALSE),
                                mNowValid(PR_FALSE),
                                mExpireNowTimer(nsnull),
@@ -348,9 +346,6 @@ nsNavHistory::~nsNavHistory()
   // in case somebody creates an extra instance of the service.
   NS_ASSERTION(gHistoryService == this, "YOU CREATED 2 COPIES OF THE HISTORY SERVICE.");
   gHistoryService = nsnull;
-
-  if (mLock)
-    PR_DestroyLock(mLock);
 }
 
 
@@ -368,9 +363,6 @@ nsNavHistory::Init()
   rv = prefService->GetBranch(PREF_BRANCH_BASE, getter_AddRefs(mPrefBranch));
   NS_ENSURE_SUCCESS(rv, rv);
 
-  mLock = PR_NewLock();
-  NS_ENSURE_TRUE(mLock, NS_ERROR_OUT_OF_MEMORY);
-
   // prefs
   LoadPrefs(PR_TRUE);
 
@@ -3024,7 +3016,7 @@ nsNavHistory::RemoveObserver(nsINavHistoryObserver* aObserver)
 }
 
 // nsNavHistory::BeginUpdateBatch
-// See RunInBatchMode, mLock _must_ be set when batching
+// See RunInBatchMode
 nsresult
 nsNavHistory::BeginUpdateBatch()
 {
@@ -3038,7 +3030,6 @@ nsNavHistory::BeginUpdateBatch()
     ENUMERATE_WEAKARRAY(mObservers, nsINavHistoryObserver,
                         OnBeginUpdateBatch())
   }
-  mozStorageTransaction transaction(mDBConn, PR_FALSE);
   return NS_OK;
 }
 
@@ -3059,15 +3050,10 @@ NS_IMETHODIMP
 nsNavHistory::RunInBatchMode(nsINavHistoryBatchCallback* aCallback,
                              nsISupports* aUserData) 
 {
-  NS_ENSURE_STATE(mLock);
   NS_ENSURE_ARG_POINTER(aCallback);
 
-  nsAutoLock lock(mLock);
-
   UpdateBatchScoper batch(*this);
-  nsresult rv = aCallback->RunBatched(aUserData);
-  NS_ENSURE_SUCCESS(rv, rv);
-  return NS_OK;
+  return aCallback->RunBatched(aUserData);
 }
 
 NS_IMETHODIMP

toolkit/components/places/src/nsNavHistory.h

@@ -329,9 +329,6 @@ public:
   // the level of nesting of batches, 0 when no batches are open
   PRInt32 mBatchLevel;
 
-  // lock for RunInBatchMode
-  PRLock* mLock;
-
   // true if the outermost batch has an associated transaction that should
   // be committed when our batch level reaches 0 again.
   PRBool mBatchHasTransaction;

toolkit/components/places/tests/unit/test_405497.js

@@ -0,0 +1,92 @@
+/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* vim:set ts=2 sw=2 sts=2 et: */
+/* ***** BEGIN LICENSE BLOCK *****
+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
+ *
+ * The contents of this file are subject to the Mozilla Public License Version
+ * 1.1 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ * http://www.mozilla.org/MPL/
+ *
+ * Software distributed under the License is distributed on an "AS IS" basis,
+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
+ * for the specific language governing rights and limitations under the
+ * License.
+ *
+ * The Original Code is Places unit test code.
+ *
+ * The Initial Developer of the Original Code is
+ * Mozilla Corporation.
+ * Portions created by the Initial Developer are Copyright (C) 2008
+ * the Initial Developer. All Rights Reserved.
+ *
+ * Contributor(s):
+ *   Shawn Wilsher <me@shawnwilsher.com> (Original Author)
+ *
+ * Alternatively, the contents of this file may be used under the terms of
+ * either the GNU General Public License Version 2 or later (the "GPL"), or
+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
+ * in which case the provisions of the GPL or the LGPL are applicable instead
+ * of those above. If you wish to allow use of your version of this file only
+ * under the terms of either the GPL or the LGPL, and not to allow others to
+ * use your version of this file under the terms of the MPL, indicate your
+ * decision by deleting the provisions above and replace them with the notice
+ * and other provisions required by the GPL or the LGPL. If you do not delete
+ * the provisions above, a recipient may use your version of this file under
+ * the terms of any one of the MPL, the GPL or the LGPL.
+ *
+ * ***** END LICENSE BLOCK ***** */
+
+Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
+
+var hs = Cc["@mozilla.org/browser/nav-history-service;1"].
+         getService(Ci.nsINavHistoryService);
+var bs = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
+         getService(Ci.nsINavBookmarksService);
+
+/**
+ * The callback object for runInBatchMode.
+ *
+ * @param aService
+ *        Takes a reference to the history service or the bookmark service.
+ *        This determines which service should be called when calling the second
+ *        runInBatchMode the second time.
+ */
+function callback(aService)
+{
+  this.callCount = 0;
+  this.service = aService;
+}
+callback.prototype = {
+  //////////////////////////////////////////////////////////////////////////////
+  //// nsINavHistoryBatchCallback
+
+  runBatched: function(aUserData)
+  {
+    this.callCount++;
+
+    if (this.callCount == 1) {
+      // We want to call run in batched once more.
+      this.service.runInBatchMode(this, null);
+      return;
+    }
+
+    do_check_eq(this.callCount, 2);
+    do_test_finished();
+  },
+
+  //////////////////////////////////////////////////////////////////////////////
+  //// nsISupports
+
+  QueryInterface: XPCOMUtils.generateQI([Ci.nsINavHistoryBatchCallback])
+};
+
+function run_test() {
+  // checking the history service
+  do_test_pending();
+  hs.runInBatchMode(new callback(hs), null);
+
+  // checking the bookmark service
+  do_test_pending();
+  bs.runInBatchMode(new callback(bs), null);
+}