389782 - history sidebar doesn't show the same results as fx 2, duplicate uris when grouped by site, last visited most visited

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

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

history sidebar doesn't show the same results as fx 2, duplicate uris when grouped by site, last visited most visited

I'm splitting out the fix for this from bug #385245, which will just track the optimization.

fix in hand.

Comment 1

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

Attachment: patch, includes fix for bug #381898

Comment 2

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

> includes fix for bug #381898

note to dietrich, that is the nsNavHistory.cpp part of this fix, but we need it for this bug.

now, for the history sidebar (when last visited mode) we do what fx 2 does, and get results by uri (like we do for the history menu, but there we limit to the at most 10 results).

Comment 3

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

note, but switching to having results as uris, we can optimize in treeView.js to avoid calling collapseDuplicates() (because there are no duplicates).  see bug #385245

going forward, for the views where can't do results as uris (and we must do results as visits), I think we should also collapse duplicates in C++ in the result (and not in treeView.js).  that should help performance and fix bugs like #386399 ("History folder children often duplicated, resulting in bug-like behavior elsewhere")

Comment 4

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

Comment on attachment 274111 [details] [diff] [review]
patch, includes fix for bug #381898

more is needed here, as with this patch I delete on crash now.

Comment 5

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

here's the stack to the crash, working on a fix:

I crash in OnDeleteURI() because the nodes's mParent is null.

 	xpcom_core.dll!nsVoidArray::IndexOf(void * aPossibleElement=0x06ffcd28)  Line 381 + 0x3 bytes	C++
 	xpcom_core.dll!nsCOMArray_base::IndexOf(nsISupports * aObject=0x06ffcd28)  Line 60	C++
 	places.dll!nsCOMArray<nsNavHistoryResultNode>::IndexOf(nsNavHistoryResultNode * aObject=0x06ffcd28)  Line 178	C++
 	places.dll!nsNavHistoryContainerResultNode::FindChild(nsNavHistoryResultNode * aNode=0x06ffcd28)  Line 623 + 0x16 bytes	C++
>	places.dll!nsNavHistoryQueryResultNode::OnDeleteURI(nsIURI * aURI=0x0643d000)  Line 2479 + 0xc bytes	C++
 	places.dll!nsNavHistoryResult::OnDeleteURI(nsIURI * aURI=0x0643d000)  Line 3927 + 0x66 bytes	C++
 	places.dll!nsNavHistory::RemovePage(nsIURI * aURI=0x0643d000)  Line 2581 + 0x7b bytes	C++
 	xpcom_core.dll!NS_InvokeByIndex_P(nsISupports * that=0x00000009, unsigned int methodIndex=1, unsigned int paramCount=1235096, nsXPTCVariant * params=0x0653fa58)  Line 102	C++
 	xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, XPCWrappedNative::CallMode mode=9)  Line 2277 + 0x1e bytes	C++
 	xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, XPCWrappedNative::CallMode mode=CALL_METHOD)  Line 2277 + 0x1e bytes	C++
 	xpc3250.dll!XPC_WN_CallMethod(JSContext * cx=0x06501730, JSObject * obj=0x06f55500, unsigned int argc=1, long * argv=0x0704ea78, long * vp=0x0012db34)  Line 1467 + 0xe bytes	C++
 	js3250.dll!js_Invoke(JSContext * cx=0x06501730, unsigned int argc=1, unsigned int flags=0)  Line 1312 + 0x20 bytes	C
 	js3250.dll!js_Interpret(JSContext * cx=0x06501730, unsigned char * pc=0x045a6241, long * result=0x0012e1f0)  Line 4024 + 0xf bytes	C
 	js3250.dll!js_Invoke(JSContext * cx=0x06501730, unsigned int argc=1, unsigned int flags=2)  Line 1331 + 0x13 bytes	C
 	xpc3250.dll!nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS * wrapper=0x06f5fa68, unsigned short methodIndex=5, const XPTMethodDescriptor * info=0x0422ea78, nsXPTCMiniVariant * nativeParams=0x0012e554)  Line 1457 + 0x14 bytes	C++
 	xpc3250.dll!nsXPCWrappedJS::CallMethod(unsigned short methodIndex=5, const XPTMethodDescriptor * info=0x0422ea78, nsXPTCMiniVariant * params=0x0012e554)  Line 566	C++
 	xpcom_core.dll!PrepareAndDispatch(nsXPTCStubBase * self=0x06f5f880, unsigned int methodIndex=5, unsigned int * args=0x0012e614, unsigned int * stackBytesToPop=0x0012e604)  Line 114 + 0x21 bytes	C++
 	xpcom_core.dll!SharedStub()  Line 142	C++
 	xpcom_core.dll!NS_InvokeByIndex_P(nsISupports * that=0x00000005, unsigned int methodIndex=1, unsigned int paramCount=1238920, nsXPTCVariant * params=0x0012e640)  Line 102	C++
 	xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, XPCWrappedNative::CallMode mode=77192968)  Line 2277 + 0x1e bytes	C++
 	xpcom_core.dll!NS_InvokeByIndex_P(nsISupports * that=0x00000005, unsigned int methodIndex=1, unsigned int paramCount=1238920, nsXPTCVariant * params=0x0012e640)  Line 102	C++
 	xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, XPCWrappedNative::CallMode mode=5)  Line 2277 + 0x1e bytes	C++
 	xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, XPCWrappedNative::CallMode mode=CALL_METHOD)  Line 2277 + 0x1e bytes	C++
 	xpc3250.dll!XPC_WN_CallMethod(JSContext * cx=0x06501730, JSObject * obj=0x03e036a0, unsigned int argc=1, long * argv=0x0704e914, long * vp=0x0012ea24)  Line 1467 + 0xe bytes	C++
 	js3250.dll!js_Invoke(JSContext * cx=0x06501730, unsigned int argc=1, unsigned int flags=0)  Line 1312 + 0x20 bytes	C
 	js3250.dll!js_Interpret(JSContext * cx=0x06501730, unsigned char * pc=0x044d1755, long * result=0x0012f0e0)  Line 4024 + 0xf bytes	C
 	js3250.dll!js_Invoke(JSContext * cx=0x06501730, unsigned int argc=1, unsigned int flags=2)  Line 1331 + 0x13 bytes	C
 	js3250.dll!js_InternalInvoke(JSContext * cx=0x06501730, JSObject * obj=0x00f63f80, long fval=77279744, unsigned int flags=0, unsigned int argc=1, long * argv=0x0704e860, long * rval=0x0012f268)  Line 1406 + 0x14 bytes	C
 	js3250.dll!JS_CallFunctionValue(JSContext * cx=0x06501730, JSObject * obj=0x00f63f80, long fval=77279744, unsigned int argc=1, long * argv=0x0704e860, long * rval=0x0012f268)  Line 4846 + 0x1f bytes	C
 	gklayout.dll!nsJSContext::CallEventHandler(nsISupports * aTarget=0x0648fc28, void * aScope=0x0655ad00, void * aHandler=0x049b3200, nsIArray * aargv=0x064bbbe8, nsIVariant * * arv=0x0012f3d4)  Line 1851 + 0x24 bytes	C++
 	gklayout.dll!nsJSEventListener::HandleEvent(nsIDOMEvent * aEvent=0x044f07a4)  Line 215 + 0x67 bytes	C++
 	gklayout.dll!nsEventListenerManager::HandleEventSubType(nsListenerStruct * aListenerStruct=0x0648fd28, nsIDOMEventListener * aListener=0x0648fcd8, nsIDOMEvent * aDOMEvent=0x044f07a4, nsISupports * aCurrentTarget=0x0648fc28, unsigned int aPhaseFlags=6)  Line 1096 + 0x12 bytes	C++
 	gklayout.dll!nsEventListenerManager::HandleEvent(nsPresContext * aPresContext=0x0650c5e8, nsEvent * aEvent=0x0012f744, nsIDOMEvent * * aDOMEvent=0x0012f6a4, nsISupports * aCurrentTarget=0x0648fc28, unsigned int aFlags=6, nsEventStatus * aEventStatus=0x0012f6a8)  Line 1216	C++
 	gklayout.dll!nsEventTargetChainItem::HandleEvent(nsEventChainPostVisitor & aVisitor={...}, unsigned int aFlags=6)  Line 202	C++
 	gklayout.dll!nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor & aVisitor={...}, unsigned int aFlags=6, nsDispatchingCallback * aCallback=0x00000000)  Line 260	C++
 	gklayout.dll!nsEventDispatcher::Dispatch(nsISupports * aTarget=0x0648fc28, nsPresContext * aPresContext=0x0650c5e8, nsEvent * aEvent=0x0012f744, nsIDOMEvent * aDOMEvent=0x00000000, nsEventStatus * aEventStatus=0x0012f78c, nsDispatchingCallback * aCallback=0x00000000)  Line 473 + 0x12 bytes	C++
 	gklayout.dll!nsXULElement::PreHandleEvent(nsEventChainPreVisitor & aVisitor={...})  Line 1510 + 0x29 bytes	C++
 	gklayout.dll!nsEventTargetChainItem::PreHandleEvent(nsEventChainPreVisitor & aVisitor={...})  Line 184 + 0x1c bytes	C++
 	gklayout.dll!nsEventDispatcher::Dispatch(nsISupports * aTarget=0x06ee3a28, nsPresContext * aPresContext=0x0650c5e8, nsEvent * aEvent=0x0012f958, nsIDOMEvent * aDOMEvent=0x00000000, nsEventStatus * aEventStatus=0x0012f94c, nsDispatchingCallback * aCallback=0x00000000)  Line 432	C++
 	gklayout.dll!PresShell::HandleDOMEventWithTarget(nsIContent * aTargetContent=0x06ee3a28, nsEvent * aEvent=0x0012f958, nsEventStatus * aStatus=0x0012f94c)  Line 5777 + 0x1c bytes	C++
 	gklayout.dll!nsXULMenuCommandEvent::Run()  Line 1630	C++
 	xpcom_core.dll!nsThread::ProcessNextEvent(int mayWait=1, int * result=0x0012fa04)  Line 491	C++
 	xpcom_core.dll!NS_ProcessNextEvent_P(nsIThread * thread=0x00bcc298, int mayWait=1)  Line 227 + 0x16 bytes	C++
 	gkwidget.dll!nsBaseAppShell::Run()  Line 154 + 0xc bytes	C++
 	tkitcmps.dll!nsAppStartup::Run()  Line 170 + 0x1c bytes	C++
 	xul.dll!XRE_main(int argc=1, char * * argv=0x00bc8aa8, const nsXREAppData * aAppData=0x00bc8e88)  Line 3057 + 0x25 bytes	C++
 	firefox.exe!main(int argc=1, char * * argv=0x00bc8aa8)  Line 153 + 0x12 bytes	C++
 	firefox.exe!__tmainCRTStartup()  Line 586 + 0x19 bytes	C
 	firefox.exe!mainCRTStartup()  Line 403	C
 	kernel32.dll!7c816fd7() 	
 	[Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll]	

Comment 6

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

timeless, you'd only crash if you had my patch (which is not checked in.)

Comment 7

[timeless]

ah, whoops

Comment 8

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

Attachment: patch, excludes fix for #381898

Comment 9

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

I have not able to make it crash again, but I did turn the assert into a NS_ENSURE_TRUE(parent, NS_ERROR_UNEXPECTED); to help track down / prevent the crash.

Note, this fix will allow for some performance improvements, see comment #3.

Comment 10

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

Attachment: patch

this patch includes a problem that dietrich spotted which is:

1)  order by last visited (or most visited)
2)  click on a link in the history sidebar, where the visit should change the order

expected results:

the selected item in the history sidebar would change positions

actual results:

the item would not change positions

the problem is that our nsNavHistoryContainerResultNode (for place:sort=4, for example) doesn't have a parent, so we can't use ReplaceChildURIAt() because it uses ReverseUpdateStats() which requires a parent.

Comment 11

[Dietrich Ayala (:dietrich)]

Comment on attachment 274877 [details] [diff] [review]
patch


>+  options.queryType = NHQO.QUERY_TYPE_HISTORY;

i think this is default

>+  options.resultType = resultType;
>+
>+  query.onlyBookmarked = false;

i think this is default as well.


>@@ -2469,18 +2479,19 @@ nsNavHistoryQueryResultNode::OnDeleteURI
>   nsCOMArray<nsNavHistoryResultNode> matches;
>   RecursiveFindURIs(onlyOneEntry, this, spec, &matches);
>   if (matches.Count() == 0)
>     return NS_OK; // not found
> 
>   for (PRInt32 i = 0; i < matches.Count(); i ++) {
>     nsNavHistoryResultNode* node = matches[i];
>     nsNavHistoryContainerResultNode* parent = node->mParent;
>-    NS_ASSERTION(parent, "URI nodes should always have parents");
>-
>+    // URI nodes should always have parents
>+    NS_ENSURE_TRUE(parent, NS_ERROR_UNEXPECTED);
>+    
>     PRInt32 childIndex = parent->FindChild(node);
>     NS_ASSERTION(childIndex >= 0, "Child not found in parent");
>     parent->RemoveChildAt(childIndex);
> 
>     if (parent->mChildren.Count() == 0 && parent->IsQuerySubcontainer()) {
>       // when query subcontainers (like hosts) get empty we should remove them
>       // as well. Just append this to our list and it will get evaluated later
>       // in the loop.

i'm not clear on why a node (besides the root) would not have a parent

Comment 12

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

Attachment: updated patch, per dietrich's comments

Comment 13

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

Attachment: updated patch, per dietrich (without the NS_BREAK())

Comment 14

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

Attachment: as checked in

update to the trunk

Comment 15

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

fixed.

Checking in browser/components/places/content/controller.js;
/cvsroot/mozilla/browser/components/places/content/controller.js,v  <--  control
ler.js
new revision: 1.173; previous revision: 1.172
done
Checking in browser/components/places/content/history-panel.js;
/cvsroot/mozilla/browser/components/places/content/history-panel.js,v  <--  hist
ory-panel.js
new revision: 1.22; previous revision: 1.21
done
Checking in toolkit/components/places/src/nsNavHistoryResult.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp,v  <--  ns
NavHistoryResult.cpp
new revision: 1.110; previous revision: 1.109
done

Comment 16

[Al Billings [:abillings - ex-MoCo]]

Seth, do you have a places.sqlite that displays this problem? I haven't been able to reproduce this from the description here.

Comment 17

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv