onhashchange event object missing newURL, oldURL properties

VERIFIED FIXED in mozilla6

Status

()

Core
Document Navigation
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: Nicholas C. Zakas, Assigned: Justin Lebar (not reading bugmail))

Tracking

({dev-doc-complete, html5})

Trunk
mozilla6
dev-doc-complete, html5
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: html5
(Assignee)

Comment 2

6 years ago
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.  :(
(Assignee)

Comment 4

6 years ago
Created attachment 523005 [details] [diff] [review]
Patch v1

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 6

6 years ago
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?
Attachment #523005 - Flags: review?(Olli.Pettay) → review-
(Assignee)

Comment 7

6 years ago
> 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.
(Assignee)

Comment 8

6 years ago
Created attachment 523077 [details] [diff] [review]
Patch v2
Attachment #523077 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

6 years ago
Attachment #523005 - Attachment is obsolete: true
(Assignee)

Comment 9

6 years ago
Created attachment 523078 [details]
Interdiff v1 -> v2
(Assignee)

Updated

6 years ago
Attachment #523078 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Updated

6 years ago
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());
>+  NS_ENSURE_TRUE(outerWindow, NS_ERROR_UNEXPECTED);
You don't use outerWindow for anything.
Attachment #523077 - Flags: review?(Olli.Pettay) → review-
(Assignee)

Comment 11

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

Yes, that would be much nicer!
(Assignee)

Comment 12

6 years ago
Created attachment 523412 [details] [diff] [review]
Patch v3
(Assignee)

Updated

6 years ago
Attachment #523077 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #523412 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 13

6 years ago
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+
(Assignee)

Updated

6 years ago
(Assignee)

Comment 15

6 years ago
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.
(Assignee)

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(Assignee)

Comment 16

6 years ago
Pushed whitespace followup.

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

Updated

6 years ago
Keywords: dev-doc-needed
OS: Windows XP → All
Hardware: x86 → All
Target Milestone: --- → mozilla6
Version: unspecified → Trunk
Documentation updated:

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

And mentioned on Firefox 6 for developers.
Keywords: dev-doc-needed → dev-doc-complete

Comment 18

6 years ago
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.
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?
(Assignee)

Comment 20

6 years ago
(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!

Updated

6 years ago
Depends on: 653364

Comment 21

6 years ago
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
Status: RESOLVED → VERIFIED

Updated

6 years ago
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.