360511 - [FIX]Going back to page with URL hash (#foo) doesn't show hash part

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect, P1)

Description

[James Ross]

It's unclear where the bug is here, but I can't find an existing one on the problem, and it appears to be at a lower level than the URLBar, so I'm going to go for docshell for now.

Steps:
1. Load a page that has hash links, like the one in the URL, or a Bugzilla bug.
2. Click a hash link (e.g. "link here" on my page).
3. Click a normal link ("CVS log", Bugzilla attachment, etc.).
4. Click back once.

Actual results:
URLBar (and document.location) show the URL *without* the hash part.

Expeted results:
URLBar (and document.location) show the URL *with* the hash part.

Note that clicking back a 2nd time will actually reload the page (seems wrong), and show the URL without the hash (correct).

This occurs for me with Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9a1) Gecko/20061110 Firefox/3.0a1 ID:2006111004, and Hannibal on IRC confirmed it does not happen in his Firefox 2.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Looks like this regressed between 2005-08-02-05 and 2005-08-03-08 trunk Seamonkey builds and between .  Checkins:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2005-08-02+05&maxdate=2005-08-03+08&cvsroot=%2Fcvsroot

I would guess a regression from bug 301516, especially given that disabling bfcache fixes the problem.  In fact, I bet the change from doing 

-  SetCurrentURI(uri);

 to doing 

+        SetCurrentURI(document->GetDocumentURI(),
+                      document->GetChannel(), PR_TRUE);

is the problem -- |uri| came from session history, of course:

-    aSHEntry->GetURI(getter_AddRefs(uri));

Except that doesn't explain why it doesn't appear on the 1.8 branch....  Maybe for some reason bfcache is not on for that page in the build in question?  Or the link was clicked before the page was done loading?

Comment 2

[Boris Zbarsky [:bzbarsky]]

I see this bug on the 1.8 and 1.8.0 branches, so _that_ much of the mystery is resolved.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Fixes bug, as expected

Comment 4

[Boris Zbarsky [:bzbarsky]]

For what it's worth, I do think this is worth fixing on branches...

Comment 5

[James Ross]

Wow, nicely found! :)  Don't know why Hannibal didn't see it on FF2, but I can't see any reason to not fix it on branch.

Comment 6

[:Gijs (he/him)]

Mm, upon trying to reproduce on a clean profile this morning, I can reproduce, though I should mention that whether or not you wait for the link in step 3 to load seems to affect the results. And even so, what I'm seeing is inconsistent (ie, with seemingly identical actions, results vary - I'm probably missing something).

Comment 7

[Daniel Veditz [:dveditz]]

Don't think this is a blocker for any branch security release, but we'll approve this patch once it's reviewed and trunk-baked.

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 245427 [details] [diff] [review]
Fixes bug, as expected

r=jst

Comment 9

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 245427 [details] [diff] [review]
Fixes bug, as expected

I do think we shold fix this on branches...  And I think this is a pretty safe fix.

Comment 11

[Jay Patel [:jay]]

Comment on attachment 245427 [details] [diff] [review]
Fixes bug, as expected

Approved for both branches, a=jay for drivers.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Fixed on branches.

Comment 13

[Carsten Book [:Tomcat]]

Verified on Trunk, 1.8.1.2 and 1.8.0.10 using the steps to reproduce from comment#1. URLBar (and document.location) show the URL with the hash part after going back.

Verified on Fedora FC 6 and on Windows XP x64 with :
Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9a2pre) Gecko/2007020611 Minefield/3.0a2pre for trunk

Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.2pre) Gecko/20070207 BonEcho/2.0.0.2pre ID:2007020703 for 1.8.1.2

Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.0.10pre) Gecko/20070207 Firefox/1.5.0.10pre ID:2007020706 for 1.8.0.10

Comment 14

[Jonathan Griffin (:jgriffin)]

Attachment: mochitest test case

Comment 15

[Boris Zbarsky [:bzbarsky]]

A few comments here:

1)  Replace (i == 1 ? false : true) with "i != 1".
2)  This test assumes the browser window is under 1000px tall.  That might stop
    being a good assumption...  I would rather see the markup guarantee that
    we'll have something to scroll to.
3)  I'd prefer that our random HTML files be in standards mode (have 
    <!DOCTYPE html> at the top).

Given 2 & 3, the markup for the test files should be something like (leaving out the head, etc):

  <!DOCTYPE html>
  <html style="height: 100%">
    <body style="height: 100%">
      <div style="height: 200%">hello large div</div>
      <a name="bottom"></a>
    </body>
  </html>

That will guarantee the div being 2x the height of the viewport.

Comment 16

[Jonathan Griffin (:jgriffin)]

Attachment: mochitest test case

Thanks for the review Boris.  I've applied all your suggestions. I've also added some additional checks that the page is really getting scrolled when we click on a fragment link.

I'll also start using <!DOCTYPE html> on top of random HTML files in these tests.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Pushed test as http://hg.mozilla.org/mozilla-central/rev/cdb21eed2fdf