Last Comment Bug 628069 - onhashchange event object missing newURL, oldURL properties
: onhashchange event object missing newURL, oldURL properties
Status: VERIFIED FIXED
: dev-doc-complete, html5
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla6
Assigned To: Justin Lebar (not reading bugmail)
:
:
Mentors:
Depends on: 653364
Blocks:
  Show dependency treegraph
 
Reported: 2011-01-22 16:42 PST by Nicholas C. Zakas
Modified: 2011-10-20 22:41 PDT (History)
7 users (show)
jruderman: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (26.91 KB, patch)
2011-03-30 07:25 PDT, Justin Lebar (not reading bugmail)
bugs: review-
Details | Diff | Splinter Review
Patch v2 (29.51 KB, patch)
2011-03-30 12:05 PDT, Justin Lebar (not reading bugmail)
bugs: review-
Details | Diff | Splinter Review
Interdiff v1 -> v2 (10.81 KB, text/plain)
2011-03-30 12:06 PDT, Justin Lebar (not reading bugmail)
no flags Details
Patch v3 (29.24 KB, patch)
2011-03-31 13:31 PDT, Justin Lebar (not reading bugmail)
bugs: review+
Details | Diff | Splinter Review

Description Nicholas C. Zakas 2011-01-22 16:42:51 PST
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 [:bz] (still a bit busy) 2011-01-22 19:16:13 PST
I suspect our support for this predates this spec section...
Comment 2 Justin Lebar (not reading bugmail) 2011-01-22 19:20:21 PST
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 [:bz] (still a bit busy) 2011-01-22 19:29:43 PST
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) 2011-03-30 07:25:51 PDT
Created attachment 523005 [details] [diff] [review]
Patch v1

One of the docshell hunks here will apply cleanly only atop the patch for bug 640387.
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2011-03-30 07:31:38 PDT
Comment on attachment 523005 [details] [diff] [review]
Patch v1

Olli is a better reviewer for this.
Comment 6 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-03-30 07:58:06 PDT
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) 2011-03-30 10:09:09 PDT
> 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) 2011-03-30 12:05:32 PDT
Created attachment 523077 [details] [diff] [review]
Patch v2
Comment 9 Justin Lebar (not reading bugmail) 2011-03-30 12:06:04 PDT
Created attachment 523078 [details]
Interdiff v1 -> v2
Comment 10 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-03-31 07:53:28 PDT
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) 2011-03-31 07:59:39 PDT
> Couldn't you use string.FindChar()?

Yes, that would be much nicer!
Comment 12 Justin Lebar (not reading bugmail) 2011-03-31 13:31:05 PDT
Created attachment 523412 [details] [diff] [review]
Patch v3
Comment 13 Justin Lebar (not reading bugmail) 2011-03-31 13:35:33 PDT
As before, you need the patch from bug 640387, which has nsContentUtils::SplitURIAtHash.
Comment 14 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-04-03 10:10:42 PDT
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) 2011-04-20 13:37:29 PDT
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) 2011-04-20 14:36:17 PDT
Pushed whitespace followup.

http://hg.mozilla.org/mozilla-central/rev/772d6e702817
Comment 17 Eric Shepherd [:sheppy] 2011-04-22 11:52:38 PDT
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 2011-04-27 04:12:33 PDT
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 [:bz] (still a bit busy) 2011-04-27 06:04:19 PDT
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) 2011-04-27 08:09:09 PDT
(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] 2011-07-29 07:00:06 PDT
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

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