Closed
Bug 389782
Opened 17 years ago
Closed 17 years ago
history sidebar doesn't show the same results as fx 2, duplicate uris when grouped by site, last visited most visited
Categories
(Firefox :: Bookmarks & History, defect)
Firefox
Bookmarks & History
Tracking
()
RESOLVED
FIXED
Firefox 3 alpha8
People
(Reporter: moco, Assigned: moco)
References
Details
Attachments
(1 file, 5 obsolete files)
8.11 KB,
patch
|
moco
:
review+
|
Details | Diff | Splinter Review |
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?
Assignee | ||
Updated•17 years ago
|
Assignee | ||
Comment 1•17 years ago
|
||
Attachment #274111 -
Flags: review?(dietrich)
Assignee | ||
Comment 2•17 years ago
|
||
> 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).
Assignee | ||
Comment 3•17 years ago
|
||
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")
Assignee | ||
Comment 4•17 years ago
|
||
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)
Assignee | ||
Comment 5•17 years ago
|
||
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]
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]
Assignee | ||
Comment 6•17 years ago
|
||
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
Updated•17 years ago
|
Flags: blocking-firefox3? → blocking-firefox3+
Assignee | ||
Comment 8•17 years ago
|
||
Attachment #274111 -
Attachment is obsolete: true
Attachment #274699 -
Flags: review?(dietrich)
Assignee | ||
Comment 9•17 years ago
|
||
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.
Assignee | ||
Comment 10•17 years ago
|
||
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)
Comment 11•17 years ago
|
||
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
Assignee | ||
Comment 12•17 years ago
|
||
Attachment #274877 -
Attachment is obsolete: true
Attachment #275074 -
Flags: review?(dietrich)
Attachment #274877 -
Flags: review?(dietrich)
Assignee | ||
Updated•17 years ago
|
Attachment #275074 -
Attachment is obsolete: true
Attachment #275074 -
Flags: review?(dietrich)
Assignee | ||
Comment 13•17 years ago
|
||
Attachment #275075 -
Flags: review?(dietrich)
Updated•17 years ago
|
Attachment #275075 -
Flags: review?(dietrich) → review+
Assignee | ||
Comment 14•17 years ago
|
||
update to the trunk
Attachment #275075 -
Attachment is obsolete: true
Attachment #275081 -
Flags: review+
Assignee | ||
Comment 15•17 years ago
|
||
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
Closed: 17 years ago
Resolution: --- → FIXED
Comment 16•17 years ago
|
||
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•15 years ago
|
||
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.
Description
•