168411 - Move bookmarks transactions into a JS service (adding a bookmark leaks the Add Bookmark dialog)

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Pierre Chanial]

When a bookmark transaction  has been created in a bookmark manager window that
has been closed, when undoing in a new BM window, I hit this assertion:
###!!! ASSERTION: 
XPConnect is being called on a scope without a 'Components' property!

This is pretty much always bad. It usually means that native code is
making a callback to an interface implemented in JavaScript, but the
document where the JS object was created has already been cleared and the
global properties of that document's window are *gone*. Generally this
indicates a problem that should be addressed in the design and use of the
callback code.
: 'Error', file xpcwrappednativescope.cpp, line 549
Break: at file xpcwrappednativescope.cpp, line 549

In fact, the transaction is correctly undone, so I ponder the porting to cpp.

Comment 1

[Pierre Chanial]

-> me

Comment 2

[Pierre Chanial]

->0.5

Comment 3

[Jan Varga [:janv]]

I guess this is no longer an issue as we moved this transaction manager to the
bookmarks service.

Comment 4

[Blake Ross]

Jan says this is no longer an issue...reopen if you disagree.

Comment 5

[Pierre Chanial]

Sorry for the lag.
Basically, the easiest part of this bug has been fixed, ie, moving the
transaction manager instanciation to the bookmarks service.
But the trickiest thing to do is to move the transactions themselves. Currently,
they depend on the window context. Once the window is destroyed, the
transactions go away with the window.
Thus, we need to move them also to the bookmark service, and we need to figure
out what exacly needs to be moved.

Comment 6

[Mike Connor [:mconnor]]

taking QA contact, sorry about the bugspam

Comment 7

[David Bradley]

I'm also seeing this assert when I switch profiles. IS bug 207814 related or the
same issue? Below is the important part of the stack.

CompositeDataSourceImpl::OnAssert line 1463
nsBookmarksService::OnAssert line 6076
InMemoryDataSource::Assert line 1418
RDFContainerUtilsImpl::MakeContainer line 450 + 28 bytes

Comment 8

[[e-mail]]

You don't have to move them to the bookmarks service, you can write a new
JS-based service instead.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This is definitely still an issue.  There are also some serious memory leak
issues here.  Keeping all the transactions alive for the lifetime of the app is
important for the feature of app-lifetime undo/redo, so that in itself might not
be considered a leak bug.  However, the current behavior also means we leak a
lot of other stuff associated with the window along with the transactions, since
the transactions keep the JS global object for the window in which they were
created alive.  Keeping that JS global object alive also means keeping the
bookmarks service itself alive, since it's a global variable in the windows
involved, which means all this stuff is actually leaked (making it much easier
to detect) rather than just held until some point during shutdown.

Also, the Undo of old transactions actually doesn't work for me in a current
Firefox debug build.

Also, retitling per comment 8, since that requires much less work (i.e., not
rewriting all the existing transactions).

Comment 10

[Joey Minta]

(In reply to comment #8)
> You don't have to move them to the bookmarks service, you can write a new
> JS-based service instead.

Looking over the code, the current bookmarks service merely holds onto the transaction manager, without providing much real functionality there.  To me, is seems the best approach is to move the transaction manager out of the bookmarks service and into its own js-service, which would also hold the transaction-creation code.  Then we'd have all the transaction code in 1 place.  Any thoughts/opinions?

Comment 11

[Joey Minta]

Attachment: proposed idl

This is the idl I was looking at creating for the transaction service.  The bookmark transactions, currently defined in bookmarks.js would be moved into the service, with the exception of BookmarkMoveTransaction.  Because this transaction is heavily dependent on BookmarkUtils, I would change the calls to BookmarkMoveTransaction to make REMOVE and INSERT calls into the transaction manager from inside bookmarks.js.

Feedback strongly encouraged. :-)

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The [array] annotation in IDL doesn't pass the size through, so that's going to either have to be a lot messier or a bit different.

Comment 13

[[e-mail]]

What's the difference between type and action?

Comment 14

[Joey Minta]

(In reply to comment #13)
> What's the difference between type and action?
> 
Type can only be INSERT, REMOVE, and IMPORT.  Normally this will be the same as aAction.  However, for moving bookmarks, there will be a remove and then an insert transaction (batched together).  In this case, aAction for both of these transactions would be 'move'.  I was planning to have a follow up to trim the type and action into one property, but wanted to try to keep this patch smaller, if possible.

Comment 15

[Joey Minta]

(In reply to comment #12)
> The [array] annotation in IDL doesn't pass the size through, so that's going to
> either have to be a lot messier or a bit different.
> 
I see a couple possible solutions to this.  One is to simply include aParentCount, aIndexCount, etc as arguments required to be passed in.  The second is to break out methods such as 
  void setParents(in aCount, [array,size_is(aCount)] in nsIRDFResource aParents);
and so forth for indexes, removedProps, items, etc.  The downside of this is that we have to hold on to all those passed in objects until someone finally calls commit().  This isn't too difficult, but code-flow becomes a bit obscure.

The third, more drastic solution is to try to move a lot of the computation bits that create all these different arrays into the transaction service as well.  This will create a bit of a fork in terms of where changes to bookmarks actually happen (things like export can remain in BookmarkUtils without a problem, but import would need to move into the transaction service).  However, it would probably allow us to only pass in an array of RDF nodes (usually called aSelection in bookmarks.js) and a couple other parameters (index, action).

Comment 16

[[e-mail]]

Would it be feasible to move each bookmark individually and then use the transaction manager to batch the transaction for undo purposes?

Comment 17

[Joey Minta]

(In reply to comment #16)
> Would it be feasible to move each bookmark individually and then use the
> transaction manager to batch the transaction for undo purposes?
> 
Yeah, I think that would work.  I'll play around with it for a bit and see what pops out.

Comment 18

[Joey Minta]

Attachment: first pass

This is a first pass at moving the transactions into a js-service.  I went with neil's suggestion of passing in single bookmarks to the service, and batching them with the transaction manager, to avoid passing in tons of arrays (and the accompanying idl mess).  I need QA to run tests on import, since I don't have a windows box at hand, but insert/remove worked without regressions in my testing.  As mentioned before, I can file a follow-up bug to try to combine aType and aAction.

Comment 19

[(not reading, please use [e-mail] instead)]

>  I need QA to run tests on import, since I don't have a windows box at hand

I can help text / review this on windows before you check in.

looking over joey's shoulder while he gave me a demo on linux, his patch may have (as a side effect) fixed the copy-paste-undo bug (see #315690)

there was some weirdness with focus and selection that this patch might have broken, which I'll test on windows to confirm.

Comment 20

[(not reading, please use [e-mail] instead)]

> there was some weirdness with focus and selection that this patch might have
> broken, which I'll test on windows to confirm.

specifically, when you copy (one or more) bookmarks and paste, it does not select the pasted bookmarks, which is a regression.

Comment 21

[(not reading, please use [e-mail] instead)]

a couple of other issues I've run into with this patch on my mac:

1) on search results delete / cut no longer deletes bookmarks.  it works if I'm not looking at search results

2) after delete, deleted items show up in searches.  this is related to my bug #315690.  the reason this patch may have appeared to fix my bug is that we are no longer unasserting properties on removal, which explains why they are showing up on search.  (the unassert was added by vlad) to fix this very remove/search issue for bug #255255

Comment 22

[Joey Minta]

Comment on attachment 224608 [details] [diff] [review]
first pass

Canceling review request.  There were some problems with asserting and unasserting properties that I need to fix.

Comment 23

[Joey Minta]

Attachment: version 2

The previous version didn't remove the old bookmarkService's transaction manager, and this was hiding a bunch of deeper problems.  This version properly wires up the other consumers of that transaction manager.  It also involves a fair bit of juggling batch-changes, but this was a necessary expense of the approach to move to only passing single changes around.

Comment 24

[(not reading, please use [e-mail] instead)]

there's a couple of tricky things to look out for to avoid regressions:

1) after paste (of one or several bookmarks) selection is preserved
2) after delete, deleted bookmark doesn't show up in search (see bug #255255)

#2 is very tied to bug #315690 (my nemesis!), so if bug #315690 appears to be fixed, watch out that bug #255255 has not regressed.

Comment 25

[Joey Minta]

Attachment: version 3

Just a couple of minor changes to fix the regressions Seth noticed.  
1.) _itemToBeToggled needs to be an array - fixes the paste-selection
2.) the in operator doesn't work nicely with addition, so we weren't getting the right props to unassert.
3.) typo in the idl.

Comment 26

[Joey Minta]

Attachment: version 4

Version 4 fixes a paste crasher, which was the last regression that Seth and I could find.

Comment 27

[(not reading, please use [e-mail] instead)]

Comment on attachment 225355 [details] [diff] [review]
version 4

r=sspitzer

changing sr request to [e-mail], ben's preferred account for reviews.

Comment 28

[Mike Connor [:mconnor]]

Comment on attachment 225355 [details] [diff] [review]
version 4


-    var transaction = new BookmarkImportTransaction("import");
+    var bkmkTxnSvc = Components.classes["@mozilla.org/bookmarks/transactionmanager;1"]
+                               .getService(Components.interfaces.nsIBookmarkTransactionManager);
+    if (countAfter - countBefore > 1) {
+        bkmkTxnSvc.startBatch();
+    }

nit: zany indents, braces around single line (repeated throughout)

we seem to get the new transaction service a lot, maybe we should save some cycles and cache it?


Index: browser/components/bookmarks/public/Makefile.in
===================================================================
RCS file: /cvsroot/mozilla/browser/components/bookmarks/public/Makefile.in,v
retrieving revision 1.5
diff -p -U8 -r1.5 Makefile.in
--- browser/components/bookmarks/public/Makefile.in	1 Feb 2005 17:36:48 -0000	1.5
+++ browser/components/bookmarks/public/Makefile.in	13 Jun 2006 01:11:01 -0000
@@ -40,12 +40,13 @@ topsrcdir	= @top_srcdir@
 srcdir		= @srcdir@
 VPATH		= @srcdir@
 
 include $(DEPTH)/config/autoconf.mk
 
 MODULE		= bookmarks
 XPIDL_MODULE	= bookmarks
 
-XPIDLSRCS	= nsIBookmarksService.idl
+XPIDLSRCS	= nsIBookmarksService.idl \
+              nsIBookmarkTransactionManager.idl
 
spaces in makefiles are bad, use tabs


+/****
+ **** module registration
+ ****/
+
+var bkmkTxnMgrModule = {
+    mCID: Components.ID("{8be133d0-681d-4f0b-972b-6a68e41afb62}"),
+    mContractID: "@mozilla.org/bookmarks/transactionmanager;1",
+    
+    registerSelf: function (compMgr, fileSpec, location, type) {
+        compMgr = compMgr.QueryInterface(Components.interfaces.nsIComponentRegistrar);
+        compMgr.registerFactoryLocation(this.mCID,
+                                        "Bookmark Transaction Manager",
+                                        this.mContractID,
+                                        fileSpec,
+                                        location,
+                                        type);
+    },
+
+    getClassObject: function (compMgr, cid, iid) {
+        if (!cid.equals(this.mCID))
+            throw Components.results.NS_ERROR_NO_INTERFACE;
+
+        if (!iid.equals(Components.interfaces.nsIFactory))
+            throw Components.results.NS_ERROR_NOT_IMPLEMENTED;
+
+        return this.mFactory;
+    },
+
+    mFactory: {
+        createInstance: function (outer, iid) {
+            if (outer != null)
+                throw Components.results.NS_ERROR_NO_AGGREGATION;
+            return (new bookmarkTransactionManager()).QueryInterface(iid);
+        }
+    },
+
+    canUnload: function(compMgr) {
+        return true;
+    }
+};

so, if the factory is implemented in the same object as the module, we'll leak, based on what gavin's seen

r+a=me with that fixed

Comment 29

[Joey Minta]

Patch landed on trunk and 1_8 branch

Comment 30

[Bill Gianopoulos [:WG9s]]

Since this landed, bookmarks menues are completely broken on branch builds and also on trunk builds with palces disabled.

Attempting to access bokmark menues or bookmark pages via hotkeyes result in the following errors in the error console:

Error: Components.classes['@mozilla.org/bookmarks/transactionmanager;1'] has no properties
Source file: chrome://browser/content/bookmarks/bookmarks.js
Line: 103
 ----------
Error: Components.classes['@mozilla.org/bookmarks/transactionmanager;1'] has no properties
Source file: chrome://browser/content/bookmarks/bookmarksManager.js
Line: 58
 ----------
Error: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIXULTreeBuilder.removeObserver]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://browser/content/bookmarks/bookmarksTree.xml ::  :: line 66"  data: no]
 ----------
Error: Components.classes['@mozilla.org/bookmarks/transactionmanager;1'] has no properties
Source file: chrome://browser/content/bookmarks/bookmarksTree.xml
Line: 899
 ----------
Error: BMDS has no properties
Source file: chrome://browser/content/bookmarks/bookmarks.js
Line: 1360
 ----------
Error: Components.classes['@mozilla.org/bookmarks/transactionmanager;1'] has no properties
Source file: chrome://browser/content/bookmarks/bookmarksTree.xml
Line: 894
 ----------
Error: BMSVC has no properties
Source file: chrome://browser/content/browser.js
Line: 102

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Probably you missed some package file changes to package nsBookmarkTransactionManager.js

Comment 32

[Joey Minta]

(In reply to comment #31)
> Probably you missed some package file changes to package
> nsBookmarkTransactionManager.js
> 
Thanks for the pointer.  package file changes checked in.

Comment 33

[(not reading, please use [e-mail] instead)]

see bug #361030, a regression caused by this change.