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

RESOLVED FIXED in Firefox 3 alpha8

Status

()

RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: moco, Assigned: moco)

Tracking

Trunk
Firefox 3 alpha8
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

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.
Flags: blocking-firefox3?
Blocks: 381898, 385245
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 3 M8
Created attachment 274111 [details] [diff] [review]
patch, includes fix for bug #381898
Attachment #274111 - Flags: review?(dietrich)
> 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).
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 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.
Attachment #274111 - Flags: review?(dietrich)
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]	

Updated

11 years ago
Severity: normal → critical
Keywords: crash
Summary: history sidebar doesn't show the same results as fx 2, duplicate uris when grouped by site, last visited most visited → history sidebar doesn't show the same results as fx 2, duplicate uris when grouped by site, last visited most visited [@ nsVoidArray::IndexOf]
timeless, you'd only crash if you had my patch (which is not checked in.)
Summary: history sidebar doesn't show the same results as fx 2, duplicate uris when grouped by site, last visited most visited [@ nsVoidArray::IndexOf] → history sidebar doesn't show the same results as fx 2, duplicate uris when grouped by site, last visited most visited

Comment 7

11 years ago
ah, whoops
Severity: critical → normal
Keywords: crash
Flags: blocking-firefox3? → blocking-firefox3+
Created attachment 274699 [details] [diff] [review]
patch, excludes fix for #381898
Attachment #274111 - Attachment is obsolete: true
Attachment #274699 - Flags: review?(dietrich)
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.
Created attachment 274877 [details] [diff] [review]
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.
Attachment #274699 - Attachment is obsolete: true
Attachment #274877 - Flags: review?(dietrich)
Attachment #274699 - Flags: review?(dietrich)
No longer blocks: 381898
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
Created attachment 275074 [details] [diff] [review]
updated patch, per dietrich's comments
Attachment #274877 - Attachment is obsolete: true
Attachment #275074 - Flags: review?(dietrich)
Attachment #274877 - Flags: review?(dietrich)
Attachment #275074 - Attachment is obsolete: true
Attachment #275074 - Flags: review?(dietrich)
Created attachment 275075 [details] [diff] [review]
updated patch, per dietrich (without the NS_BREAK())
Attachment #275075 - Flags: review?(dietrich)
Attachment #275075 - Flags: review?(dietrich) → review+
Created attachment 275081 [details] [diff] [review]
as checked in

update to the trunk
Attachment #275075 - Attachment is obsolete: true
Attachment #275081 - Flags: review+
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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Seth, do you have a places.sqlite that displays this problem? I haven't been able to reproduce this from the description here.
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.