The crash was here: http://hg.mozilla.org/mozilla-central/annotate/606acfb74fb5/toolkit/components/places/src/nsNavHistoryResult.cpp#l4167 Just before the crash this was output: "pure virtual method called terminate called without an active exception" Stack: Operating system: Linux 0.0.0 Linux 2.6.18-53.1.19.el5 #1 SMP Wed May 7 08:20:19 EDT 2008 i686 GNU/Linux CPU: x86 GenuineIntel family 10 model 15 stepping 8 1 CPU Crash reason: SIGABRT Crash address: 0xa6f7f2 Thread 0 (crashed) 0 ld-2.5.so + 0x7f2 eip = 0x00a6f7f2 esp = 0xbfc8b4b4 ebp = 0xbfc8b4c0 ebx = 0x00007c7d esi = 0xbfc8b560 edi = 0x00bc9ff4 eax = 0x00000000 ecx = 0x00007c7d edx = 0x00000006 efl = 0x00200206 1 libc-2.5.so + 0x2a450 eip = 0x00abb451 esp = 0xbfc8b4c8 ebp = 0xbfc8b5ec 2 libstdc++.so.6.0.8 + 0xb739f eip = 0x009f63a0 esp = 0xbfc8b5f4 ebp = 0xbfc8b62c 3 libstdc++.so.6.0.8 + 0xb4e84 eip = 0x009f3e85 esp = 0xbfc8b634 ebp = 0xbfc8b64c 4 libstdc++.so.6.0.8 + 0xb4ec1 eip = 0x009f3ec2 esp = 0xbfc8b654 ebp = 0xbfc8b65c 5 libstdc++.so.6.0.8 + 0xb5564 eip = 0x009f4565 esp = 0xbfc8b664 ebp = 0xbfc8b67c 6 libxul.so!nsNavHistoryResult::OnItemAdded(long long, long long, int) [nsNavHistoryResult.cpp:606acfb74fb5 : 4167 + 0x25] eip = 0x40968caa esp = 0xbfc8b684 ebp = 0xbfc8b6cc 7 libxul.so!nsNavBookmarks::InsertBookmark(long long, nsIURI*, int, nsACString_internal const&, long long*) [nsNavBookmarks.cpp:606acfb74fb5 : 1124 + 0x2c] eip = 0x409743e9 esp = 0xbfc8b6d4 ebp = 0xbfc8b7fc 8 libxul.so!NS_GetXPTCallStub_P + 0x30 eip = 0x40a9712b esp = 0xbfc8b804 ebp = 0xbfc8b83c 9 libxul.so!XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) [xpcwrappednative.cpp:606acfb74fb5 : 2480 + 0x20] eip = 0x401eea70 esp = 0xbfc8b844 ebp = 0xbfc8ba8c 10 libxul.so!XPC_WN_CallMethod(JSContext*, JSObject*, unsigned int, int*, int*) [xpcwrappednativejsops.cpp:606acfb74fb5 : 1585 + 0x9] eip = 0x401f4efd esp = 0xbfc8ba94 ebp = 0xbfc8bb5c 11 libmozjs.so!js_Invoke [jsinterp.cpp:606acfb74fb5 : 1368 + 0x10] eip = 0x40e7052e esp = 0xbfc8bb64 ebp = 0xbfc8bc2c 12 libmozjs.so!js_Interpret [jsinterp.cpp:606acfb74fb5 : 5088 + 0x20] eip = 0x40e62e84 esp = 0xbfc8bc34 ebp = 0xbfc8be9c 13 libmozjs.so!js_Invoke [jsinterp.cpp:606acfb74fb5 : 1376 + 0xa] eip = 0x40e70541 esp = 0xbfc8bea4 ebp = 0xbfc8bf5c 14 libxul.so!nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) [xpcwrappedjsclass.cpp:606acfb74fb5 : 1608 + 0x1a] eip = 0x401ec46d esp = 0xbfc8bf64 ebp = 0xbfc8c13c 15 libxul.so!nsXPCWrappedJS::CallMethod(unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) [xpcwrappedjs.cpp:606acfb74fb5 : 561 + 0x13] eip = 0x401e775d esp = 0xbfc8c144 ebp = 0xbfc8c16c 16 libxul.so!PrepareAndDispatch [xptcstubs_gcc_x86_unix.cpp:606acfb74fb5 : 95 + 0x1d] eip = 0x40a97c41 esp = 0xbfc8c174 ebp = 0xbfc8c23c 17 libxul.so!nsThread::ProcessNextEvent(int, int*) [nsThread.cpp:606acfb74fb5 : 510 + 0x5] eip = 0x40a89bc9 esp = 0xbfc8c244 ebp = 0xbfc8c27c 18 libxul.so!NS_ProcessNextEvent_P(nsIThread*, int) [nsThreadUtils.cpp : 230 + 0xd] eip = 0x40a574f7 esp = 0xbfc8c284 ebp = 0xbfc8c2ac 19 libxul.so!nsXULWindow::ShowModal() [nsXULWindow.cpp:606acfb74fb5 : 415 + 0x9] eip = 0x408614b7 esp = 0xbfc8c2b4 ebp = 0xbfc8c2ec 20 libxul.so!nsContentTreeOwner::ShowAsModal() [nsContentTreeOwner.cpp:606acfb74fb5 : 528 + 0xb] eip = 0x4085da39 esp = 0xbfc8c2f4 ebp = 0xbfc8c30c 21 libxul.so!nsWindowWatcher::OpenWindowJSInternal(nsIDOMWindow*, char const*, char const*, char const*, int, nsIArray*, int, nsIDOMWindow**) [nsWindowWatcher.cpp:606acfb74fb5 : 990 + 0x7] eip = 0x4083adf0 esp = 0xbfc8c314 ebp = 0xbfc8c5bc 22 libxul.so!nsWindowWatcher::OpenWindowJS(nsIDOMWindow*, char const*, char const*, char const*, int, nsIArray*, nsIDOMWindow**) [nsWindowWatcher.cpp:606acfb74fb5 : 487 + 0x1f] eip = 0x4083b533 esp = 0xbfc8c5c4 ebp = 0xbfc8c60c 23 libxul.so!nsGlobalWindow::OpenInternal(nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, int, int, int, int, nsIArray*, nsISupports*, nsIPrincipal*, JSContext*, nsIDOMWindow**) [nsGlobalWindow.cpp:606acfb74fb5 : 7312 + 0x19] eip = 0x406292bc esp = 0xbfc8c614 ebp = 0xbfc8c73c 24 libxul.so!nsGlobalWindow::OpenDialog(nsAString_internal const&, nsAString_internal const&, nsAString_internal const&, nsIDOMWindow**) [nsGlobalWindow.cpp:606acfb74fb5 : 5136 + 0x35] eip = 0x4062968d esp = 0xbfc8c744 ebp = 0xbfc8c7bc 25 libxul.so!NS_GetXPTCallStub_P + 0x30 eip = 0x40a9712b esp = 0xbfc8c7c4 ebp = 0xbfc8c7f8 26 libxul.so!XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode) [xpcwrappednative.cpp:606acfb74fb5 : 2480 + 0x20] eip = 0x401eea70 esp = 0xbfc8c800 ebp = 0xbfc8ca48 27 libxul.so!XPC_WN_CallMethod(JSContext*, JSObject*, unsigned int, int*, int*) [xpcwrappednativejsops.cpp:606acfb74fb5 : 1585 + 0x9] eip = 0x401f4efd esp = 0xbfc8ca50 ebp = 0xbfc8cb18 28 libmozjs.so!js_Invoke [jsinterp.cpp:606acfb74fb5 : 1368 + 0x10] eip = 0x40e7052e esp = 0xbfc8cb20 ebp = 0xbfc8cbe8 29 libmozjs.so!js_Interpret [jsinterp.cpp:606acfb74fb5 : 5088 + 0x20] eip = 0x40e62e84 esp = 0xbfc8cbf0 ebp = 0xbfc8ce58 30 libmozjs.so!js_Invoke [jsinterp.cpp:606acfb74fb5 : 1376 + 0xa] eip = 0x40e70541 esp = 0xbfc8ce60 ebp = 0xbfc8cf18 31 libxul.so!nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS*, unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) [xpcwrappedjsclass.cpp:606acfb74fb5 : 1608 + 0x1a] eip = 0x401ec46d esp = 0xbfc8cf20 ebp = 0xbfc8d0f8 32 libxul.so!nsXPCWrappedJS::CallMethod(unsigned short, XPTMethodDescriptor const*, nsXPTCMiniVariant*) [xpcwrappedjs.cpp:606acfb74fb5 : 561 + 0x13] eip = 0x401e775d esp = 0xbfc8d100 ebp = 0xbfc8d128 33 libxul.so!PrepareAndDispatch [xptcstubs_gcc_x86_unix.cpp:606acfb74fb5 : 95 + 0x1d] eip = 0x40a97c41 esp = 0xbfc8d130 ebp = 0xbfc8d1f8 34 libxul.so!nsThread::ProcessNextEvent(int, int*) [nsThread.cpp:606acfb74fb5 : 510 + 0x5] eip = 0x40a89bc9 esp = 0xbfc8d200 ebp = 0xbfc8d238 35 libxul.so!NS_ProcessNextEvent_P(nsIThread*, int) [nsThreadUtils.cpp : 230 + 0xd] eip = 0x40a574f7 esp = 0xbfc8d240 ebp = 0xbfc8d268 36 libxul.so!nsBaseAppShell::Run() [nsBaseAppShell.cpp:606acfb74fb5 : 170 + 0x9] eip = 0x409b0a06 esp = 0xbfc8d270 ebp = 0xbfc8d288 37 libxul.so!nsAppStartup::Run() [nsAppStartup.cpp:606acfb74fb5 : 192 + 0x5] eip = 0x4087701e esp = 0xbfc8d290 ebp = 0xbfc8d2a8 38 libxul.so!XRE_main [nsAppRunner.cpp:606acfb74fb5 : 3340 + 0x7] eip = 0x401b8e19 esp = 0xbfc8d2b0 ebp = 0xbfc8d8d8 39 firefox-bin!main [nsBrowserApp.cpp:606acfb74fb5 : 156 + 0xe] eip = 0x080495b1 esp = 0xbfc8d8e0 ebp = 0xbfc8d938 40 libc-2.5.so + 0x15deb eip = 0x00aa6dec esp = 0xbfc8d940 ebp = 0xbfc8d9a8

Happened on 1.9.1: http://tinderbox.mozilla.org/showlog.cgi?tree=Firefox3.5&errorparser=unittest&logfile=1239107915.1239114171.27161.gz&buildtime=1239107915&buildname=Linux%20mozilla-1.9.1%20unit%20test&fulltext=1#err1 Windows crashed in the same place, but without a useful stack, so I can't tell if it's the same thing: http://tinderbox.mozilla.org/showlog.cgi?tree=Firefox3.5&errorparser=unittest&logfile=1239111002.1239118751.6012.gz&buildtime=1239111002&buildname=WINNT%205.2%20mozilla-1.9.1%20unit%20test&fulltext=1#err1

Actually this has crashed a few times in a row: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.5/1239107915.1239114171.27161.gz&fulltext=1#err1 http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.5/1239107915.1239114171.27161.gz&fulltext=1#err1 Marco: looks like it might have started with your push on branch: http://hg.mozilla.org/releases/mozilla-1.9.1/pushloghtml?changeset=a90afa3281b4 but I'm not really sure.

so this is crashing more on branch than on trunk? Could be my pushes have make a crash more visible, unluckily branch and trunk code are a bit different atm, on trunk there is better code to manage part of the observers, and that could help. I'm unable to tell if those are directly related to the pushes though. If i should blindly point to a bug i would tell Bug 485458, but i simply think that could have make the crash visible, we could try to temporarly early return that test and see if crashes are stopping. I'll look into the observer code to see if i can find what is causing the crash.

i disabled the test to see if that helps http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d6ae5901e69e

i notice in some of the logs the stack is different 6 libxul.so!nsNavHistoryResult::OnItemAdded(long long, long long, int) [nsNavHistoryResult.cpp:9726056fbac8 : 4134 + 0x25] eip = 0x4094e238 esp = 0xbfc45294 ebp = 0xbfc452dc 7 libxul.so!nsNavBookmarks::CreateContainerWithID(long long, long long, nsACString_internal const&, nsAString_internal const&, int, int*, long long*) [nsNavBookmarks.cpp:9726056fbac8 : 1398 + 0x23] eip = 0x4095bc90 esp = 0xbfc452e4 ebp = 0xbfc4539c 8 libxul.so!nsNavBookmarks::CreateFolder(long long, nsACString_internal const&, int, long long*) [nsNavBookmarks.cpp:9726056fbac8 : 1270 + 0x21]

what is really interesting is that disabling the test helped stopping crashes, but the version of the test that is running on 1.9.1 does not add any bookmark.

After looking some time at the code, i think the crash could be activated by cycle collector, so we could end up calling onItemAdded on a self destroyed node. This patch simply adds some check on the result and the node before calling the observer, i'd like to try pushing this directly to 1.9.1 see if anything changes about those crashes, and then backout it. Unluckily i can't do any other way because crashes are easily reproduceable only on 1.9.1 tinderboxes and random too :(

Comment on attachment 371985 [details] [diff] [review] test patch r=me for exploratorial temporary landing on branch for crash testing.

backed out. http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e17934c35f09 bad news, crashed again while patch was in the tree. http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.5/1239396121.1239402584.5991.gz crashes are now rare like on trunk (on branch won't crash anymore since test is disabled), should check what has landed these days that could have make a difference in reproduceability.

(In reply to comment #10) > bad news, crashed again while patch was in the tree. > http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.5/1239396121.1239402584.5991.gz It crashed in the cycle immediately after that one, too: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.5/1239397188.1239403470.7235.gz

Blocking P1 based on comments indicating that a recent push might be making this crash more visible.

Does anything guarantee that nsNavHistoryResult::OnItemAdded http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsNavHistoryResult.cpp#4166 keeps objects alive when it is calling their OnItemAdded method. As far as I see, for example ENUMERATE_BOOKMARK_FOLDER_OBSERVERS doesn't do that. It just uses the object from array and calls its method. The arrays contains nsRefPtr objects, but that isn't necessarily enough for example if someone removes the object from the array during the method call. Or perhaps the problem is in some other macro. ...or maybe I'm all wrong here. Maybe observers aren't allowed to remove anything from the observer list during the method call. I don't know this code at all :)

err....yeah So, any other time we enumerate our observers, use ENUMERATE_WEAKARRAY, which holds a reference to them just in case they are removed. These, http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsNavHistoryResult.cpp#4102, need to do the same as ENUMERATE_WEAKARRAY I think. This has been around since forever :(

Actually, ENUMERATE_BOOKMARK_FOLDER_OBSERVERS looks pretty safe, since it creates a copy of nsTArray<nsRefPtr<...> > array. But the other macros make a copy of nsTArray<SomeObject*>. So for example some object may be removed from mAllBookmarksObservers, but ENUMERATE_ALL_BOOKMARKS_OBSERVERS may still use that object. And nothing guarantees that the object is alive.

Hey Marco - marking this as assigned to you based on bug activity (since no blocker should be assigned to nobody@) - feel free to re-assign if you see fit.

iirc there was no code change that triggered this. it was a suite of tests. crash stats shows that this crash was reported 3 times in the last week, across all Firefox versions. Once was 3.0.7, and twice was 3.0b3 (!!!). while we should certainly fix this asap, it doesn't seem like a P1 blocker to me.

I'd buy that. --> P2

indeed the increased rate of seeing the bug is simply a test that looks like increasing the crash rate on branch tinderboxes, not related to any code change. The biggest problem is there's no way to reproduce locally and see if a patch would really fix the issue.

I think smaug is spot on with comment 13 though. That is likely the cause of our crashes.

i have an half done patch but still looking at some detail.

Sorry for the fancy name, but i crash due to bug 488311 (or similar) and i must avoid autocomplete popups on bugzilla's forms. Asking Shawn a first look.

Comment on attachment 372736 [details] [diff] [review] please don't crash. >+++ b/browser/components/places/src/nsPlacesTransactionsService.js >+ // undo transactions should always be done in reverse order. >+ for (var i = this._childTransactions.length - 1; i >= 0; i--) { > var txn = this._childTransactions[i]; > txn.undoTransaction(); > } >+ PlacesUtils.bookmarks.removeItem(this._id); So, this was always wrong right? Can we please have a chrome tests added for this (removing the item, not the order)? >+++ b/browser/components/places/tests/browser/browser_425884.js >+ testRootNode.containerOpen = false; >+ toolbarNode.containerOpen = false; A comment explaining why we have to do this please. >+++ b/toolkit/components/places/src/nsNavHistoryResult.cpp >+nsNavHistoryContainerResultNode::~nsNavHistoryContainerResultNode() { >+ mChildren.Clear(); > } comment here as well, although it's probably self explanatory. nit: brace on newline >+nsNavHistoryQueryResultNode::~nsNavHistoryQueryResultNode() { >+ if (mResult && mResult->mAllBookmarksObservers.IndexOf(this) != >+ mResult->mAllBookmarksObservers.NoIndex) >+ mResult->RemoveAllBookmarksObserver(this); >+ if (mResult && mResult->mHistoryObservers.IndexOf(this) != >+ mResult->mHistoryObservers.NoIndex) >+ mResult->RemoveHistoryObserver(this); ditto > static PLDHashOperator > RemoveBookmarkFolderObserversCallback(nsTrimInt64HashKey::KeyType aKey, > nsNavHistoryResult::FolderObserverList*& aData, > void* userArg) > { >+ aData->Clear(); > delete aData; Do we actually need to clear here? Isn't this called in the destructor now? >+#define ENUMERATE_QUERY_OBSERVERS(_functionCall, _observersList, _conditionCall) \ >+ { \ >+ QueryObserverList _listCopy(_observersList); \ >+ for (PRUint32 _obs_i = 0; _obs_i < _listCopy.Length(); _obs_i ++) { \ >+ if (_listCopy[_obs_i] && _listCopy[_obs_i]->_conditionCall) \ >+ _listCopy[_obs_i]->_functionCall; \ >+ } \ >+ } PR_BEGIN_MACRO and PR_END_MACRO instead of braces please Do we understand how exactly this crashes now? Can we make a test that crashes before the fix, and fails afterwards?

(In reply to comment #23) > (From update of attachment 372736 [details] [diff] [review]) > >+++ b/browser/components/places/src/nsPlacesTransactionsService.js > >+ // undo transactions should always be done in reverse order. > >+ for (var i = this._childTransactions.length - 1; i >= 0; i--) { > > var txn = this._childTransactions[i]; > > txn.undoTransaction(); > > } > >+ PlacesUtils.bookmarks.removeItem(this._id); > So, this was always wrong right? Can we please have a chrome tests added for > this (removing the item, not the order)? i'm thinking if it would change anything, the item is removed even now, because removeItem is called before undoing child transactions, but child transactions undo will throw. i can probably test that it doesn't throw in xpcshell. > > static PLDHashOperator > > RemoveBookmarkFolderObserversCallback(nsTrimInt64HashKey::KeyType aKey, > > nsNavHistoryResult::FolderObserverList*& aData, > > void* userArg) > > { > >+ aData->Clear(); > > delete aData; > Do we actually need to clear here? Isn't this called in the destructor now? this is an hash containing TArray of folderResultNodes, i prefer being explicit in the destruction of every node in the array, this method is called by cycle collection unlinker that for TArrays calls Clear(). > Do we understand how exactly this crashes now? Can we make a test that crashes > before the fix, and fails afterwards? I've tried but i wasn't able to create such a test, so i can't test if this patch fixes the crash since it is visible only on tinderboxes. Smaug's theory is valid imho, cycle collector could remove some node after we got the observer's list copy, so we end up calling a method on a dangling pointer.

i had to clean up some warning and js strict warning because there were too many, and was hard to find errors in the middle of browser chrome tests output. Added a test to ensure undo a createItem/createTolder transaction with child transactions won't throw. Still unable to build a crash test as of now.

Comment on attachment 372897 [details] [diff] [review] patch v1.2 >+ // Close containers, this will remove their observers. nit: you want a period here, not a comma. >+ // Close containers, cleaning up their observers. this one is OK, so take your pick >+ // Create a folder with child item transactions nit: period at end please >+nsNavHistoryContainerResultNode::~nsNavHistoryContainerResultNode() >+{ >+ // Explicitly clean up array of children of this container, we must ensure >+ // all references are gone and all of their destructors are called. Again, you want a period. >+nsNavHistoryQueryResultNode::~nsNavHistoryQueryResultNode() { >+ // Remove this node from result's observers, we don't need to be notified >+ // anymore. again > static PLDHashOperator > RemoveBookmarkFolderObserversCallback(nsTrimInt64HashKey::KeyType aKey, > nsNavHistoryResult::FolderObserverList*& aData, > void* userArg) > { >+ // Explicitly remove nodes from list of observers, we must ensure >+ // all references are gone and all of their destructors are called. ditto This looks OK to me, but I want dietrich to look too since you are about the only person who really knows this code well. r=sdwilsh

addressed sdwilsh's comments, moving to dietrich.

Comment on attachment 373219 [details] [diff] [review] patch v1.3 >+ // Close containers, cleaning up their observers. >+ testRootNode.containerOpen = false; >+ toolbarNode.containerOpen = false; isn't the point of these changes that this cleanup will occur in the dtor? why do you have to do this manually? >+ // Test creating an item with child transactions. >+ var newDateAdded = Date.now() - 20000; >+ var childTxn = ptSvc.editItemDateAdded(null, newDateAdded); >+ var itemWChildTxn = ptSvc.createItem(uri("http://www.example.com"), root, bmStartIndex, "Testing1", null, null, [childTxn]); >+ try { >+ ptSvc.doTransaction(itemWChildTxn); //Also testing doTransaction mega-nit: space after // >+ var itemId = (bmsvc.getBookmarkIdsForURI(uri("http://www.example.com"), {}))[0]; >+ do_check_eq(observer._itemAddedId, itemId); >+ do_check_eq(newDateAdded, bmsvc.getItemDateAdded(itemId)); >+ itemWChildTxn.undoTransaction(); >+ do_check_eq(observer._itemRemovedId, itemId); >+ itemWChildTxn.redoTransaction(); >+ do_check_true(bmsvc.isBookmarked(uri("http://www.example.com"))); >+ var newId = (bmsvc.getBookmarkIdsForURI(uri("http://www.example.com"), {}))[0]; >+ do_check_eq(newDateAdded, bmsvc.getItemDateAdded(newId)); >+ do_check_eq(observer._itemAddedId, newId); >+ itemWChildTxn.undoTransaction(); >+ do_check_eq(observer._itemRemovedId, newId); >+ } catch (ex) { >+ do_throw("Setting a child transaction in a createItem transaction did throw: " + ex); >+ } >+ >+ // Create a folder with child item transactions. >+ var childItemTxn = ptSvc.createItem(uri("http://www.childItem.com"), root, bmStartIndex, "childItem"); >+ var folderWChildItemTxn = ptSvc.createFolder("Folder", root, bmStartIndex, null, [childItemTxn]); >+ try { >+ ptSvc.doTransaction(folderWChildItemTxn); >+ var childItemId = (bmsvc.getBookmarkIdsForURI(uri("http://www.childItem.com"), {}))[0]; >+ do_check_eq(observer._itemAddedId, childItemId); >+ do_check_eq(observer._itemAddedIndex, 0); >+ do_check_true(bmsvc.isBookmarked(uri("http://www.childItem.com"))); >+ folderWChildItemTxn.undoTransaction(); >+ do_check_false(bmsvc.isBookmarked(uri("http://www.childItem.com"))); >+ folderWChildItemTxn.redoTransaction(); >+ newchildItemId = (bmsvc.getBookmarkIdsForURI(uri("http://www.childItem.com"), {}))[0]; >+ do_check_eq(observer._itemAddedIndex, 0); >+ do_check_eq(observer._itemAddedId, newchildItemId); >+ do_check_true(bmsvc.isBookmarked(uri("http://www.childItem.com"))); >+ folderWChildItemTxn.undoTransaction(); >+ do_check_false(bmsvc.isBookmarked(uri("http://www.childItem.com"))); >+ } catch (ex) { >+ do_throw("Setting a child item transaction in a createFolder transaction did throw: " + ex); >+ } why wrap these in try/catch blocks? >@@ -3453,16 +3458,19 @@ nsNavHistoryFolderResultNode::OnItemAdde > ReindexRange(aIndex, PR_INT32_MAX, 1); > > nsRefPtr<nsNavHistoryResultNode> node; > if (itemType == nsINavBookmarksService::TYPE_BOOKMARK) { > nsNavHistory* history = nsNavHistory::GetHistoryService(); > NS_ENSURE_TRUE(history, NS_ERROR_OUT_OF_MEMORY); > rv = history->BookmarkIdToResultNode(aItemId, mOptions, getter_AddRefs(node)); > NS_ENSURE_SUCCESS(rv, rv); >+ // Correctly set batch status for new query nodes. >+ if (mResult && node->IsQuery()) >+ node->GetAsQuery()->mBatchInProgress = mResult->mBatchInProgress; > } > else if (itemType == nsINavBookmarksService::TYPE_FOLDER || > itemType == nsINavBookmarksService::TYPE_DYNAMIC_CONTAINER) { > rv = bookmarks->ResultNodeForContainer(aItemId, mOptions, getter_AddRefs(node)); > NS_ENSURE_SUCCESS(rv, rv); > } > else if (itemType == nsINavBookmarksService::TYPE_SEPARATOR) { > node = new nsNavHistorySeparatorResultNode(); does this also need to be done for other container node types as well? r=me. please request an additional review pass from smaug.

(In reply to comment #28) > (From update of attachment 373219 [details] [diff] [review]) > why wrap these in try/catch blocks? because they were throwing, we were removing an item and then trying to add annotations to it (causing an exception) > >+ // Correctly set batch status for new query nodes. > >+ if (mResult && node->IsQuery()) > >+ node->GetAsQuery()->mBatchInProgress = mResult->mBatchInProgress; > > does this also need to be done for other container node types as well? the only interfaces using batchInProgress are queryNode and result... the one that is warning all around is basically queryNode. Other containers as of now are not interested in batches.

Comment on attachment 373219 [details] [diff] [review] patch v1.3 Smaug, could you please take a look at observers changes and tell if it's about what you were thinking to?

Comment on attachment 373219 [details] [diff] [review] patch v1.3 >+nsNavHistoryQueryResultNode::~nsNavHistoryQueryResultNode() { >+ // Remove this node from result's observers. We don't need to be notified >+ // anymore. >+ if (mResult && mResult->mAllBookmarksObservers.IndexOf(this) != >+ mResult->mAllBookmarksObservers.NoIndex) >+ mResult->RemoveAllBookmarksObserver(this); >+ if (mResult && mResult->mHistoryObservers.IndexOf(this) != >+ mResult->mHistoryObservers.NoIndex) >+ mResult->RemoveHistoryObserver(this); >+} Is this really needed? mHistoryObservers and mAllBookmarksObservers have strong pointer to |this|, so destructor shouldn't be called if the object is still in mResult->mXXXObservers lists, right? Perhaps replace with an assertion? > nsNavHistoryResult::~nsNavHistoryResult() > { > // delete all bookmark folder observer arrays which are allocated on the heap > mBookmarkFolderObservers.Enumerate(&RemoveBookmarkFolderObserversCallback, nsnull); >+ // Remove all other observers. >+ mAllBookmarksObservers.Clear(); >+ mHistoryObservers.Clear(); nsTArray calls Clear() in its destructor, so why to call Clear() explicitly here?

(In reply to comment #31) > (From update of attachment 373219 [details] [diff] [review]) > >+nsNavHistoryQueryResultNode::~nsNavHistoryQueryResultNode() { > >+ // Remove this node from result's observers. We don't need to be notified > >+ // anymore. > >+ if (mResult && mResult->mAllBookmarksObservers.IndexOf(this) != > >+ mResult->mAllBookmarksObservers.NoIndex) > >+ mResult->RemoveAllBookmarksObserver(this); > >+ if (mResult && mResult->mHistoryObservers.IndexOf(this) != > >+ mResult->mHistoryObservers.NoIndex) > >+ mResult->RemoveHistoryObserver(this); > >+} > Is this really needed? mHistoryObservers and mAllBookmarksObservers > have strong pointer to |this|, so destructor shouldn't be called if > the object is still in mResult->mXXXObservers lists, right? > Perhaps replace with an assertion? but there's a cycle, and looks like the cycle collector could remove the query node before the result itself is destroyed. indeed adding an assertion i see the node is destroyed by the cycle collector before the result is.

fixed comments (apart last one, see comment above).

not marking in-testsuite since even if this adds a test it is not related to the crash itself.

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8077dff341df re-enabled test browser_bookmarkProperties.js, waiting to see if crashes are still there.

No crashes reported anymore on trunk and 1.9.1.