Closed Bug 487040 Opened 15 years ago Closed 15 years ago

Crash [@ nsNavHistoryResult::OnItemAdded ] in mochitest-browser-chrome

Categories

(Toolkit :: Places, defect, P2)

Product:
Toolkit
Component:
Places
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.9.2a1

People

(Reporter: ted, Assigned: mak)

References

(
URL
)

Details

(Keywords: crash, intermittent-failure, verified1.9.1)

Crash Data

Attachments

(1 file, 4 obsolete files)

patch v1.4
25.70 KB, patch
Details | Diff | Splinter Review 
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
Blocks: 438871
Whiteboard: [orange]
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 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.
OS: Linux → All
Hardware: x86 → All
Attached patch test patch (obsolete) — — Splinter Review 
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 :(
Attachment #371985 - Flags: review?(dietrich)
Comment on attachment 371985 [details] [diff] [review]
test patch

r=me for exploratorial temporary landing on branch for crash testing.
Attachment #371985 - Flags: review?(dietrich) → review+
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
Flags: blocking1.9.1?
Blocking P1 based on comments indicating that a recent push might be making this crash more visible.
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P1
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.
Assignee: nobody → mak77
Severity: normal → critical
Keywords: crash
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
Priority: P1 → 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.
Attached patch please don't crash. (obsolete) — — Splinter Review 
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.
Attachment #371985 - Attachment is obsolete: true
Attachment #372736 - Flags: review?(sdwilsh)
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?
Attachment #372736 - Flags: review?(sdwilsh) → review-
(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.
Attached patch patch v1.2 (obsolete) — — Splinter Review 
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.
Attachment #372736 - Attachment is obsolete: true
Attachment #372897 - Flags: review?(sdwilsh)
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
Attachment #372897 - Flags: review?(sdwilsh)
Attachment #372897 - Flags: review?(dietrich)
Attachment #372897 - Flags: review+
Attached patch patch v1.3 (obsolete) — — Splinter Review 
addressed sdwilsh's comments, moving to dietrich.
Attachment #372897 - Attachment is obsolete: true
Attachment #373219 - Flags: review?(dietrich)
Attachment #372897 - Flags: review?(dietrich)
Attachment #373219 - Flags: review?(dietrich) → review+
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?
Attachment #373219 - Flags: review?(Olli.Pettay)
Whiteboard: [orange] → [orange][has patch][needs review smaug]
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?
Attachment #373219 - Flags: review?(Olli.Pettay) → review+
Whiteboard: [orange][has patch][needs review smaug] → [orange][has patch]
(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.
Attached patch patch v1.4 — — Splinter Review 
fixed comments (apart last one, see comment above).
Attachment #373219 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/c43a4464f76c
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
not marking in-testsuite since even if this adds a test it is not related to the crash itself.
Whiteboard: [orange][has patch] → [orange]
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.
Keywords: fixed1.9.1
No crashes reported anymore on trunk and 1.9.1.
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
Crash Signature: [@ nsNavHistoryResult::OnItemAdded ]
Whiteboard: [orange]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: