420605 - fragment links have wrong favicon and description in history entry

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Ori Avtalion (salty-horse)]

Using today's nightly build.

When navigating to a page fragment, a new history item is created.
Instead of having the page's favicon, it has the default favicon.
Instead of having the page's title for a description, it has the filename part of the URL.

To reproduce:
1) Visit http://en.wikipedia.org/wiki/Mozilla_Firefox
2) Click on "Release history" in the table of contents.
*) Now at http://en.wikipedia.org/wiki/Mozilla_Firefox#Release_history
3) Open the history sidebar, sort by last visited.
*) The last entry is Mozilla_Firefox, and it has a default favicon, instead of "Mozilla Firefox" and the Wikipedia favicon.

Note that the back/forward history menu displays the correct description for the fragment.

Comment 1

[Ria Klaassen (not reading all bugmail)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre) Gecko/2008030307 Minefield/3.0b4pre

I see this too. I see this at least since end May and branch has no icons at all in history so it is not a regression.

Comment 2

[Ria Klaassen (not reading all bugmail)]

(In reply to comment #1)
> branch has no icons at
> all in history 

I meant only default favicons.

Comment 3

[Lucas Malor (mail: c6kfnkn2uc AT snkmail DOT c0m)]

Currently I see there's two history entries after reproducing steps: one for "Mozilla Firefox - Wikipedia etc" with Wikipedia icon, and one for Mozilla_Firefox#Release_history, named Mozilla_Firefox, without favicon. Also in back/forward menu Mozilla_Firefox#Release_history has no favicon. 

IMO anchor links must not only have favicons, but they should be distinguished adding them anchor link name in history (in this example, Mozilla_Firefox#Release_history instead of Mozilla_Firefox).

Anyway, confirmed.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008052406 Minefield/3.0pre

Comment 4

[John P Baker]

Attachment: Proof of concept

This is a proof of concept for the icons; It basically works by shadowing the fragment entry in places with an entry for the page. This seems to work pretty well although it may still need a visit entry to keep the place entry alive for longer.

The second part is The first part is a proof of concept for a problem that history has in displaying a suitable icon on visiting a fragment of a document - three guesses as to the bug number.

The second part is  just some early ideas for the titles but, as it already seems to work quite well, is included for further experimentation.

I wasn't that keen on just showing #fragment but then it occurred to me that I could attempt to use information from the document, for example:

Bug 420605 – fragment links have wrong favicon and description in history entry [Description]
All-in-One Sidebar :: Firefox Extras [Avaliações]
Web Server Statistics for Central Web Server [Redirection Report]

and just use #fragment as a fallback

Bristol University | A-Z index | Full [#w]

There may be various problems with this - it doesn't seem to handle javascript changes very well - so the option is off by default.

Comment 5

[John P Baker]

(In reply to comment #4)
> The second part is The first part is a proof of concept for a problem that
> history has in displaying a suitable icon on visiting a fragment of a document
> - three guesses as to the bug number.

Sorry, I don't know how that 'paragraph' got back in.

Comment 6

[John P Baker]

Attachment: screenshot with and without extension installed

Comment 7

[Lucas Malor (mail: c6kfnkn2uc AT snkmail DOT c0m)]

This is an interesting workaround, but you think you can implement it in Firefox C++ source code?

Comment 8

[John P Baker]

Attachment: Proof of concept v0.2

(In reply to comment #4)
> There may be various problems with this - it doesn't seem to handle javascript
> changes very well - so the option is off by default.

This turned out to be bug 438288

Comment 9

[John P Baker]

Attachment: WiP

I have been sitting on this for a while so as to see the fate of the HTML5 (IE8) hashchange event (bug 385434); My preferred solution is to try an event listener for this (proposed) event and to set the icon and title in the database to the current ones.

In the meantime the first part of this patch catches the hash change, in the onLocationChange of the progress listener, by comparing any Location containing a '#' with the previous location; The second part is what the event handler would do if it was available.

For nightly testers, I will upload a new extension containing this code (version 0.4) to amo later today.

Comment 10

[John P Baker]

Attachment: patch v1

hashchange has now been implemented, so possibly something like this.

I don't believe that I need the try block anymore but it is useful for testing

Comment 11

[John P Baker]

The extension has been updated (0.5) on amo to reflect this latest change.

[Nit^H^Hote for self: despite other handlers using 'evt' or 'event', change parameter name to 'aEvent']

Comment 12

[John P Baker]

Attachment: testcase

Comment 13

[Dietrich Ayala (:dietrich)]

john, can you ask review from gavin or another browser peer for this?

Comment 14

[John P Baker]

Attachment: failure

Hmm! I don't get a hashchange for this case but I do get a history entry; I am not sure if it is valid, but unraveling the specs is proving a bit hard...

Comment 15

[Justin Lebar (not reading bugmail)]

Attachment: failure, slightly modified

re comment 14: Interesting.  I get neither a hashchange nor a history entry for that testcase.  At least on my browser, it appears that setting location.hash='#' actually sets location.hash to '', as illustrated by the testcase I just uploaded.

I'm running the latest Shiretoko nightly on Linux. Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.1pre) Gecko/20090708 Shiretoko/3.5.1pre

Comment 16

[Justin Lebar (not reading bugmail)]

Oops.  I meant to test this on the latest trunk nightly.  But I get the same results as above: Neither a hashchange nor a history entry for either of the testcases in comment 14 and comment 15.  Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090708 Minefield/3.6a1pre

Comment 17

[Dão Gottwald [:dao]]

This looks like a places bug, not a tabbrowser one. Why don't you fix it in places code?

Comment 18

[John P Baker]

(In reply to comment #17)
> This looks like a places bug, not a tabbrowser one. Why don't you fix it in
> places code?

It seems to me to be a hybrid bug; Storing the favicons in the places database is currently done by tabbrowser so adding the fragment code here seems right; Storing the title is currently done in the docshell - as is firing the hashchange - but I am not sure that it should also store the title for fragments, so I put it here mostly for convenience.

I would be happy if we just do the icon here and punt the title somewhere else. But where?

Comment 19

[Dão Gottwald [:dao]]

Rather than storing any extra data, why don't we use foo's title and favicon for foo#bar?

Comment 20

[John P Baker]

(In reply to comment #18)
> Storing the title is currently done in the docshell - as is firing the
> hashchange - but I am not sure that it should also store the title for
> fragments, so I put it here mostly for convenience.
> 
> I would be happy if we just do the icon here and punt the title somewhere else.
> But where?

Having found the appropriate code for session history, the docShell probably _is_ the
correct place; Presumably something like:

7895     /* Set the title for the SH entry for this target url. so that
7896      * SH menus in go/back/forward buttons won't be empty for this.
7897      */
7898     if (mSessionHistory) {
7899         PRInt32 index = -1;
7900         mSessionHistory->GetIndex(&index);
7901         nsCOMPtr<nsIHistoryEntry> hEntry;
7902         mSessionHistory->GetEntryAtIndex(index, PR_FALSE,
7903                                          getter_AddRefs(hEntry));
7904         NS_ENSURE_TRUE(hEntry, NS_ERROR_FAILURE);
7905         nsCOMPtr<nsISHEntry> shEntry(do_QueryInterface(hEntry));
7906         if (shEntry)
7907             shEntry->SetTitle(mTitle);
7908     }
7909 
+        /* Set the title for the Global History entry for this target url. */
+        if (mGlobalHistory) {
+           mGlobalHistory->SetPageTitle(mCurrentURI, mTitle);
+        }
+
7910     if (doHashchange) {

But I would be out of my depth working here.

Comment 21

[John P Baker]

Attachment: patch v2a (favicon)

(In reply to comment #19)
> Rather than storing any extra data, why don't we use foo's title and favicon
> for foo#bar?

This was the approach in comment #4; The problem is you may never have visited foo (you may have got to foo#bar from foo#ey) which then mean that you have to create an entry for foo which (a) can disappear as there is no visit for it and
(b) creates a lot of database queries to track it.

Comment 22

[Dão Gottwald [:dao]]

(In reply to comment #21)
> This was the approach in comment #4; The problem is you may never have visited
> foo (you may have got to foo#bar from foo#ey)

Well, that doesn't mean that you haven't visited foo before foo#ey. Also, if you visit foo#bar directly without a hash change, we store the favicon and the title. So the only remaining edge case is really visiting foo#ey directly (i.e. not foo) and then foo#bar. That's neither dramatic nor very common, and it doesn't justify the bloat your patch would add, imho.

Comment 23

[John P Baker]

Attachment: patch v3

Move handler to browser.js

The bad news is that is not so easy to find the relevant browser.
The good news is that the code is now a good match for the "icon" code in the DOMLinkAdded handler.

Comment 24

[Dão Gottwald [:dao]]

Let my slightly rephrase an earlier question: Why does Places store random respectively no data for foo#bar, instead of using foo's title and favicon? Wouldn't it be nicer if that worked out of the box?

Comment 25

[John P Baker]

(In reply to comment #20)

> Having found the appropriate code for session history, the docShell probably
> _is_ the correct place; Presumably something like:

...

> 7909 
> +        /* Set the title for the Global History entry for this target url. */
> +        if (mGlobalHistory) {
> +           mGlobalHistory->SetPageTitle(mCurrentURI, mTitle);
> +        }
> +
> 7910     if (doHashchange) {
> 
> But I would be out of my depth working here.

I am afraid that I have punted this to bug 503832 - so it will now need a c++ programmer.

(In reply to comment #24)
> Let my slightly rephrase an earlier question: Why does Places store random
> respectively no data for foo#bar, instead of using foo's title and favicon?
> Wouldn't it be nicer if that worked out of the box?

As used here places is just an implementation of nsIGlobalHistory2 so it is not reasonable for it to do anything but store and retrieve titles as requested by the browser; Note that Seamonkey 1.1.17 also exhibits the same behaviour as this bug - use STR from comment #0 and then Go/History - which convinces me it is a Core bug.

By analogy the icons are stored by the browser (on DOMLinkAdded in browser.js via setIcon in tabbrowser which calls the favicon service) and so the patch does the same on hashchange for the fragment URLs; Note that hashchange is fired just after the session history title is updated and hopefully where the global title will be.

Comment 26

[John P Baker]

Attachment: patch v4

Check aEvent.isTrusted

setAndLoadFaviconForPage checks isFailedIcon so don't do it here.

Comment 27

[Dão Gottwald [:dao]]

(In reply to comment #25)
> As used here places is just an implementation of nsIGlobalHistory2 so it is not
> reasonable for it to do anything but store and retrieve titles as requested by
> the browser

I haven't checked if nsIGlobalHistory2 is explicit about this. Maybe the interface isn't sane, or maybe Places could interpret it more laxy.

> Note that Seamonkey 1.1.17 also exhibits the same behaviour as
> this bug - use STR from comment #0 and then Go/History - which convinces me it
> is a Core bug.

The fact that multiple consumers suffer from this actually seems to support that either the interface or the implementation should be fixed.

Btw, without being convinced that you're using the right approach, note that this structure is unnecessarily complicated:

const hashChangeHandler = {
  handleEvent: function (aEvent) {
    switch (aEvent.type) {
      case "hashchange":
        this.onHashChange(aEvent);
        break;
    }
  },
  onHashChange: function(aEvent) {
    FOO
  }
};

Use this instead:

function hashChangeHandler(aEvent) {
  FOO
}

Comment 28

[Dão Gottwald [:dao]]

(In reply to comment #27)
> I haven't checked if nsIGlobalHistory2 is explicit about this. Maybe the
> interface isn't sane, or maybe Places could interpret it more laxy.

s/laxy/laxly/

Comment 29

[Dão Gottwald [:dao]]

Comment on attachment 389111 [details] [diff] [review]
patch v4

>+    var browser = gBrowser.getBrowserForDocument(aEvent.target.document);
>+    if (!browser)
>+      return

How about:

if (aEvent.target != content)
  return;

var browser = gBrowser.selectedBrowser;

Comment 30

[John P Baker]

Attachment: patch v5

(In reply to comment #27)
> Btw, without being convinced that you're using the right approach, note that
> this structure is unnecessarily complicated:

Yes I think I must have been using 'this' at some stage which made it convenient.

(In reply to comment #29)

> How about:
> 
> if (aEvent.target != content)
>   return;
> 
> var browser = gBrowser.selectedBrowser;

I'm pretty sure that the event could fire on any browser so need to identify which one it was.

Comment 31

[Dão Gottwald [:dao]]

It could, but even if you care about that case, you should still check the 'content' shortcut.

Comment 32

[John P Baker]

Attachment: patch v5.1

Comment 33

[John P Baker]

Attachment: patch v5.2

Comment 34

[Gervase Markham [:gerv]]

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

Comment 35

[Philipp von Weitershausen [:philikon]]

Attachment: Test case and bugfix based on John's v5.2 patch

Apart from ruining the aesthetics of the affected history entries, this bug also causes problems for add-ons that query history and rely on description + favicon being correct (e.g. BarTab). So it'd be quite useful if we could decide whether John's patch was the correct way to proceed, especially with the docshell part (bug 503832) being fixed now (or soon to be fixed again ;)).

I have attached a patch against mozilla-central containing John's bugfix and a test case exercising the fix. Dietrich suggested Gavin as a reviewer in comment 13, so I'm submitting it to him. Cheers!

Comment 36

[Philipp von Weitershausen [:philikon]]

Attachment: Test case and bugfix, updated for bug 544097

Here's an updated version of the patch containing John's bugfix and my test case. The only difference between this and the previous version of the patch concerns bug 544097. Since that landed, tests are no longer supposed to use localhost:8888 but mochi.test:8888.

Would be cool if we can reach a verdict whether this bugfix can go in or not. I certainly wouldn't mind seeing this (and bug 503832) fixed for the Firefox 3.6.x line. ;)

Comment 37

[:Gavin Sharp [email: [e-mail]]]

It seems wasteful to have favicon entries for every fragment navigated to, and it kind of sucks to have to add this kind of code at this level (in the app vs. in the core).

Couldn't we instead change the favicon service to ignore the fragment when responding to queries, and just return the base page's favicon? That means we lose support for AJAXy pages that dynamically change their favicon based on the fragment, but I'm not sure that's really worth supporting, and doing the simpler thing now doesn't really preclude us from doing more later AFAICT.

Comment 38

[Shawn Wilsher :sdwilsh]

(In reply to comment #37)
> Couldn't we instead change the favicon service to ignore the fragment when
> responding to queries, and just return the base page's favicon? That means we
> lose support for AJAXy pages that dynamically change their favicon based on the
> fragment, but I'm not sure that's really worth supporting, and doing the
> simpler thing now doesn't really preclude us from doing more later AFAICT.
This shouldn't be too hard to do, and I agree that it is the better approach.

Comment 39

[Philipp von Weitershausen [:philikon]]

I'd be fine with that, and I certainly agree with the sentiment of rather not having this code in the app. Note that changing the favicon in JavaScript after having changed window.location.hash will actually store the right favicon right now (tested on FF 3.6) since that explicitly triggers a store in the favicon service. So as Gavin said, that "feature" would be lost. I personally don't feel strongly about that, so I'm ok with ditching it in the process.

Gavin's suggestion would basically boil down to the favicon service normalising URIs to the base URI before both storing and querying. The normalisation when storing is necessary because one might visit http://host/page#fragment and move on to other fragments before ever visiting http://host/page. I'll work on a patch for this.

Comment 40

[John P Baker]

(In reply to comment #37)
> It seems wasteful to have favicon entries for every fragment navigated to, and
> it kind of sucks to have to add this kind of code at this level (in the app vs.
> in the core).

There is already a history entry and the favicon is probably already stored so there
isn't actually much extra stored - a favicon_id instead of a null.

> Couldn't we instead change the favicon service to ignore the fragment when
> responding to queries, and just return the base page's favicon? That means we
> lose support for AJAXy pages that dynamically change their favicon based on the
> fragment, but I'm not sure that's really worth supporting, and doing the
> simpler thing now doesn't really preclude us from doing more later AFAICT.

Some of the earlier experiments (see proof of concept attachments) on this bug tried that approach; The trouble is that there may not be an entry for the base page so that, on visit to http://host/page#fragment, they had to create an entry for http://host/page  which would be lost on browser shutdown as there was no visit. [This can also then delete the favicon data from the database as there may now be no reference to it]

I also suspect that the queries for history that include the favicon will become very hairy such that the right decision becomes to store the favicon relation when you know what it is - rather than trying to figure it out later.

Comment 41

[dziastinux]

Here is another test case which fails in all Firefox versions (including 4.0):
http://jsbin.com/ulaxo4

Comment 42

[Justin Lebar (not reading bugmail)]

Now that we've fixed bug 655270, we should be able to use the same approach to fixing this bug: On a hash load, call into the favicon service and tell it that the new favicon is the same as the old page's favicon.

John, do you mind if I pick up this bug?

Comment 43

[Justin Lebar (not reading bugmail)]

(In reply to comment #37)
> It seems wasteful to have favicon entries for every fragment navigated to,

Maybe, but in bug 655270, we made it so each pushState causes a new favicon entry.  I don't think this is a lot different.

> and it kind of sucks to have to add this kind of code at this level (in the
> app vs. in the core).

I can move it into docshell if you'd prefer.

Comment 44

[:Gavin Sharp [email: [e-mail]]]

Yes, having this code in docshell would be better.

Comment 45

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1 - Send notification from docshell

Comment 46

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 534310 [details] [diff] [review]
Patch v1 - Send notification from docshell

>diff --git a/docshell/test/browser/browser_bug420605.js b/docshell/test/browser/browser_bug420605.js

>+    gBrowser.selectedBrowser.addEventListener(
>+        "DOMContentLoaded", onPageLoad, true);

Probably a good idea to remove this listener at some point (even though you're removing the tab it's added on).

Looks great to me, but I'm not a docshell peer.

Comment 47

[Justin Lebar (not reading bugmail)]

Comment on attachment 534310 [details] [diff] [review]
Patch v1 - Send notification from docshell

Jonas, do you want to review the docshell bits?

Comment 48

[Justin Lebar (not reading bugmail)]

http://hg.mozilla.org/mozilla-central/rev/4ae5181b22f7

Comment 49

[Philipp von Weitershausen [:philikon]]

(In reply to comment #48)
> http://hg.mozilla.org/mozilla-central/rev/4ae5181b22f7

Thanks, Justin, for finishing this up!