Closed
Bug 487040
Opened 16 years ago
Closed 16 years ago
Crash [@ nsNavHistoryResult::OnItemAdded ] in mochitest-browser-chrome
Categories
(Toolkit :: Places, defect, P2)
Toolkit
Places
Tracking
()
VERIFIED
FIXED
mozilla1.9.2a1
People
(Reporter: ted, Assigned: mak)
References
()
Details
(Keywords: crash, intermittent-failure, verified1.9.1)
Crash Data
Attachments
(1 file, 4 obsolete files)
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
Reporter | ||
Comment 1•16 years ago
|
||
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
Summary: Crash [@ nsNavHistoryResult::OnItemAdded ] in mochitest-chrome → Crash [@ nsNavHistoryResult::OnItemAdded ] in mochitest-browser-chrome
Reporter | ||
Comment 2•16 years ago
|
||
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.
Assignee | ||
Comment 3•16 years ago
|
||
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.
Assignee | ||
Comment 4•16 years ago
|
||
i disabled the test to see if that helps
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d6ae5901e69e
Assignee | ||
Comment 5•16 years ago
|
||
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]
Assignee | ||
Comment 6•16 years ago
|
||
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.
Updated•16 years ago
|
OS: Linux → All
Hardware: x86 → All
Assignee | ||
Comment 7•16 years ago
|
||
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 8•16 years ago
|
||
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+
Assignee | ||
Comment 9•16 years ago
|
||
Assignee | ||
Comment 10•16 years ago
|
||
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.
Comment 11•16 years ago
|
||
(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
Updated•16 years ago
|
Flags: blocking1.9.1?
Comment 12•16 years ago
|
||
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
Comment 13•16 years ago
|
||
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 :)
Comment 14•16 years ago
|
||
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 :(
Comment 15•16 years ago
|
||
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.
Comment 16•16 years ago
|
||
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
Comment 17•16 years ago
|
||
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.
Assignee | ||
Comment 19•16 years ago
|
||
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.
Comment 20•16 years ago
|
||
I think smaug is spot on with comment 13 though. That is likely the cause of our crashes.
Assignee | ||
Comment 21•16 years ago
|
||
i have an half done patch but still looking at some detail.
Assignee | ||
Comment 22•16 years ago
|
||
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 23•16 years ago
|
||
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-
Assignee | ||
Comment 24•16 years ago
|
||
(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.
Assignee | ||
Comment 25•16 years ago
|
||
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 26•16 years ago
|
||
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+
Assignee | ||
Comment 27•16 years ago
|
||
addressed sdwilsh's comments, moving to dietrich.
Attachment #372897 -
Attachment is obsolete: true
Attachment #373219 -
Flags: review?(dietrich)
Attachment #372897 -
Flags: review?(dietrich)
Updated•16 years ago
|
Attachment #373219 -
Flags: review?(dietrich) → review+
Comment 28•16 years ago
|
||
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.
Assignee | ||
Comment 29•16 years ago
|
||
(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.
Assignee | ||
Comment 30•16 years ago
|
||
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)
Updated•16 years ago
|
Whiteboard: [orange] → [orange][has patch][needs review smaug]
Comment 31•16 years ago
|
||
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+
Updated•16 years ago
|
Whiteboard: [orange][has patch][needs review smaug] → [orange][has patch]
Assignee | ||
Comment 32•16 years ago
|
||
(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.
Assignee | ||
Comment 33•16 years ago
|
||
fixed comments (apart last one, see comment above).
Attachment #373219 -
Attachment is obsolete: true
Assignee | ||
Comment 34•16 years ago
|
||
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Assignee | ||
Comment 35•16 years ago
|
||
not marking in-testsuite since even if this adds a test it is not related to the crash itself.
Whiteboard: [orange][has patch] → [orange]
Assignee | ||
Comment 36•16 years ago
|
||
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
Comment 37•16 years ago
|
||
No crashes reported anymore on trunk and 1.9.1.
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
Updated•13 years ago
|
Crash Signature: [@ nsNavHistoryResult::OnItemAdded ]
Updated•12 years ago
|
Keywords: intermittent-failure
Updated•12 years ago
|
Whiteboard: [orange]
You need to log in
before you can comment on or make changes to this bug.
Description
•