Bug 382073 - Bookmarks Service batch update API is a footgun. r=sayrer/mrbkap, a=mconnor.

git-svn-id: svn://10.0.0.236/trunk@227359 18797224-902f-48f8-a5cc-f745e15eee43

mozilla/browser/components/places/content/controller.js

@@ -1556,34 +1556,50 @@ function PlacesAggregateTransaction(name, transactions) {
 PlacesAggregateTransaction.prototype = {
   __proto__: PlacesBaseTransaction.prototype,
   
-  doTransaction: function() {
+  doTransaction: function PAT_doTransaction() {
     this.LOG("== " + this._name + " (Aggregate) ==============");
-    if (this._transactions.length >= this.MIN_TRANSACTIONS_FOR_BATCH)
-      this.bookmarks.beginUpdateBatch();
-    for (var i = 0; i < this._transactions.length; ++i) {
-      var txn = this._transactions[i];
-      if (this.container > -1) 
-        txn.container = this.container;
-      txn.doTransaction();
+    if (this._transactions.length >= this.MIN_TRANSACTIONS_FOR_BATCH) {
+      var callback = {
+        _self: this,
+        runBatched: function() {
+          this._self.commit(false);
+        }
+      };
+      this.utils.bookmarks.runInBatchMode(callback, null);
     }
-    if (this._transactions.length >= this.MIN_TRANSACTIONS_FOR_BATCH)
-      this.bookmarks.endUpdateBatch();
+    else
+      this.commit(false);
+
     this.LOG("== " + this._name + " (Aggregate Ends) =========");
   },
-  
-  undoTransaction: function() {
+
+  undoTransaction: function PAT_undoTransaction() {
     this.LOG("== UN" + this._name + " (UNAggregate) ============");
-    if (this._transactions.length >= this.MIN_TRANSACTIONS_FOR_BATCH)
-      this.bookmarks.beginUpdateBatch();
+    if (this._transactions.length >= this.MIN_TRANSACTIONS_FOR_BATCH) {
+      var callback = {
+        _self: this,
+        runBatched: function() {
+          this._self.commit(true);
+        }
+      };
+      this.utils.bookmarks.runInBatchMode(callback, null);
+    }
+    else
+      this.commit(true);
+  
+    this.LOG("== UN" + this._name + " (UNAggregate Ends) =======");
+  },
+
+  commit: function PAT_commit(aUndo) {
     for (var i = this._transactions.length - 1; i >= 0; --i) {
       var txn = this._transactions[i];
       if (this.container > -1) 
         txn.container = this.container;
-      txn.undoTransaction();
+      if (aUndo)
+        txn.undoTransaction();
+      else
+        txn.doTransaction();
     }
-    if (this._transactions.length >= this.MIN_TRANSACTIONS_FOR_BATCH)
-      this.bookmarks.endUpdateBatch();
-    this.LOG("== UN" + this._name + " (UNAggregate Ends) =======");
   }
 };
 

mozilla/browser/components/places/src/nsPlacesImportExportService.cpp

@@ -293,7 +293,8 @@ nsEscapeHTML(const char * string)
 	return(rv);
 }
 
-NS_IMPL_ISUPPORTS1(nsPlacesImportExportService, nsIPlacesImportExportService)
+NS_IMPL_ISUPPORTS2(nsPlacesImportExportService, nsIPlacesImportExportService,
+                   nsINavHistoryBatchCallback)
 
 nsPlacesImportExportService::nsPlacesImportExportService()
 {
@@ -2299,18 +2300,6 @@ nsPlacesImportExportService::ImportHTMLFromFileInternal(nsILocalFile* aFile,
     return NS_ERROR_INVALID_ARG;
   }
 
-  // wrap the import in a transaction to make it faster
-  mBookmarksService->BeginUpdateBatch();
-
-  if (aIsImportDefaults) {
-    PRInt64 bookmarksRoot;
-    rv = mBookmarksService->GetBookmarksRoot(&bookmarksRoot);
-    NS_ENSURE_SUCCESS(rv,rv);
-
-    rv = mBookmarksService->RemoveFolderChildren(bookmarksRoot);
-    NS_ENSURE_SUCCESS(rv, rv);
-  }
-
   nsCOMPtr<nsIParser> parser = do_CreateInstance(kParserCID, &rv);
   NS_ENSURE_SUCCESS(rv, rv);
 
@@ -2327,32 +2316,51 @@ nsPlacesImportExportService::ImportHTMLFromFileInternal(nsILocalFile* aFile,
   nsCOMPtr<nsIURI> fileURI;
   rv = ioservice->NewFileURI(file, getter_AddRefs(fileURI));
   NS_ENSURE_SUCCESS(rv, rv);
-  nsCOMPtr<nsIChannel> channel;
-  rv = ioservice->NewChannelFromURI(fileURI, getter_AddRefs(channel));
+  rv = ioservice->NewChannelFromURI(fileURI, getter_AddRefs(mImportChannel));
   NS_ENSURE_SUCCESS(rv, rv);
-  rv = channel->SetContentType(NS_LITERAL_CSTRING("text/html"));
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  // streams
-  nsCOMPtr<nsIInputStream> stream;
-  rv = channel->Open(getter_AddRefs(stream));
-  NS_ENSURE_SUCCESS(rv, rv);
-  nsCOMPtr<nsIInputStream> bufferedstream;
-  rv = NS_NewBufferedInputStream(getter_AddRefs(bufferedstream), stream, 4096);
+  rv = mImportChannel->SetContentType(NS_LITERAL_CSTRING("text/html"));
   NS_ENSURE_SUCCESS(rv, rv);
 
   // init parser
   rv = parser->Parse(fileURI, nsnull);
   NS_ENSURE_SUCCESS(rv, rv);
 
+  // wrap the import in a transaction to make it faster
+  mIsImportDefaults = aIsImportDefaults;
+  mBookmarksService->RunInBatchMode(this, parser);
+  mImportChannel = nsnull;
+  return NS_OK;
+}
+
+NS_IMETHODIMP
+nsPlacesImportExportService::RunBatched(nsISupports* aUserData)
+{
+  nsresult rv;
+  if (mIsImportDefaults) {
+    PRInt64 bookmarksRoot;
+    rv = mBookmarksService->GetBookmarksRoot(&bookmarksRoot);
+    NS_ENSURE_SUCCESS(rv,rv);
+
+    rv = mBookmarksService->RemoveFolderChildren(bookmarksRoot);
+    NS_ENSURE_SUCCESS(rv, rv);
+  }
+
+  // streams
+  nsCOMPtr<nsIInputStream> stream;
+  rv = mImportChannel->Open(getter_AddRefs(stream));
+  NS_ENSURE_SUCCESS(rv, rv);
+  nsCOMPtr<nsIInputStream> bufferedstream;
+  rv = NS_NewBufferedInputStream(getter_AddRefs(bufferedstream), stream, 4096);
+  NS_ENSURE_SUCCESS(rv, rv);
+
   // feed the parser the data
   // Note: on error, we always need to set the channel's status to be the
   // same, and to always call OnStopRequest with the channel error.
-  nsCOMPtr<nsIStreamListener> listener = do_QueryInterface(parser, &rv);
+  nsCOMPtr<nsIStreamListener> listener = do_QueryInterface(aUserData, &rv);
   NS_ENSURE_SUCCESS(rv, rv);
-  rv = listener->OnStartRequest(channel, nsnull);
-  rv = SyncChannelStatus(channel, rv);
-  while(NS_SUCCEEDED(rv))
+  rv = listener->OnStartRequest(mImportChannel, nsnull);
+  rv = SyncChannelStatus(mImportChannel, rv);
+  while (NS_SUCCEEDED(rv))
   {
     PRUint32 available;
     rv = bufferedstream->Available(&available);
@@ -2361,20 +2369,19 @@ nsPlacesImportExportService::ImportHTMLFromFileInternal(nsILocalFile* aFile,
       available = 0;
     }
     if (NS_FAILED(rv)) {
-      channel->Cancel(rv);
+      mImportChannel->Cancel(rv);
       break;
     }
     if (!available)
       break; // blocking input stream has none available when done
 
-    rv = listener->OnDataAvailable(channel, nsnull, bufferedstream, 0, available);
-    rv = SyncChannelStatus(channel, rv);
+    rv = listener->OnDataAvailable(mImportChannel, nsnull, bufferedstream, 0,
+                                   available);
+    rv = SyncChannelStatus(mImportChannel, rv);
     if (NS_FAILED(rv))
       break;
   }
-  listener->OnStopRequest(channel, nsnull, rv);
-  // commit transaction
-  mBookmarksService->EndUpdateBatch();
+  listener->OnStopRequest(mImportChannel, nsnull, rv);
   return NS_OK;
 }
 

mozilla/browser/components/places/src/nsPlacesImportExportService.h

@@ -12,12 +12,15 @@
 #include "nsINavHistoryService.h"
 #include "nsINavBookmarksService.h"
 #include "nsIMicrosummaryService.h"
+#include "nsIChannel.h"
 
-class nsPlacesImportExportService : public nsIPlacesImportExportService
+class nsPlacesImportExportService : public nsIPlacesImportExportService,
+                                    public nsINavHistoryBatchCallback
 {
   public:
     NS_DECL_ISUPPORTS
     NS_DECL_NSIPLACESIMPORTEXPORTSERVICE
+    NS_DECL_NSINAVHISTORYBATCHCALLBACK
     nsPlacesImportExportService();
 
   private:
@@ -31,6 +34,9 @@ class nsPlacesImportExportService : public nsIPlacesImportExportService
     nsCOMPtr<nsILivemarkService> mLivemarkService;
     nsCOMPtr<nsIMicrosummaryService> mMicrosummaryService;
 
+    nsCOMPtr<nsIChannel> mImportChannel;
+    PRBool mIsImportDefaults;
+
     nsresult ImportHTMLFromFileInternal(nsILocalFile* aFile, PRBool aAllowRootChanges,
                                        PRInt64 aFolder, PRBool aIsImportDefaults);
     nsresult WriteContainer(nsINavHistoryResultNode* aFolder, const nsACString& aIndent, nsIOutputStream* aOutput);

mozilla/toolkit/components/places/public/nsINavBookmarksService.idl

@@ -47,6 +47,7 @@
 interface nsIFile;
 interface nsIURI;
 interface nsITransaction;
+interface nsINavHistoryBatchCallback;
 
 [ptr] native PRInt64Array(nsTArray<PRInt64>);
 
@@ -54,7 +55,7 @@ interface nsITransaction;
  * Observer for bookmark changes.
  */
 
-[scriptable, uuid(ef368152-354a-4a2b-92e4-910a80153c6f)]
+[scriptable, uuid(f9828ba8-9c70-4d95-b926-60d9e4378d7d)]
 interface nsINavBookmarkObserver : nsISupports
 {
   /**
@@ -495,23 +496,16 @@ interface nsINavBookmarksService : nsISupports
   void removeObserver(in nsINavBookmarkObserver observer);
 
   /**
-   * Causes observers to be notified of a beginUpdateBatch when a lot of things
+   * Runs the passed callback in batch mode. Use this when a lot of things
    * are about to change. Calls can be nested, observers will only be
    * notified when all batches begin/end.
    *
-   * It is EXTREMELY IMPORTANT that you call EndUpdateBatch for each call to
-   * beginUpdateBatch. If you don't do this, certain parts of the UI will not
-   * get updated and any changes to bookmarks will not get written to disk.
-   * From C++ code inside the places component, use nsBookmarksUpdateBatcher
-   * defined in nsNavBookmarks.h to scope batches. For JS or from other
-   * components, just please be very careful to close the batch, especially
-   * when encountering an error and returning early.
+   * @param aCallback
+   *        nsINavHistoryBatchCallback interface to call.
+   * @param aUserData
+   *        Opaque parameter passed to nsINavBookmarksBatchCallback
    */
-  void beginUpdateBatch();
-
-  /**
-   * Causes observers to be notified of an endUpdateBatch when a batch is
-   * done changing. Should match beginUpdateBatch or bad things will happen.
-   */
-  void endUpdateBatch();
+  void runInBatchMode(in nsINavHistoryBatchCallback aCallback,
+                      in nsISupports aUserData);
 };
+

mozilla/toolkit/components/places/public/nsINavHistoryService.idl

@@ -46,6 +46,7 @@ interface nsINavHistoryQueryResultNode;
 interface nsINavHistoryQuery;
 interface nsINavHistoryQueryOptions;
 interface nsINavHistoryResult;
+interface nsINavHistoryBatchCallback;
 interface nsITreeColumn;
 interface nsIWritablePropertyBag;
 
@@ -1144,7 +1145,7 @@ interface nsINavHistoryQueryOptions : nsISupports
   nsINavHistoryQueryOptions clone();
 };
 
-[scriptable, uuid(1a8df7bf-4889-4331-a521-f6c4dacd20e7)]
+[scriptable, uuid(b6751da3-a87c-4991-bb69-3c2289d3e0ff)]
 interface nsINavHistoryService : nsISupports
 {
   /**
@@ -1332,17 +1333,17 @@ interface nsINavHistoryService : nsISupports
   void removeObserver(in nsINavHistoryObserver observer);
 
   /**
-   * Causes observers to be notified of a beginUpdateBatch when a lot of things
+   * Runs the passed callback in batch mode. Use this when a lot of things
    * are about to change. Calls can be nested, observers will only be
    * notified when all batches begin/end.
+   *
+   * @param aCallback
+   *        nsINavHistoryBatchCallback interface to call.
+   * @param aUserData
+   *        Opaque parameter passed to nsINavBookmarksBatchCallback
    */
-  void beginUpdateBatch();
-
-  /**
-   * Causes observers to be notified of an endUpdateBatch when a batch is
-   * done changing. Should match beginUpdateBatch or bad things will happen.
-   */
-  void endUpdateBatch();
+  void runInBatchMode(in nsINavHistoryBatchCallback aCallback,
+                      in nsISupports aClosure);
 
   /** 
    * True if history is disabled. currently, 
@@ -1367,3 +1368,11 @@ interface nsIMorkHistoryImporter : nsISupports
    */
   void importHistory(in nsIFile file, in nsINavHistoryService history);
 };
+
+/**
+ * @see runInBatchMode of nsINavHistoryService/nsINavBookmarksService
+ */
+[scriptable, uuid(5143f2bb-be0a-4faf-9acb-b0ed3f82952c)]
+interface nsINavHistoryBatchCallback : nsISupports {
+  void runBatched(in nsISupports aUserData);
+};

mozilla/toolkit/components/places/src/nsLivemarkService.js

@@ -441,6 +441,50 @@ LivemarkLoadListener.prototype = {
     return this.__history;
   },
 
+  // called back from handleResult
+  runBatched: function LLL_runBatched(aUserData) {
+    var result = aUserData.QueryInterface(Ci.nsIFeedResult);
+
+    // We need this to make sure the item links are safe
+    var secMan = Cc[SEC_CONTRACTID].getService(Ci.nsIScriptSecurityManager);
+      
+    // Clear out any child nodes of the livemark folder, since
+    // they're about to be replaced.
+    var lmService = Cc[LS_CONTRACTID].getService(Ci.nsILivemarkService);
+    this.deleteLivemarkChildren(this._livemark.folderId);
+
+    // Enforce well-formedness because the existing code does
+    if (!result || !result.doc || result.bozo) {
+      this.insertLivemarkFailedItem(this._livemark.folderId);
+      this._ttl = EXPIRATION;
+      throw Cr.NS_ERROR_FAILURE;
+    }
+
+    var title, href, entry;
+    var feed = result.doc.QueryInterface(Ci.nsIFeed);
+    // Loop through and check for a link and a title
+    // as the old code did
+    for (var i = 0; i < feed.items.length; ++i) {
+      entry = feed.items.queryElementAt(i, Ci.nsIFeedEntry);
+      if (entry.title)
+        title = entry.title.plainText();
+      else if (entry.updated)
+        title = entry.updated;
+
+      if (entry.link) {
+        try {
+          secMan.checkLoadURIStr(this._livemark.feedURI.spec, entry.link.spec,
+                                 SEC_FLAGS);
+          href = entry.link;
+        }
+        catch (ex) { }
+      }
+
+      if (href && title)
+        this.insertLivemarkChild(this._livemark.folderId, href, title);
+    }
+  },
+
   /**
    * See nsIFeedResultListener.idl
    */
@@ -449,54 +493,11 @@ LivemarkLoadListener.prototype = {
       this._livemark.locked = false;
       return;
     }
-    
-    this._bms.beginUpdateBatch();
-      
-    try {  
-      // We need this to make sure the item links are safe
-      var secMan = Cc[SEC_CONTRACTID].getService(Ci.nsIScriptSecurityManager);
-      
-      // Clear out any child nodes of the livemark folder, since
-      // they're about to be replaced.
-      var lmService = Cc[LS_CONTRACTID].getService(Ci.nsILivemarkService);
-      this.deleteLivemarkChildren(this._livemark.folderId);
-
-      // Enforce well-formedness because the existing code does
-      if (!result || !result.doc || result.bozo) {
-        this.insertLivemarkFailedItem(this._livemark.folderId);
-        this._ttl = EXPIRATION;
-        throw Cr.NS_ERROR_FAILURE;
-      }
-
-      var title, href, entry;
-      var feed = result.doc.QueryInterface(Ci.nsIFeed);
-      // Loop through and check for a link and a title
-      // as the old code did
-      for (var i = 0; i < feed.items.length; ++i) {
-        entry = feed.items.queryElementAt(i, Ci.nsIFeedEntry);
-        if (entry.title)
-          title = entry.title.plainText();
-        else if (entry.updated)
-          title = entry.updated;
-        
-        if (entry.link) {
-          try {
-              secMan.checkLoadURIStr(this._livemark.feedURI.spec,
-                                     entry.link.spec, SEC_FLAGS);
-              href = entry.link;
-          } 
-          catch (ex) {
-          }
-        }
-        
-        if (href && title) {
-          this.insertLivemarkChild(this._livemark.folderId,
-                                   href, title);
-        }
-      }
-    } 
+    try {
+      // The actual work is done in runBatched, see above.
+      this._bms.runInBatchMode(this, result);
+    }
     finally {
-      this._bms.endUpdateBatch();
       this._processor.listener = null;
       this._processor = null;
       this._livemark.locked = false;
@@ -598,6 +599,7 @@ LivemarkLoadListener.prototype = {
     if (iid.equals(Ci.nsIFeedResultListener) ||
         iid.equals(Ci.nsIStreamListener) ||
         iid.equals(Ci.nsIRequestObserver)||
+        iid.equals(Ci.nsINavHistoryBatchCallback) ||
         iid.equals(Ci.nsISupports))
       return this;
     throw Cr.NS_ERROR_NO_INTERFACE;

mozilla/toolkit/components/places/src/nsNavBookmarks.cpp

@@ -47,6 +47,7 @@
 #include "nsFaviconService.h"
 #include "nsAnnotationService.h"
 #include "nsPrintfCString.h"
+#include "nsAutoLock.h"
 
 const PRInt32 nsNavBookmarks::kFindBookmarksIndex_ID = 0;
 const PRInt32 nsNavBookmarks::kFindBookmarksIndex_Type = 1;
@@ -88,6 +89,8 @@ nsNavBookmarks::~nsNavBookmarks()
 {
   NS_ASSERTION(sInstance == this, "Expected sInstance == this");
   sInstance = nsnull;
+  if (mLock)
+    PR_DestroyLock(mLock);
 }
 
 NS_IMPL_ISUPPORTS3(nsNavBookmarks,
@@ -233,6 +236,9 @@ nsNavBookmarks::Init()
   rv = transaction.Commit();
   NS_ENSURE_SUCCESS(rv, rv);
 
+  mLock = PR_NewLock();
+  NS_ENSURE_TRUE(mLock, NS_ERROR_OUT_OF_MEMORY);
+
   nsAnnotationService* annosvc = nsAnnotationService::GetAnnotationService();
   NS_ENSURE_TRUE(annosvc, NS_ERROR_OUT_OF_MEMORY);
 
@@ -2325,7 +2331,8 @@ nsNavBookmarks::GetURIForKeyword(const nsAString& aKeyword, nsIURI** aURI)
   return NS_NewURI(aURI, spec);
 }
 
-NS_IMETHODIMP
+// See RunInBatchMode, mLock _must_ be set when batching
+nsresult
 nsNavBookmarks::BeginUpdateBatch()
 {
   if (mBatchLevel++ == 0) {
@@ -2344,7 +2351,7 @@ nsNavBookmarks::BeginUpdateBatch()
   return NS_OK;
 }
 
-NS_IMETHODIMP
+nsresult
 nsNavBookmarks::EndUpdateBatch()
 {
   if (--mBatchLevel == 0) {
@@ -2357,6 +2364,21 @@ nsNavBookmarks::EndUpdateBatch()
   return NS_OK;
 }
 
+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;
+}
+
 NS_IMETHODIMP
 nsNavBookmarks::AddObserver(nsINavBookmarkObserver *aObserver,
                             PRBool aOwnsWeak)

mozilla/toolkit/components/places/src/nsNavBookmarks.h

@@ -107,6 +107,9 @@ public:
   // Called by History service when quitting.
   nsresult OnQuit();
 
+  nsresult BeginUpdateBatch();
+  nsresult EndUpdateBatch();
+
 private:
   static nsNavBookmarks *sInstance;
 
@@ -144,6 +147,9 @@ 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;

mozilla/toolkit/components/places/src/nsNavHistory.cpp

@@ -2294,7 +2294,7 @@ nsNavHistory::RemoveObserver(nsINavHistoryObserver* aObserver)
 
 // nsNavHistory::BeginUpdateBatch
 
-NS_IMETHODIMP
+nsresult
 nsNavHistory::BeginUpdateBatch()
 {
   mBatchesInProgress ++;
@@ -2308,7 +2308,7 @@ nsNavHistory::BeginUpdateBatch()
 
 // nsNavHistory::EndUpdateBatch
 
-NS_IMETHODIMP
+nsresult
 nsNavHistory::EndUpdateBatch()
 {
   if (mBatchesInProgress == 0)
@@ -2319,6 +2319,17 @@ nsNavHistory::EndUpdateBatch()
   return NS_OK;
 }
 
+NS_IMETHODIMP
+nsNavHistory::RunInBatchMode(nsINavHistoryBatchCallback* aCallback,
+                             nsISupports* aUserData) {
+  NS_ENSURE_ARG_POINTER(aCallback);
+
+  UpdateBatchScoper batch(*this);
+  nsresult rv = aCallback->RunBatched(aUserData);
+  NS_ENSURE_SUCCESS(rv, rv);
+  return NS_OK;
+}
+
 NS_IMETHODIMP
 nsNavHistory::GetHistoryDisabled(PRBool *_retval)
 {

mozilla/toolkit/components/places/src/nsNavHistory.h

@@ -303,6 +303,11 @@ public:
                           const PRUint16* aGroupingMode, PRUint32 aGroupCount,
                           nsCOMArray<nsNavHistoryResultNode>* aDest);
 
+  // Don't use these directly, inside nsNavHistory use UpdateBatchScoper,
+  // else use nsINavHistoryService::RunInBatchMode
+  nsresult BeginUpdateBatch();
+  nsresult EndUpdateBatch();
+
   // better alternative to QueryStringToQueries (in nsNavHistoryQuery.cpp)
   nsresult QueryStringToQueryArray(const nsACString& aQueryString,
                                    nsCOMArray<nsNavHistoryQuery>* aQueries,