639105 - A new entry does not appear in History menu for certain pages

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Alice0775 White]

Build Identifier:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b13pre) Gecko/20110304 Firefox/4.0b13pre ID:20110304030406

History entry does not show in History menu in MenuBar/FirefoxButton.

However, The entry appears in the Library/Sidebar

It works on Firefox3.5.x.
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.18pre) Gecko/20110304 Firefox/3.5.18pre ID:20110304030206

But not work on Firefox3.6.x and Minefield4.0b13pre.
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.16pre) Gecko/20110304 Namoroka/3.6.16pre ID:20110304033304
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b13pre) Gecko/20110304 Firefox/4.0b13pre ID:20110304030406

Reproducible: Always

Steps to Reproduce:
1. Start Minefield with new profile
2. Open URL ( http://www.dofus.com/en/mmorpg-game/objects/2-items/1-amulet )
3. Alt > History or Click Firefox Button > History>

Actual Results:
 History entry does not show in History menu

Expected Results:
 History entry should show in History menu

Comment 1

[Alice0775 White]

Regression window:
Works:
http://hg.mozilla.org/mozilla-central/rev/b618ca270e9b
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090418 Namoroka/3.6a1pre ID:20090418030842
Fails:
http://hg.mozilla.org/mozilla-central/rev/907f2f562f44
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090419 Namoroka/3.6a1pre ID:20090419030935
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b618ca270e9b&tochange=907f2f562f44

In local build,
build from 55cb6c5991d1 : fail
build from c43a4464f76c : work
Regressed by:
55cb6c5991d1	Marco Bonardo — Bug 487777 - History menu is slow and takes ages to open, trunk version, r=dietrich

Comment 2

[Marco Bonardo [:mak]]

This page does a particular redirect like A -> B -> A that confuses the redirects code.

Comment 3

[Alice0775 White]

Other case,
An extra history entry registered in sidebar/Library, but not history menu.

[STR]
1. Open http://t.co/cFm7yp4

[Actual]
I can see history entries as follows.

Sidebar/Library:
http://d.hatena.ne.jp/vividcode/20110425/1303740589
http://t.co/cFm7yp4  <-- extra?

History menu:
http://d.hatena.ne.jp/vividcode/20110425/1303740589

[Expected]
It must be the same,

Comment 4

[Marco Bonardo [:mak]]

Well, they should not always be the same, the history menu is a filtered view of the history sidebar, where only redirect targets are shown. If you think that entry in the sidebar is spurious (or completely useless), please file a bug apart.

The bug here is that the entry should NOT be filtered because it's a redirect target, and the history menu is missing functionality due to this.

Comment 5

[Marco Bonardo [:mak]]

Attachment: patch v1.0

I suspect reviewing changes to this test is as painful as making them.
I added some comments, feel free to ask for more :)

Comment 6

[Dietrich Ayala (:dietrich)]

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

> 
>   if (mRedirectsMode == nsINavHistoryQueryOptions::REDIRECTS_MODE_SOURCE) {
>+    // Should exist at least one visit that is not a redirect target.

"At least one visit should exist that is not a redirect target."

or

"At least one visit that is not a redirect target should exist."

ditto for the other added comments like this.


r=me on the changes. my only concern is that we don't have performance testing that covers changes like this. can you do some query plan analysis, and testing on large history to get a sense of the perf risk with this?

Comment 7

[Marco Bonardo [:mak]]

Sure, will do some check, I think there is no interesting move here, since it's mostly a move from a NOT EXISTS to a EXISTS. But will test on my db and max db.
On the other side, having a faster query generally has low value if it's selecting wrong results.

Comment 8

[Marco Bonardo [:mak]]

The query plan is the same, the timing seems slightly better on new queries. the only drawback seems to be the redirect_source case where the full explain is slightly longer, but the timing doesn't change.

Comment 9

[Marco Bonardo [:mak]]

http://hg.mozilla.org/projects/places/rev/3e5d2b7831d0

Comment 10

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/3e5d2b7831d0

Comment 11

[Simona Badau, Desktop QA]

Verified on:
Mozilla/5.0 (X11; Linux i686; rv:6.0a1) Gecko/20110513 Firefox/6.0a1