Closed Bug 628069 Opened 14 years ago Closed 13 years ago

onhashchange event object missing newURL, oldURL properties


(Core :: DOM: Navigation, defect)

Not set





(Reporter: webkit, Assigned: justin.lebar+bug)



(Keywords: dev-doc-complete, html5)


(2 files, 2 obsolete files)

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b10pre) Gecko/20110121 Firefox/4.0b10pre
Build Identifier: 4.0b10pre

HTML5 specifies that the event object created for the hashchange event should have two additional properties: newURL and oldURL. These are missing in Firefox.

Reproducible: Always

Steps to Reproduce:
window.onhashchange = function(event){
alert("Old URL: " + event.oldURL + "\nNew URL: " + event.newURL);
Actual Results:  
alert displays undefined for both values

Expected Results:  
alert displays the URLs

Works correctly in the latest versions of Opera and Chrome. Does not work correctly in IE8, IE9, or Safari 5.
I suspect our support for this predates this spec section...
Component: General → Document Navigation
QA Contact: general → docshell
Ever confirmed: true
Keywords: html5
I can confirm your suspicions!  I didn't get the memo that this had changed...

Is this something we want for FF4?  It doesn't seem like such a big deal to me, considering that we shipped hashchange in 3.6...
This is So Out Of Scope for 4 at this point.

I wish there HTML5 had a sane annotation functionality so you can check _when_ the text you're looking at was last messed with.  People keep thinking that if the page says something then it's said that forever.  :(
Attached patch Patch v1 (obsolete) — Splinter Review
One of the docshell hunks here will apply cleanly only atop the patch for bug 640387.
Attachment #523005 - Flags: review?(bzbarsky)
Comment on attachment 523005 [details] [diff] [review]
Patch v1

Olli is a better reviewer for this.
Attachment #523005 - Flags: review?(bzbarsky) → review?(Olli.Pettay)
Comment on attachment 523005 [details] [diff] [review]
Patch v1

>+nsresult NS_NewDOMHashChangeEvent(nsIDOMEvent** aInstancePtrResult,
>+                                nsPresContext* aPresContext,
>+                                nsEvent* aEvent)
>+  nsDOMHashChangeEvent* event =
>+    new nsDOMHashChangeEvent(aPresContext, aEvent);
>+  if (!event) {
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  }
'new ' is infallible, so there shouldn't be need for the OOM check

>+ *
>+ * The Initial Developer of the Original Code is the Mozilla Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 2011
>+ * the Initial Developer. All Rights Reserved.

>+  rv = privateEvent->SetTarget(outerWindow);
No need for this. 

>+  nsresult FireHashchange(nsAString &aOldURL, nsAString &aNewURL);
Should these be const

>+  virtual nsresult DispatchAsyncHashchange(nsAString& aOldURL,
>+                                           nsAString& aNewURL) = 0;
Same here

Please add tests where you actually create, init and dispatch hashchange events using a script.

Is it guaranteed that oldURL does not leak any cross-domain information?
Attachment #523005 - Flags: review?(Olli.Pettay) → review-
> Is it guaranteed that oldURL does not leak any cross-domain information?

You should only get a hashchange if the URLs before the hash marks match.

That said, this is important enough that it's probably worth checking in DispatchAsyncHashchange.
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #523077 - Flags: review?(Olli.Pettay)
Attachment #523005 - Attachment is obsolete: true
Attached file Interdiff v1 -> v2
Attachment #523078 - Attachment mime type: application/octet-stream → text/plain
Assignee: nobody → justin.lebar+bug
Comment on attachment 523077 [details] [diff] [review]
Patch v2

>+nsGlobalWindow::DispatchAsyncHashchange(const nsAString &aOldURL,
>+                                        const nsAString &aNewURL)
>+  FORWARD_TO_INNER(DispatchAsyncHashchange, (aOldURL, aNewURL), NS_OK);
>+  const char* mismatchWarning =
>+    "URLs passed to DispatchAsyncHashchange differ in more than their hashes. "
>+    "This is likely a bug in the caller.";
>+  // Make sure that aOldURL and aNewURL are identical up to the '#'.
>+  const PRUnichar *oldIter = aOldURL.BeginReading();
>+  const PRUnichar *oldEnd  = aOldURL.EndReading();
>+  const PRUnichar *newIter = aNewURL.BeginReading();
>+  const PRUnichar *newEnd  = aNewURL.EndReading();
>+  for (; oldIter < oldEnd && newIter < newEnd; oldIter++, newIter++)
>+  {
>+    if (*oldIter == '#' || *newIter == '#')
>+      break;
>+    if (*oldIter != *newIter) {
>+      NS_WARNING(mismatchWarning);
>+      return NS_ERROR_FAILURE;
>+    }
>+  }
>+  // We exited the loop above either because we saw '#' in one of the URLs, or
>+  // because we ran off the end of one of the URLs.  Make sure that the other
>+  // URL is either at '#' or at the end.
>+  if (oldIter < oldEnd && *oldIter != '#') {
>+    NS_WARNING(mismatchWarning);
>+    return NS_ERROR_FAILURE;
>+  }
>+  if (newIter < newEnd && *newIter != '#') {
>+    NS_WARNING(mismatchWarning);
>+    return NS_ERROR_FAILURE;
>+  }

This all looks quite complicated.
Couldn't you use string.FindChar()?
Something like

old = aOldURL.FindChar('#');
new = aNewURL.FindChar('#');
if (old == -1) old = aOldURL.Length();
if (new == -1) new = aNewURL.Length();
NS_ENSURE_STATE(nsDependentSubstring(aOldURL, 0, old).Equals(nsDependendSubstring(aNewURL, 0, new));
That would be a bit slower, but this shouldn't be performance critical.

>+  nsCOMPtr<nsIDOMEventTarget> outerWindow =
>+    do_QueryInterface(GetOuterWindow());
You don't use outerWindow for anything.
Attachment #523077 - Flags: review?(Olli.Pettay) → review-
> Couldn't you use string.FindChar()?

Yes, that would be much nicer!
Attached patch Patch v3Splinter Review
Attachment #523077 - Attachment is obsolete: true
Attachment #523412 - Flags: review?(Olli.Pettay)
As before, you need the patch from bug 640387, which has nsContentUtils::SplitURIAtHash.
Comment on attachment 523412 [details] [diff] [review]
Patch v3

>+class HashchangeCallback : public nsRunnable {
Nit, { should be in the next line

>+  nsCOMPtr<nsIRunnable> callback =
>+    new HashchangeCallback(oldWideSpec, newWideSpec, this);
>+  return NS_DispatchToMainThread(callback);
Not about this bug, but seems like current spec requires sync dispatch.
I need to read again all those emails about sync/async issues.
Attachment #523412 - Flags: review?(Olli.Pettay) → review+

I noticed that I didn't move the brace on HashchangeCallback.  I'll fix it next time I push.
Closed: 13 years ago
Resolution: --- → FIXED
Keywords: dev-doc-needed
OS: Windows XP → All
Hardware: x86 → All
Target Milestone: --- → mozilla6
Version: unspecified → Trunk
Documentation updated:

And mentioned on Firefox 6 for developers.
My addon contains the following code

handleEvent : function bug420605_handleEvent(aEvent) {
    switch (aEvent.type) {


      case "hashchange":
        if (aEvent.isTrusted)


aEvent.isTrusted is now undefined here.
Hmm.  The classinfo for the new event doesn't include nsIDOMNSEvent.  That's true for popstate as well, looks like.  John, can you please file a followup bug on that?
(In reply to comment #19)
> Hmm.  The classinfo for the new event doesn't include nsIDOMNSEvent.  That's
> true for popstate as well, looks like.

> John, can you please file a followup
> bug on that?

And please cc me!
Depends on: 653364
Build ID: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0) Gecko/20100101 Firefox/6.0

The changes made in m-c and landed (comment 15, comment 16) are visible in hg, under beta (i.e. addition of reference to nsDOMHashChangeEvent.cpp
Also, the tested with the test htmls added (i.e.File_bug628069.html) which worked fine.
Setting this as verified for Firefox 6 Beta
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.