Last Comment Bug 640387 - onhashchange not fired on navigation when navigation entries created via history.pushState
: onhashchange not fired on navigation when navigation entries created via hist...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: Justin Lebar (not reading bugmail)
:
: Andrew Overholt [:overholt]
Mentors:
http://zewt.org/~glenn/test-hashchang...
Depends on: 653741 662094 662170 694584
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-09 15:38 PST by Glenn Maynard
Modified: 2011-10-17 11:55 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (17.97 KB, patch)
2011-03-29 12:04 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Patch v1.1 (17.99 KB, patch)
2011-03-30 07:30 PDT, Justin Lebar (not reading bugmail)
bzbarsky: review-
Details | Diff | Splinter Review
Patch v2 (21.14 KB, patch)
2011-03-30 13:41 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Interdiff v1 -> v2 (12.52 KB, text/plain)
2011-03-30 14:00 PDT, Justin Lebar (not reading bugmail)
no flags Details
Patch v3 (21.76 KB, patch)
2011-03-31 09:26 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Interdiff v2 -> v3 (7.93 KB, text/plain)
2011-03-31 09:27 PDT, Justin Lebar (not reading bugmail)
no flags Details
Patch v4 (21.77 KB, patch)
2011-03-31 13:32 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Patch v5 (22.10 KB, patch)
2011-04-01 12:35 PDT, Justin Lebar (not reading bugmail)
justin.lebar+bug: review-
Details | Diff | Splinter Review
Part 2 v1 (3.55 KB, patch)
2011-04-06 17:32 PDT, Justin Lebar (not reading bugmail)
no flags Details | Diff | Splinter Review
Patch v6 (33.87 KB, patch)
2011-04-09 08:44 PDT, Justin Lebar (not reading bugmail)
bugs: review+
Details | Diff | Splinter Review
Interdiff v5 -> v6 (14.12 KB, text/plain)
2011-04-09 08:46 PDT, Justin Lebar (not reading bugmail)
no flags Details

Description Glenn Maynard 2011-03-09 15:38:56 PST
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) 2011-03-09 15:43:24 PST
Oh, that's bad.  I wonder if this regressed from the changes to pushstate we made recently.
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2011-03-09 19:25:19 PST
Justin, want to take this?
Comment 3 Justin Lebar (not reading bugmail) 2011-03-09 22:38:57 PST
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 [:bz] (still a bit busy) 2011-03-09 23:42:23 PST
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) 2011-03-29 12:04:34 PDT
Created attachment 522759 [details] [diff] [review]
Patch v1
Comment 6 Justin Lebar (not reading bugmail) 2011-03-30 07:30:24 PDT
Created attachment 523006 [details] [diff] [review]
Patch v1.1

Closing body/html tags in test.
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2011-03-30 07:50:37 PDT
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) 2011-03-30 13:09:50 PDT
In this case, should we fire a hashchange event?
Comment 9 Justin Lebar (not reading bugmail) 2011-03-30 13:12:43 PDT
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) 2011-03-30 13:13:17 PDT
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) 2011-03-30 13:41:13 PDT
Created attachment 523096 [details] [diff] [review]
Patch v2
Comment 12 Justin Lebar (not reading bugmail) 2011-03-30 14:00:41 PDT
Created attachment 523107 [details]
Interdiff v1 -> v2
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2011-03-31 07:06:49 PDT
> In this case, should we fire a hashchange event?

No; comment 9 is correct.
Comment 14 Olli Pettay [:smaug] 2011-03-31 08:28:36 PDT
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) 2011-03-31 09:26:03 PDT
Created attachment 523333 [details] [diff] [review]
Patch v3
Comment 16 Justin Lebar (not reading bugmail) 2011-03-31 09:27:35 PDT
Created attachment 523334 [details]
Interdiff v2 -> v3
Comment 17 Justin Lebar (not reading bugmail) 2011-03-31 09:29:55 PDT
nsContentUtils::SplitURIAtHash should take const nsACStrings; please consider that fixed.
Comment 18 Justin Lebar (not reading bugmail) 2011-03-31 09:31:06 PDT
(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) 2011-03-31 13:32:44 PDT
Created attachment 523415 [details] [diff] [review]
Patch v4
Comment 20 Justin Lebar (not reading bugmail) 2011-04-01 12:35:23 PDT
Created attachment 523658 [details] [diff] [review]
Patch v5
Comment 21 Justin Lebar (not reading bugmail) 2011-04-01 12:36:05 PDT
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] 2011-04-03 10:03:03 PDT
Is the latest patch ready for review?
Comment 23 Justin Lebar (not reading bugmail) 2011-04-03 16:46:11 PDT
Yes; sorry about that.  I was having trouble with bzexport, and I forgot to set the review tag manually.
Comment 24 Olli Pettay [:smaug] 2011-04-03 19:51:17 PDT
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) 2011-04-03 21:33:49 PDT
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] 2011-04-03 21:52:53 PDT
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 2011-04-03 22:19:18 PDT
(2-space indentation makes *all* code hard to read.)
Comment 28 Justin Lebar (not reading bugmail) 2011-04-06 16:43:00 PDT
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) 2011-04-06 17:32:46 PDT
Created attachment 524316 [details] [diff] [review]
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) 2011-04-09 08:44:21 PDT
Created attachment 524853 [details] [diff] [review]
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) 2011-04-09 08:46:37 PDT
Created attachment 524854 [details]
Interdiff v5 -> v6
Comment 32 Olli Pettay [:smaug] 2011-04-20 11:03:35 PDT
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] 2011-04-20 11:04:15 PDT
Ah, ok, seems like the SHEntry is updated a bit later...
Comment 34 Justin Lebar (not reading bugmail) 2011-04-20 11:22:33 PDT
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) 2011-04-20 13:38:22 PDT
http://hg.mozilla.org/mozilla-central/rev/228d41c97600
Comment 36 Glenn Maynard 2011-06-22 12:30:57 PDT
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) 2011-06-22 12:40:57 PDT
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 [:bz] (still a bit busy) 2011-06-22 12:59:05 PDT
Glenn, for future reference you can typically see which release a change will be in by looking at the "Target Milestone" field.

Note You need to log in before you can comment on or make changes to this bug.