Closed Bug 874407 Opened 6 years ago Closed 6 years ago

new visits are inserted incorrectly in the Library and the sidebar treeviews

Categories

(Toolkit :: Places, defect)

21 Branch
x86
All
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla24
Tracking Status
firefox21 --- wontfix
firefox22 + verified
firefox23 + fixed
firefox24 + fixed
firefox-esr17 --- unaffected

People

(Reporter: meebmoob, Assigned: mak)

References

(Depends on 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:21.0) Gecko/20100101 Firefox/21.0 (Beta/Release)
Build ID: 20130511120803

Steps to reproduce:

With either the history sidebar opened and set to display last visited, or the history window open and showing today's history, visited a website.


Actual results:

The most recent history entry was overwritten by the currently visited page - the correct history list only appears after closing and reopening the sidebar or history window.


Expected results:

The most recent history entry should be inserted at the top of the list with previous entries displayed beneath it.
Component: Untriaged → Bookmarks & History
I can reproduce the problem.

STR:
1. Open history sidebar last visited view / Library history right pane
URL1
URL2
URL3

2. Click to open URL3

Actual Results:
URL3
URL2
URL3

URL1 was overwritten


Expected results:
URL3
URL1
URL2


Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/325397090d12
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130206 Firefox/21.0 ID:20130206135631
Bad:
http://hg.mozilla.org/mozilla-central/rev/4a6b8dd4dfe3
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130206 Firefox/21.0 ID:20130206180753
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=325397090d12&tochange=4a6b8dd4dfe3


Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/03232247aef8
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130206 Firefox/21.0 ID:20130206113128
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/2308bb67a6fa
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130206 Firefox/21.0 ID:20130206122830
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=03232247aef8&tochange=2308bb67a6fa


Regressed by:
 6b183775ad02	Marco Bonardo — Bug 831094 - Avoid replacing nodes in Places query results. r=Mano sr=gavin
Blocks: 831094
Status: UNCONFIRMED → NEW
Component: Bookmarks & History → Places
Ever confirmed: true
Keywords: regression
Product: Firefox → Toolkit
OS: Windows XP → All
Summary: new history displays incorrectly in existing window/sidebar → new history displays incorrectly in Library window/sidebar
:mak,
can you consider backing Bug831094 out from Firefox22beta/Aurora23.0a2?
Flags: needinfo?(mak77)
(In reply to Alice0775 White from comment #2)
> :mak,
> can you consider backing Bug831094 out from Firefox22beta/Aurora23.0a2?

no, that would break far more than what we may fix. We build on top of that and that is also a very convenient performance win.
Why the question?
Flags: needinfo?(mak77)
Marco if you can provide a low risk forward fix for this in the next week or two we should be able to uplift to Beta and ship without this regression in our next cycle.
Assignee: nobody → mak77
The problem is in the view, I wrote a test for the backend and it passed properly, everything looks good debugging there. Instead the treeview gets the wrong index of the node to be moved from _getRowForNode. Still investigating the reason and will write a chrome test for it.
And that happens cause the backend moves the node in the result before notifying the frontend, the frontend then asks the backend for the index of the child to be moved, but it gets the new position, not the old one.
Summary: new history displays incorrectly in Library window/sidebar → new visits are inserted incorrectly in the Library and the sidebar treeviews
Attached patch patch v1.0Splinter Review
The fix is be safe enough for backporting.

I modified an existing test but didn't rename it cause there are some bugs filed about random failures and I want to keep those valid for now.
Attachment #756936 - Flags: review?(mano)
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c37f4a3e488
Flags: in-testsuite+
Target Milestone: --- → mozilla24
linux only failure...
and the failure happens on the sanity check, doesn't make much sense :(
Dumb mistake from my site, I should not have used post increment, pointlessy error-prone, as demonstrated. Due to that, 2 visits were being added with the same time, the natural ordering on linux differs and causes the failure. I'm just going to repush, but using pre increment in the test.
https://hg.mozilla.org/mozilla-central/rev/d8cfe163b77d
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Comment on attachment 756936 [details] [diff] [review]
patch v1.0

This fix is quite safe and comes with a test, I think it's worth an uplift.
[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 831094
User impact if declined: history treeviews (Library and sidebar) are wrongly updated and may end up in misbehaviors of commands on zombie rows. Plus the most recent visit data is not reported.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): the patch is simple and contained, similar code is used in other known-to-be-good parts. Test included.
String or IDL/UUID changes made by this patch: none
Attachment #756936 - Flags: approval-mozilla-beta?
Attachment #756936 - Flags: approval-mozilla-aurora?
Comment on attachment 756936 [details] [diff] [review]
patch v1.0

Low risk, fixes regression, tested - approving for uplift.
Attachment #756936 - Flags: approval-mozilla-beta?
Attachment #756936 - Flags: approval-mozilla-beta+
Attachment #756936 - Flags: approval-mozilla-aurora?
Attachment #756936 - Flags: approval-mozilla-aurora+
Verified fixed using the STR from comment 1, with Firefox 22 beta 4 (build ID: 20130605070403) on both Ubuntu 12.10 32bit and Mac OSX 10.8.3 in 32bit mode.
QA Contact: manuela.muntean
Depends on: 892485
I've tried again with the current release Firefox 25.0 and Firefox 25.0b12 (Mozilla/5.0 (X11; Linux x86_64; rv:25.0) Gecko/20100101 Firefox/25.0) :I'm still seeing the same issue reported here https://bugzilla.mozilla.org/show_bug.cgi?id=900525,links in the history sidebar cannot be clicked on in sequence,as new history entries are added the highlighted item moves erratically up and down the list.
Since I'm  still seeing this bug in the latest betas,do I have to file a new bug?
(In reply to msth67 from comment #19)
> I've tried again with the current release Firefox 25.0 and Firefox 25.0b12
> (Mozilla/5.0 (X11; Linux x86_64; rv:25.0) Gecko/20100101 Firefox/25.0) :I'm
> still seeing the same issue reported here
> https://bugzilla.mozilla.org/show_bug.cgi?id=900525,links in the history
> sidebar cannot be clicked on in sequence,as new history entries are added
> the highlighted item moves erratically up and down the list.

On Ubuntu 13.10 64-bit, I can see the same behavior with both Firefox 20 and Firefox 25: the highlighted link jumps erratically up and down the history list in the sidebar,and new links are added between the  one  just clicked and the next highlighted link (as mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=900525#c0)
(In reply to msth67 from comment #20)
> Since I'm  still seeing this bug in the latest betas,do I have to file a new
> bug?

I think the issue you are reporting is not this bug, this bug was replacing in-place a page with another one, causing a dataloss, while your issue is a page jumping in the list. I think there are other reports of this issue, but it's not this bug.
Yes,I think you're actually right:for some reason I assumed that the issue I've originally reported here https://bugzilla.mozilla.org/show_bug.cgi?id=900525 (and which,BTW,is still unsolved to date in every beta/nightly build I've tried so far) was somehow related to this one.
You need to log in before you can comment on or make changes to this bug.