628069 - onhashchange event object missing newURL, oldURL properties

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect)

Description

[Nicholas C. Zakas]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

I suspect our support for this predates this spec section...

Comment 2

[Justin Lebar (not reading bugmail)]

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...

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.  :(

Comment 4

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

One of the docshell hunks here will apply cleanly only atop the patch for bug 640387.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 523005 [details] [diff] [review]
Patch v1

Olli is a better reviewer for this.

Comment 6

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

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.
s/Corporation/Foundation/

>+  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?

Comment 7

[Justin Lebar (not reading bugmail)]

> 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.

Comment 8

[Justin Lebar (not reading bugmail)]

Attachment: Patch v2

Comment 9

[Justin Lebar (not reading bugmail)]

Attachment: Interdiff v1 -> v2

Comment 10

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

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());
>+  NS_ENSURE_TRUE(outerWindow, NS_ERROR_UNEXPECTED);
You don't use outerWindow for anything.

Comment 11

[Justin Lebar (not reading bugmail)]

> Couldn't you use string.FindChar()?

Yes, that would be much nicer!

Comment 12

[Justin Lebar (not reading bugmail)]

Attachment: Patch v3

Comment 13

[Justin Lebar (not reading bugmail)]

As before, you need the patch from bug 640387, which has nsContentUtils::SplitURIAtHash.

Comment 14

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

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.

Comment 15

[Justin Lebar (not reading bugmail)]

http://hg.mozilla.org/mozilla-central/rev/84bf34708885

I noticed that I didn't move the brace on HashchangeCallback.  I'll fix it next time I push.

Comment 16

[Justin Lebar (not reading bugmail)]

Pushed whitespace followup.

http://hg.mozilla.org/mozilla-central/rev/772d6e702817

Comment 17

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en/DOM/window.onhashchange#The_hashchange_event

And mentioned on Firefox 6 for developers.

Comment 18

[John P Baker]

My addon contains the following code

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

...

      case "hashchange":
        if (aEvent.isTrusted)
          this.onHashChange(aEvent);
        break;

...

aEvent.isTrusted is now undefined here.

Comment 19

[Boris Zbarsky [:bzbarsky]]

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?

Comment 20

[Justin Lebar (not reading bugmail)]

(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!

Comment 21

[AndreiD[QA]]

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 Makefile.in).
Also, the tested with the test htmls added (i.e.File_bug628069.html) which worked fine.
Setting this as verified for Firefox 6 Beta