640387 - onhashchange not fired on navigation when navigation entries created via history.pushState

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Glenn Maynard]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9.2.8) Gecko/20100722 Firefox/3.6.8 (.NET CLR 3.5.30729)
Build Identifier: FF4 RC1

When history states that only change the URL hash are added via history.pushState, navigating via browser back does not fire onhashchange.


Reproducible: Always

Steps to Reproduce:
1. Open the web console.
2. Load http://zewt.org/~glenn/test-hashchange-ff4.html.  The page will add several history states.
3. Click browser back to navigate back to "/test-hashchange-ff4.html#2".

A single "hashchange: #2" should be logged to the console; it isn't.

Comment 1

[Justin Lebar (not reading bugmail)]

Oh, that's bad.  I wonder if this regressed from the changes to pushstate we made recently.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Justin, want to take this?

Comment 3

[Justin Lebar (not reading bugmail)]

I can't until I finish school and move, March 23 or so at the earliest.

So if we want to fix this for the release, no way.  :)

Comment 4

[Boris Zbarsky [:bzbarsky]]

I don't think we're going to be changing this for the release at this point, unless someone really thinks this is a stop-ship blocker.  I certainly don't think it is.

Comment 5

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

Comment 6

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1.1

Closing body/html tags in test.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 523006 [details] [diff] [review]
Patch v1.1

This changes behavior in a way that matters, I think.

In particular, when going from

  http://example.com/#foo

to

  http://example.com/

via a non-history load the old code does a load instead of a scroll.  It looks like your new code will do a scroll.  If we don't have a test for this already, we should.  That's what this block is all about:

     if (hashNew == kNotFound &&
         (hashCurrent == kNotFound || aLoadType != LOAD_HISTORY)) {
         return NS_OK;
     }

in the old code.

Comment 8

[Justin Lebar (not reading bugmail)]

In this case, should we fire a hashchange event?

Comment 9

[Justin Lebar (not reading bugmail)]

6.5.9 History traversal, step 6:

> If the specified entry has a URL whose fragment identifier differs from that of > the current entry's when compared in a case-sensitive manner, and the two share > the same Document object, then let hash changed be true

Doing a real load creates a new document object (right?), so I guess we're off the hook.

Comment 10

[Justin Lebar (not reading bugmail)]

Quote fail.

6.5.9 History traversal, step 6:

> If the specified entry has a URL whose fragment identifier differs from that of 
> the current entry's when compared in a case-sensitive manner, and the two share 
> the same Document object, then let hash changed be true.

Comment 11

[Justin Lebar (not reading bugmail)]

Attachment: Patch v2

Comment 12

[Justin Lebar (not reading bugmail)]

Attachment: Interdiff v1 -> v2

Comment 13

[Boris Zbarsky [:bzbarsky]]

> In this case, should we fire a hashchange event?

No; comment 9 is correct.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 523096 [details] [diff] [review]
Patch v2

Based on IRC I'm expecting some re-factoring related to hash parsing.

Comment 15

[Justin Lebar (not reading bugmail)]

Attachment: Patch v3

Comment 16

[Justin Lebar (not reading bugmail)]

Attachment: Interdiff v2 -> v3

Comment 17

[Justin Lebar (not reading bugmail)]

nsContentUtils::SplitURIAtHash should take const nsACStrings; please consider that fixed.

Comment 18

[Justin Lebar (not reading bugmail)]

(In reply to comment #17)
> nsContentUtils::SplitURIAtHash should take const nsACStrings; please consider
> that fixed.

Whoops; nevermind.  I was looking at the wrong function.

Comment 19

[Justin Lebar (not reading bugmail)]

Attachment: Patch v4

Comment 20

[Justin Lebar (not reading bugmail)]

Attachment: Patch v5

Comment 21

[Justin Lebar (not reading bugmail)]

My docshell logic wasn't quite right in v4 (detect by existing test, browser_500328.js).  It's improved in v5.  Perhaps it's even correct.

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Is the latest patch ready for review?

Comment 23

[Justin Lebar (not reading bugmail)]

Yes; sorry about that.  I was having trouble with bzexport, and I forgot to set the review tag manually.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 523658 [details] [diff] [review]
Patch v5

>+  PRInt32 index = spec.FindChar('#');
>+  if (index == -1)
>+    index = spec.Length();
if () {
...
}


>+        // If we have no new anchor, we do not want to do a short-circuited
>+        // load, unless there is a current anchor and we are doing a history
>+        // load.
>+        PRBool doAnchorLoad = sameExceptHashes &&
>+            (!newHash.IsEmpty() ||
>+             (!curHash.IsEmpty() && aLoadType == LOAD_HISTORY));
Indentation

Comment 25

[Justin Lebar (not reading bugmail)]

Not to nit the nit, but...

> if () {
> ...
> }

My understanding is that the style guideline for this kind of thing is "match the surroundings."  I've read a variety of style documents, but we seem to actually follow only a few rules.

$ grep 'if\s*(.*)\s*$' nsContentUtils.cpp  | wc -l
73
$ grep 'if\s*(.*) {\s*$' nsContentUtils.cpp  | wc -l
372

The ratio is the same for content/base/src/*.cpp.  This doesn't indicate to me a strong preference for one form over another, since of course many of the if () {}'s are multiline if's.

Are we trying to move towards if () {} in new code?

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

if (expr) {
  statement;
}
is what the coding style says we should be doing.
In some code the rules haven't been followed, but IMHO,
in new code the rules should be followed - whenever possible.
(Though, using 2 space indentation in a code which uses 4 spaces would
 make the code hard to read.)

(Javascript code and Spidermonkey code may have different rules, and
 some files may unfortunately use their own rules.)

Comment 27

[Glenn Maynard]

(2-space indentation makes *all* code hard to read.)

Comment 28

[Justin Lebar (not reading bugmail)]

Comment on attachment 523658 [details] [diff] [review]
Patch v5

This is not quite right, unfortunately.

Suppose our history is

  foo.html#a
  bar.html
  foo.html#b  <-- current entry

and suppose we go(-2).  This patch will fire a hashchange, but that's wrong, since the two entries for foo.html correspond to different documents.

Comment 29

[Justin Lebar (not reading bugmail)]

Attachment: Part 2 v1

This, on top of v5, seems to fix the problem.  But I need to push to try.

Comment 30

[Justin Lebar (not reading bugmail)]

Attachment: Patch v6

It's finally green on try!

I ended up rewriting the docshell short-circuit load code, and it's entirely posssible I've changed some behavior, despite passing the tests, so I'd appreciate a close look at the changes to InternalLoad.

Comment 31

[Justin Lebar (not reading bugmail)]

Attachment: Interdiff v5 -> v6

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 524853 [details] [diff] [review]
Patch v6


>-
>-                if (mLSHE && wasAnchor) {
>-                    // If it's an anchor load, set mLSHE's doc identifier to
>-                    // mOSHE's doc identifier -- These are the same documents,
>-                    // as far as HTML5 is concerned.
>-                    PRUint64 docIdent;
>-                    rv = mOSHE->GetDocIdentifier(&docIdent);
>-                    if (NS_SUCCEEDED(rv)) {
>-                        mLSHE->SetDocIdentifier(docIdent);
>-                    }
>-                }


Could you explain this. Why don't we need to update mLSHE anymore?

Comment 33

[Olli Pettay [:smaug][[e-mail]]]

Ah, ok, seems like the SHEntry is updated a bit later...

Comment 34

[Justin Lebar (not reading bugmail)]

Smaug asked me on IRC to comment about the docIdentifier / pageIdentifier change.

pageIdentifier existed before pushState.  Two SHEntries linked by only a difference in hashes have the same page identifier.

When I added pushState, I added document identifiers.  Two SHEntries have the same doc identifier if they had the same page identifier or if they were linked by pushState.

In some sense, the decision to keep both page and document identifiers was the root cause of this bug.  As far as I can tell, there should be no difference between a hash navigation and a pushstate.

The spec says (*) that if we have

  a) foo.html      pushstates to
  b) bar.html      pushstates to
  c) foo.html#bar

and we go back from (c) to (a), we should fire hashchange (because it's navigation within a document). So it's not necessary for us to keep track of the fact that (a) and (c) aren't directly linked by anchor navigations (i.e. that they would have different page identifiers).

(*) http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#history-traversal step 6:

If the specified entry has a URL whose fragment identifier differs from that of the current entry's when compared in a case-sensitive manner, and the two share the same Document object, then let hash changed be true

Comment 35

[Justin Lebar (not reading bugmail)]

http://hg.mozilla.org/mozilla-central/rev/228d41c97600

Comment 36

[Glenn Maynard]

Did this fix not make it into the FF5 release, even though it was made almost two months earlier?  This problem is still happening, with the same test case.

Comment 37

[Justin Lebar (not reading bugmail)]

Correct, the change didn't make it into Firefox 5.  FF5 branched on 4/12, but the change was checked in on 4/20.  The change is in Firefox 6, which is currently in the aurora channel.

The calendar is at https://wiki.mozilla.org/RapidRelease/Calendar  The pipeline is 12 weeks deep, so it takes at most 3 months for a change to make it from trunk to a release.

Comment 38

[Boris Zbarsky [:bzbarsky]]

Glenn, for future reference you can typically see which release a change will be in by looking at the "Target Milestone" field.