Closed Bug 168411 Opened 18 years ago Closed 14 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
Firefox 2 beta1

People

(Reporter: p_ch, Assigned: jminta)

References

Details

(Keywords: fixed1.8.1, memory-leak)

Attachments

(1 file, 4 obsolete files)

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.
-> me
Assignee: blaker → chanial
Target Milestone: --- → Phoenix0.3
->0.5
Target Milestone: Phoenix0.3 → Phoenix0.5
Target Milestone: Phoenix0.5 → ---
I guess this is no longer an issue as we moved this transaction manager to the
bookmarks service.
Jan says this is no longer an issue...reopen if you disagree.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → WORKSFORME
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.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
taking QA contact, sorry about the bugspam
QA Contact: asa → mconnor
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
Summary: figure out how the transaction manager should work in several windows → move the bm transactions to the bookmarks service
You don't have to move them to the bookmarks service, you can write a new
JS-based service instead.
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).
Keywords: mlk
Summary: move the bm transactions to the bookmarks service → move bookmarks transactions into a JS service
Blocks: 275250
OS: Linux → All
Hardware: PC → All
Summary: move bookmarks transactions into a JS service → Move bookmarks transactions into a JS service (adding a bookmark leaks the Add Bookmark dialog)
Flags: blocking-firefox2?
Assignee: p_ch → nobody
Status: REOPENED → NEW
QA Contact: mconnor → bookmarks
Flags: blocking-firefox2? → blocking-firefox2+
Target Milestone: --- → Firefox 2 beta1
Assignee: nobody → jminta
Priority: -- → P2
(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?
Whiteboard: swag:3d
Attached file proposed idl (obsolete) —
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. :-)
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.
What's the difference between type and action?
(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.
(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).
Would it be feasible to move each bookmark individually and then use the transaction manager to batch the transaction for undo purposes?
(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.
Attached patch first pass (obsolete) — Splinter Review
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.
Attachment #224267 - Attachment is obsolete: true
Attachment #224608 - Flags: review?(sspitzer)
>  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.
Blocks: 315690
> 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.
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 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.
Attachment #224608 - Flags: review?(sspitzer)
Attached patch version 2 (obsolete) — Splinter Review
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.
Attachment #224608 - Attachment is obsolete: true
Attachment #225098 - Flags: review?(sspitzer)
Whiteboard: swag:3d → [swag:3d] [has patch, needs review Seth]
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.
Attached patch version 3 (obsolete) — Splinter Review
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.
Attachment #225098 - Attachment is obsolete: true
Attachment #225295 - Flags: review?(sspitzer)
Attachment #225098 - Flags: review?(sspitzer)
Attached patch version 4Splinter Review
Version 4 fixes a paste crasher, which was the last regression that Seth and I could find.
Attachment #225295 - Attachment is obsolete: true
Attachment #225355 - Flags: superreview?(beng)
Attachment #225355 - Flags: review?(sspitzer)
Attachment #225355 - Flags: approval-branch-1.8.1?(beng)
Attachment #225295 - Flags: review?(sspitzer)
Comment on attachment 225355 [details] [diff] [review]
version 4

r=sspitzer

changing sr request to bugs@bengoodger.com, ben's preferred account for reviews.
Attachment #225355 - Flags: superreview?(bugs)
Attachment #225355 - Flags: superreview?(beng)
Attachment #225355 - Flags: review?(sspitzer)
Attachment #225355 - Flags: review+
Attachment #225355 - Flags: approval-branch-1.8.1?(bugs)
Attachment #225355 - Flags: approval-branch-1.8.1?(beng)
Attachment #225355 - Flags: superreview?(mconnor)
Attachment #225355 - Flags: superreview?(bugs)
Attachment #225355 - Flags: approval-branch-1.8.1?(mconnor)
Attachment #225355 - Flags: approval-branch-1.8.1?(bugs)
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
Attachment #225355 - Flags: superreview?(mconnor)
Attachment #225355 - Flags: superreview+
Attachment #225355 - Flags: approval-branch-1.8.1?(mconnor)
Attachment #225355 - Flags: approval-branch-1.8.1+
Patch landed on trunk and 1_8 branch
Status: NEW → RESOLVED
Closed: 17 years ago14 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Whiteboard: [swag:3d] [has patch, needs review Seth]
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

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Probably you missed some package file changes to package nsBookmarkTransactionManager.js
(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.
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Depends on: 342110
Depends on: 342116
see bug #361030, a regression caused by this change.
Blocks: 361030
You need to log in before you can comment on or make changes to this bug.