Closed Bug 428690 Opened 12 years ago Closed 11 years ago

Two entries added to history menu when visiting a page that redirects

Categories

(Firefox :: Bookmarks & History, defect, P2)

3.0 Branch
defect

Tracking

()

RESOLVED FIXED
Firefox 3.6a1

People

(Reporter: projects, Assigned: mak)

References

Details

(Keywords: dev-doc-complete, polish)

Attachments

(1 file, 5 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.8.1.13) Gecko/20080311 Firefox/2.0.0.13
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008041206 Minefield/3.0pre

When you visit pages or bookmarks whose URL redirects to another page, two entries are added to the "History" menu. One entry displays the title of the page that is eventually loaded (as expected), but the other entry shows the path or filename of the URL that was originally called.

This also occurs when clicking items from the "Latest Headlines" live bookmark (http://news.bbc.co.uk/go/rss/-/2/hi/default.stm) or RSS feeds served by FeedBurner (e.g. http://feeds.feedburner.com/BurnThisRSS2). In both cases, the feed items are URL redirects.

Reproducible: Always

Steps to Reproduce:
1. Click the "Getting Started" bookmark on the bookmarks toolbar (or visit http://www.mozilla.com/products/firefox/central.html)
2. Wait for the page to finish loading
Actual Results:  
Note two new items atop the "History" menu
  central.html
  Firefox Central | Mozilla

Expected Results:  
Only one new item should be in the "History" menu
  Firefox Central | Mozilla

This also occurs in Firefox 2 when manually typing in addresses that redirect. However, if you click a link or bookmark in Firefox 2 that redirects, only one entry is added to the "History" menu, which is the expected behavior.
Component: History → Bookmarks & History
QA Contact: history → bookmarks
Duplicate of this bug: 448462
Duplicate of this bug: 450353
Status: UNCONFIRMED → NEW
Component: Bookmarks & History → Places
Ever confirmed: true
OS: Windows Vista → All
QA Contact: bookmarks → places
Hardware: PC → All
Version: unspecified → 3.0 Branch
Priority: -- → P2
Target Milestone: --- → Firefox 3.1
Blocks: 450352
I'm going implementing includeRedirects option
Assignee: nobody → mak77
Status: NEW → ASSIGNED
a problem:

user TYPES in url A that redirects to B (ideally we could have a bunch of redirects in the middle), what user see for real in the browser is TitleB and UriB, so imho we should try to hold the final target page in our views by default, that's because in-the-middle redirects could have wrong titles, and both uri and title are not what the user see in the browser.
Suppose that we have a query with includeRedirects = 0 and onlyTyped = 1, the original uri is typed but we redirect from it, so first option will discard A. The targer uri B will instead be discarded by onlyTyped option since it has not been typed. So we end up not showing both A and B.

possible solutions:
1. when a typed page redirects mark redirected to as typed. This is easy but database is not consistent, user has not typed in B really.

2. if a redirected-from page has empty title set it to target one. This does not fix uri, uriA is not what the user see when he visit the page, the same for favicon and we could still have wrong titles like "redirecting...".

3. actually we use typed as a bool, we could change it to an int, typed = 1 means a page has been typed in, typed = 2 means a page is a redirect from a typed in url. So when both options are excluding each other we could use WHERE typed IN (1,2) and we have consistent data in the db. The problem is that, since includeRedirects = 0 would be the default, doing onlyTyped = 1 will get an uri we have not typed at all.

4. ...
4. set typed = 2 for redirects from a typed in page, includeRedirects = 0 will discard all redirects maintaining the first typed page, and that will be the default behaviour, so no headache. We could instead introduce a new option resolveRedirects = 1 that will invert the behaviour maintaining the target redirects for UI purpose (in ui elements like most visited or history menu we should show uris/titles that the user has already seen in the main ui).

resolveRedirects = 0 will retain typed = 1 pages, with wrong titles
resolveRedirects = 1 will retain typed = 1,2 pages, with correct titles (but onlyTyped will also report uris redirects from a typed one)
Attached patch wip 0.1 (obsolete) — Splinter Review
first take, support includeRedirects and resolveRedirects. for now it works only for history queries, and still don't have support for typed only pages.
The test unit executes a matrix of tests mixing up options and looking if we get consistent transition types and number of results.
Next step is adding a onlyTyped option.
Attached patch wip 0.2 (obsolete) — Splinter Review
added support for onlyTyped and managed resolveRedirects in this case.
again this is only supported by history queries for now.

I must tell this makes the history menu and Most Visited much cleaner
Attachment #338859 - Attachment is obsolete: true
Blocks: 420446
applied to Most Visited (attachment in bug 420446): https://bugzilla.mozilla.org/attachment.cgi?id=339247
No longer blocks: 420446
Seeking blocking status for Firefox 3.1.

This bug negatively impacts the "Most Visited" feature.  Other people have shared their frustrations with me that they see duplicate entries in the menu.  This problem is fairly common because many people type "cnn.com" in the URL bar which redirects to "www.cnn.com".
Flags: blocking-firefox3.1?
instead of using typed = 2 we could define the old referrer column in moz_historyvisits, that would allow to check back if we come from typed without recursion (would be slower though)
Whiteboard: [needs unbitrot & bookmarks queries support]
Blocks: 420446
Flags: wanted-firefox3.1+
Flags: blocking-firefox3.1?
Flags: blocking-firefox3.1-
Keywords: polish
Duplicate of this bug: 421852
really would like to see this for 3.1. what's left to figure out here?
i would say the remaining part is "convert it to the temp table era", and discard onlyTyped changes since i think we should rethink those better (option to query based on transition type and a better history table).
I'll try to unbitrot this in the next days.
Attached patch wip 0.3 (obsolete) — Splinter Review
moving to a single redirectsMode option (and leaving out onlyTyped for now, since i really prefer having a way to query the transition type, that we can eventually optimize later when building the queries).

I'm still not sure if we should change the default behaviour from the actual one where we show everything... hwv finally it should be configurable...
also for 3.2 would be better moving placesQueryBuilder to its own file and clean it up somehow if possible, will most likely file a followup for it.
Attachment #339246 - Attachment is obsolete: true
per irc the options should be:
REDIRECTS_MODE_SOURCE: show only sources
REDIRECTS_MODE_TARGET: show only targets
REDIRECTS_MODE_ALL: show both (default, like it is now, let's see in future)
Attached patch wip 0.4 (obsolete) — Splinter Review
all of the above, needs a deeper test since the actual one is only counting results.
Fixed smart query and updated smart bookmarks version (this is the first time we do), added the new option to the new serialization test, updated constants and new default.
Attachment #358079 - Attachment is obsolete: true
Whiteboard: [needs unbitrot & bookmarks queries support] → [needs unit tests]
Attached patch patch v1.0 (obsolete) — Splinter Review
All of the above, and a dedicated unit test!
Attachment #358184 - Attachment is obsolete: true
Attachment #360400 - Flags: review?(dietrich)
Whiteboard: [needs unit tests] → [needs review dietrich]
Keywords: dev-doc-needed
Flags: in-testsuite?
Target Milestone: Firefox 3.1 → ---
Blocks: 407285
Comment on attachment 360400 [details] [diff] [review]
patch v1.0


>+  },
>+  // redirectsMode
>+  {
>+    property: "redirectsMode",
>+    desc:     "nsINavHistoryQueryOptions.redirectsMode",
>+    matches:  simplePropertyMatches,
>+    runs:     [
>+      function (aQuery, aQueryOptions) {
>+        aQueryOptions.redirectsMode = aQueryOptions.REDIRECTS_MODE_ALL;
>+      },
>+      function (aQuery, aQueryOptions) {
>+        aQueryOptions.redirectsMode = aQueryOptions.REDIRECTS_MODE_TARGET;
>+      }
>+    ]
>+  },
> ];

why only 2 of the 3 states?

> 
> ///////////////////////////////////////////////////////////////////////////////
> 
> /**
>  * Enumerates all the sequences of the cartesian product of the arrays contained
>  * in aSequences.  Examples:
>  *
>diff --git a/toolkit/components/places/tests/queries/test_redirectsMode.js b/toolkit/components/places/tests/queries/test_redirectsMode.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/places/tests/queries/test_redirectsMode.js
>@@ -0,0 +1,268 @@
>+/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim:set ts=2 sw=2 sts=2 et: */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Places Test Code.
>+ *
>+ * The Initial Developer of the Original Code is Mozilla Corporation
>+ * Portions created by the Initial Developer are Copyright (C) 2009
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Marco Bonardo <mak77@bonardo.net> (Original Author)
>+ *
>+ * Alternatively, the contents of this file may be used under the terms of
>+ * either the GNU General Public License Version 2 or later (the "GPL"), or
>+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
>+ * in which case the provisions of the GPL or the LGPL are applicable instead
>+ * of those above. If you wish to allow use of your version of this file only
>+ * under the terms of either the GPL or the LGPL, and not to allow others to
>+ * use your version of this file under the terms of the MPL, indicate your
>+ * decision by deleting the provisions above and replace them with the notice
>+ * and other provisions required by the GPL or the LGPL. If you do not delete
>+ * the provisions above, a recipient may use your version of this file under
>+ * the terms of any one of the MPL, the GPL or the LGPL.
>+ *
>+ * ***** END LICENSE BLOCK ***** */
>+
>+// Array of visits we will add to the database, will be populated later
>+// in the test.
>+let visits = [];
>+
>+/**
>+ * Gets a sequence of query options, and compare query results obtained throgh
>+ * them with a custom filtered array of visits, based on the values we are
>+ * expecting from the query.

s/Gets/Takes/
s/throgh/through/

r=me otherwise
Attachment #360400 - Flags: review?(dietrich) → review+
Whiteboard: [needs review dietrich]
Attached patch patch v1.1Splinter Review
addressed comments, fixed a minor problem in head_queries so all unit tests run correctly. This should be ready to land.
Attachment #360400 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/e743e00363e3
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.2a1
Flags: in-testsuite? → in-testsuite+
Depends on: 484263
Comment on attachment 367600 [details] [diff] [review]
patch v1.1

asking approval allows us to have a better history menu and a better "most visited". medium risk, has tests. the only regression found has a patch and tests.
Attachment #367600 - Flags: approval1.9.1?
Whiteboard: [also requires approval to bug 484263]
Is there any reason this was limited to history and not used for the awesome bar results too? I can easily get multiple entries from redirects in the awesome bar results.
because those are completely different code paths, iirc there's already a bug filed for the location bar component.
I apparently can't take this without bug 484263, which is not nominated for approval - any reason for that? Also, what's the compatibility-hit here and in that other bug for add-ons and such?
yes bug 484263 is needed to avoid recreating smart bookmarks when they are upgraded if the user has deleted them(this patch upgrades smart bookmarks version), there's no reason for it to not be approved with this (i'll nominate it), bug 484263 has no impact on add-ons.

Impact of this bug on add-ons is only the uuid change (so hitting binary components, not standard add-ons). This adds a new option, but has been built to not change old results.
(In reply to comment #25)
> Impact of this bug on add-ons is only the uuid change (so hitting binary
> components, not standard add-ons). This adds a new option, but has been built
> to not change old results.
It's not just a uuid change, it also adds an attribute.  If you want this on branch, you should do what I did in bug 468305.
...(which I'm not sure if this is worth doing that on branch)
adding a special interface just for removing a couple entries from a menu is not worth doing unless bug is a blocker.

Shawn: what's the impact of a new attribute on old implementers who does not even know about it and that does not change the behavior of old queries (the default value is the old behavior)?
I think the evaluation point is more around the uuid change, i don't know the number of binary components who implement a place query, but i suppose is quite low (i don't have access to addons mxr to look for the uuid)
(In reply to comment #28)
> adding a special interface just for removing a couple entries from a menu is
> not worth doing unless bug is a blocker.
hence comment 27

> Shawn: what's the impact of a new attribute on old implementers who does not
> even know about it and that does not change the behavior of old queries (the
> default value is the old behavior)?
> I think the evaluation point is more around the uuid change, i don't know the
> number of binary components who implement a place query, but i suppose is quite
> low (i don't have access to addons mxr to look for the uuid)
It changes the vtable, so they'll be totally broken (binary components only).
Comment on attachment 367600 [details] [diff] [review]
patch v1.1

Clearing approval, looks like this has made history menu query slower. So i prefer not taking this till we have a better query.
Attachment #367600 - Flags: approval1.9.1?
Depends on: 487777
Whiteboard: [also requires approval to bug 484263]
we have better query now, but this can't still make 3.5 due to idl changes, btw any perf issue on trunk is solved.
Duplicate of this bug: 495698
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
If Firefox wrongly marks something as a redirect, how do you get it to recognise it as a normal page again?
(In reply to comment #35)
> If Firefox wrongly marks something as a redirect, how do you get it to
> recognise it as a normal page again?

Firefox does not wrongly marks something as a redirect. if it does it's a bug in channel implementation, the bug should have clear steps to reproduce and the link to the address that is wrongly interpreted.
Firefox has done so for me, I stayed at a hotel and it made you log in everytime you wanted to go online thus turning whatever page I went to into a redirect up until the point I logged in. Bug reported here: https://bugzilla.mozilla.org/show_bug.cgi?id=550750

But the original question stands, _IF_ Firefox incorrectly thinks something is a redirect, what do I need to do in order to undo it? There seems to be something in my places database that has been marked in which I seemingly have no access to. I'd really just like my Most Visited to work properly again.
(In reply to comment #37)
> Firefox has done so for me, I stayed at a hotel and it made you log in
> everytime you wanted to go online thus turning whatever page I went to into a
> redirect up until the point I logged in. Bug reported here:
> https://bugzilla.mozilla.org/show_bug.cgi?id=550750

ok, this is an edge-case, that unfortunatly we don't handle. Firefox is not uncorrect here, since what you see IS a redirect, and there is no easy way to workaround it since we can't know if you're in an hotel or at home or what's the way you access the web in particular moments of the day. But i suppose you don't live in an hotel for all of your life, so the problem should be limited.
That said i'll confirm the bug you pointed at, the the discussion should proceed there. thanks.
You need to log in before you can comment on or make changes to this bug.