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

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Alan York]

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.

Comment 1

[Alice0775 White]

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

Comment 2

[Alice0775 White]

:mak,
can you consider backing Bug831094 out from Firefox22beta/Aurora23.0a2?

Comment 3

[Marco Bonardo [:mak]]

(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?

Comment 4

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 5

[Marco Bonardo [:mak]]

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.

Comment 6

[Marco Bonardo [:mak]]

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.

Comment 7

[Marco Bonardo [:mak]]

Attachment: patch v1.0

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.

Comment 8

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c37f4a3e488

Comment 9

[Ed Morley [:emorley]]

Backed out for failures in test_bug549192.xul:
https://tbpl.mozilla.org/php/getParsedLog.php?id=23711996&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/6fc06b7fa44d

Comment 10

[Marco Bonardo [:mak]]

linux only failure...

Comment 11

[Marco Bonardo [:mak]]

and the failure happens on the sanity check, doesn't make much sense :(

Comment 12

[Marco Bonardo [:mak]]

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.

Comment 13

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d8cfe163b77d

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d8cfe163b77d

Comment 15

[Marco Bonardo [:mak]]

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

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 756936 [details] [diff] [review]
patch v1.0

Low risk, fixes regression, tested - approving for uplift.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3d63ffc038cf
https://hg.mozilla.org/releases/mozilla-beta/rev/13d9cc0d5428

Comment 18

[Manuela Muntean [Away]]

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.

Comment 19

[msth67]

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.

Comment 20

[msth67]

Since I'm  still seeing this bug in the latest betas,do I have to file a new bug?

Comment 21

[Manuela Muntean [Away]]

(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)

Comment 22

[Marco Bonardo [:mak]]

(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.

Comment 23

[msth67]

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.