Last Comment Bug 198595 - event.target, event.currentTarget and the this object are not set consistently in onload handlers of XMLHttpRequest and XMLDocument
: event.target, event.currentTarget and the this object are not set consistentl...
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: XML (show other bugs)
: Trunk
: x86 Windows 98
: -- normal (vote)
: ---
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
: Ashish Bhatt
:
Mentors:
: 341610 (view as bug list)
Depends on: 334368 295340 324865 332528 337374 361811 437651
Blocks: 290127 310602 312213
  Show dependency treegraph
 
Reported: 2003-03-21 07:13 PST by Martin Honnen
Modified: 2008-07-03 09:35 PDT (History)
23 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Image needed for test case to demonstrate normal onload event handler settings (1.77 KB, image/gif)
2003-03-21 07:14 PST, Martin Honnen
no flags Details
XML file needed for testing onload of XMLDocument and XMLHttpRequest (103 bytes, text/xml)
2003-03-21 07:16 PST, Martin Honnen
no flags Details
test case (same onload handler function is attached to window object, HTMLImage object, XMLHttpRequest object and XMLDocument object, for the last two there are inconsistencies as already described (1.67 KB, text/html)
2003-03-21 07:24 PST, Martin Honnen
no flags Details
demonstrate the failure to set "this" properly in XMLHttpRequest methods (1.60 KB, text/html)
2005-09-29 11:29 PDT, howard
no flags Details
Fix (27.07 KB, patch)
2006-03-13 22:08 PST, Boris Zbarsky [:bz] (still a bit busy)
jonas: review+
peterv: superreview+
Details | Diff | Splinter Review
Merged to tip (28.34 KB, patch)
2006-04-30 17:27 PDT, Boris Zbarsky [:bz] (still a bit busy)
jonas: review+
Details | Diff | Splinter Review
Patch that I checked in (25.64 KB, patch)
2006-05-05 10:16 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review

Description Martin Honnen 2003-03-21 07:13:08 PST
Usually (since Netscape 4 at least) when you assign a function as an event
handler with JavaScript to an object and the event handler is called then the
"this" object is set to the object the handler function has been assigned to.
However with the XMLHttpRequest if you assign a function as an onload handler
and that is executed the "this" object is not the XMLHttpRequest instance but
the function. This is very odd and inconsistent with the way the "this" object
is set for other event handlers in Mozilla. If the "this" object is set as
normal then you can implement one function and assign it as the event handler of
different object, using "this" the handler can refer to the current object it is
called on.
Furthermore with the XMLHttpRequest object the event object property
currentTarget in the onload handler is null, in my view it should be the
XMLHttpRequest instance the handler is being fired on to be consistent with the
use of the event.currentTarget with other event handlers in Mozilla. That again
would allow to use one function and assign it as the handler to different
objects, the function code can then access event.currentTarget to refer to the
object the handler is being called on. (Admittedly with this argument for both
the "this" object and the event.currentTarget property you could only fix one
but usually both properties are set as requested).

What is set with an onload handler and an XMLHttpRequest instance is the
event.target object. This property however is null in the onload handler of an
XMLDocument (created with document.implementation.createDocument) while there
the "this" object and the event.currentTarget are set as usual. Thus I request
to make XMLHttpRequest and XMLDocument onload handler/event initialization
consistent and set event.target and event.currentTarget and the "this" object to
the object the handler is called on.

I will attach a test case that demonstrates the different and inconsistent settings.

I have been testing with Mozilla 1.3 (Mozilla/5.0 (Windows; U; Win98; en-US;
rv:1.3) Gecko/20030312) and with Netscape 7.02 (Mozilla/5.0 (Windows; U; Win98;
en-US; rv:1.0.2) Gecko/20030208 Netscape/7.02) so this is probably in the 1.0
branch as well as the trunk.
Comment 1 Martin Honnen 2003-03-21 07:14:49 PST
Created attachment 118017 [details]
Image needed for test case to demonstrate normal onload event handler settings
Comment 2 Martin Honnen 2003-03-21 07:16:35 PST
Created attachment 118018 [details]
XML file needed for testing onload of XMLDocument and XMLHttpRequest
Comment 3 Martin Honnen 2003-03-21 07:24:26 PST
Created attachment 118019 [details]
test case (same onload handler function is attached to window object, HTMLImage object, XMLHttpRequest object and XMLDocument object, for the last two there are inconsistencies as already described

As the page shows, the onload handler attached to the window object has
event.currentTarget and the "this" object set to the window object.
For the HTMLImage object, event.currentTarget and the "this" object are set to
the HTMLImage object.
However for the XMLHttpRequest object, event.currentTarget and the "this"
object are not set.
For the XMLDocument the event.target is not set.
Comment 4 Martin Honnen 2003-03-21 07:32:09 PST
Comment 1 describes the bug correctly, in comment 3
(http://bugzilla.mozilla.org/show_bug.cgi?id=198595#c3) that I made while
uploading the test case I incorrectly state that for the XMLHttpRequest object
the "this" object is not set, however it is set but very oddly to the function
itself and not to the XMLHttpRequest object.
Comment 5 Heikki Toivonen (remove -bugzilla when emailing directly) 2003-05-01 16:27:57 PDT
currentTarget and originalTarget (does this need to be set too?) can be set
easily for XMLHttpRequest in nsXMLHttpRequest::RequestCompleted():

  privevent->SetTarget(this);
+ privevent->SetCurrentTarget(this);
+ privevent->SetOriginalTarget(this);

I don't know how to set the others at the moment.
Comment 6 Gernot Fricke 2004-05-03 04:09:38 PDT
The problem is more common:

when defining an user defined object's method as eventhandler for a html-object
(e.g. A,TD,TABLE,DIV etc) (afair ANY event) then in the method "this" points to
the HTML object invoking the event, not to the user defined object.

Gernot
Comment 7 howard 2005-09-29 11:27:22 PDT
This also applies to the onreadystatechange method. Different browsers
exhibit different behavior, only one of which makes sense -- given

    x = new XMLHttpRequest();
    x.onreadystatechange = function() {
        alert('this==x: ' + (this === x));
    }

I'd expect the alert box to report 'true'; but in firefox (1.0.7, 1.5b),
the "this" object is apparently the onreadystatechange function itself.

FWIW, 
    Opera 8 acts as I'd expect
    IE6 says x===window, but it's not, really: window.readyState is undefined,
        while x.readyState has a numeric value, after calls to x.open/send
    Safari 1.2 acts like IE6
    
The apparent equivalence of "this" and arguments.callee is phony and
unusable though; there's no "arguments.callee.readyState". So, *NOW*, I
understand complaints on various Javascript lists and groups about the
"requirement" that you use globals with XMLHttpRequest: it's caused by
the failure to properly set "this".

Attachment added.
Comment 8 howard 2005-09-29 11:29:22 PDT
Created attachment 197883 [details]
demonstrate the failure to set "this" properly in XMLHttpRequest methods
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2006-03-09 22:20:12 PST
So the real issue is that the onload for XMLHttpRequest in Gecko is a DOM EventListener, not a DOM0 function....  as a result, it acts like DOM EventListeners do -- there's no binding of |this| to the event target.

We could try to change this, I suppose, but should we?  Ian?
Comment 10 Hixie (not reading bugmail) 2006-03-10 12:12:48 PST
What does DOM Events say EventListeners should have their "this" bound to? And where does it say it? Is it interoperably implemented?

I think XMLHttpRequest.onreadystatechange should work the same way as window.onload and so forth. I would like to make the HTML5 spec say that.
Comment 11 Alex Vincent [:WeirdAl] 2006-03-10 12:20:49 PST
Hixie, it's an interesting problem.  Officially, the spec states event listeners should be of the format:

var listener = {
  handleEvent: function handleEvent(evt) {
  }
}
node.addEventListener("foo", listener, true);

However, we have for ages supported:

node.addEventListener("foo", function evtListener(evt) {
}, true);

I remember asking about that specifically several years ago, and jst I believe was the one who told me that was allowed by the spec.  Under this latter stance, the "this" object within the event listener isn't defined by the Events spec, as I understand it...
Comment 12 Alex Vincent [:WeirdAl] 2006-03-10 12:23:01 PST
For the latter definition, reference Appendix C of DOM Level 2 Events.
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2006-03-10 12:24:53 PST
> What does DOM Events say EventListeners should have their "this" bound to?

Undefined in the spec.  All the bindings say is:

  Object EventListener
    This is an ECMAScript function reference. This method has no return value.
    The parameter is a Event object.

I guess for addEventListener and so forth we do rebind |this|, so we should do the same for XMLHttpRequest.

Alex, please read http://www.w3.org/TR/2000/REC-DOM-Level-2-Events-20001113/ecma-script-binding.html carefully.
Comment 14 Hixie (not reading bugmail) 2006-03-10 12:31:56 PST
If window.onload=function(){}, <body onload="">, and addEventListener("load", function(){}, false) all behave the same, then XMLHttpRequest.onreadystatechange should as well. I've forwarded your comments to the W3C for fixing DOM3 Events.
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2006-03-10 12:57:47 PST
Ah, I see.  nsIDOMEventListener is marked [function], so we invoke the function when any method on that interface (there's only one) is called....

In any case, we jump into JS via explicit call to js_Invoe in the "not getter, not setter, fval is not primitive" branch in nsXPCWrappedJSClass::CallMethod.  Is there some way we could:

1)  Indicate what the |this| for this function should be.
2)  Make it so?

Or should we not try to solve this in XPConnect and just have XMLHttpRequest manually clone the JS function object if it's passed one as an nsIDOMEventListener?  This would still leave other places that take nsIDOMEventListener with issues, but....
Comment 16 Peter Van der Beken [:peterv] 2006-03-10 13:22:44 PST
(In reply to comment #15)
> 1)  Indicate what the |this| for this function should be.
> 2)  Make it so?

See nsEventListenerThisTranslator in nsDOMClassInfo. We already map this to the event's target for nsIDOMEventListener afaik.
Comment 17 Boris Zbarsky [:bz] (still a bit busy) 2006-03-10 13:42:50 PST
> See nsEventListenerThisTranslator in nsDOMClassInfo

Ah..  So we don't actually use this in this case.  We end up in CallMethod, our paramCount is 0, so we never reach the code at http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/src/xpcwrappedjsclass.cpp#1106

It looks like I just missed something kinda important -- unlike onload (which works now, btw), onreadystatechange is not in fact an nsIDOMEventListener.  See http://lxr.mozilla.org/seamonkey/source/extensions/xmlextras/base/public/nsIXMLHttpRequest.idl#288

In particular, it does not get an event object passed to it.  And since the HandleEvent() method takes no args, the translator setup doesn't work.

Perhaps HandleEvent() should simply take the XMLHttpRequest object as a single arg?  And then we add a translator to "translate" it to a value for |this|?
Comment 18 Hixie (not reading bugmail) 2006-03-10 13:58:23 PST
BTW apparently this is already resolved for the DOM3 spec:
   http://lists.w3.org/Archives/Public/public-webapi/2006Mar/0101.html
Comment 19 Peter Van der Beken [:peterv] 2006-03-10 14:00:36 PST
(In reply to comment #17)
> Perhaps HandleEvent() should simply take the XMLHttpRequest object as a single
> arg?  And then we add a translator to "translate" it to a value for |this|?

Sounds good to me.
Comment 20 Boris Zbarsky [:bz] (still a bit busy) 2006-03-10 14:04:54 PST
> BTW apparently this is already resolved for the DOM3 spec:

Except that onreadystatechange is not a DOM event handler in our impl (why not?) so that the DOM3 spec is not really relevant here.  :(

Someone want to code up comment 17?  ;)
Comment 21 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-03-10 14:21:21 PST
In the first version of the xmlhttprequest spec onreadystatechange isn't going to be an eventhandler either, for compat with IE. However we were going to define |this| to be bound to the XMLHttpRequest object so that we in the next version of the spec could make it an eventhandler.
Comment 22 Boris Zbarsky [:bz] (still a bit busy) 2006-03-12 16:03:18 PST
Mmmm... So how do we handle this, then?  If we change our onreadystatechange thing to get the nsIXMLHttpRequest as the first arg, then when we switch it to take an event instead we'll be incompatible.

Should we just go ahead and make onreadystatechange an EventHandler in Gecko?  We'd need to make up an event name or something, but....
Comment 23 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-03-13 02:52:34 PST
(In reply to comment #22)
> Mmmm... So how do we handle this, then?  If we change our onreadystatechange
> thing to get the nsIXMLHttpRequest as the first arg, then when we switch it to
> take an event instead we'll be incompatible.

The spec will not define the XHR to be the first arg. It will define XHR to be the |this| object. In the first version of the spec the function will get no arguments at all.

> Should we just go ahead and make onreadystatechange an EventHandler in Gecko? 
> We'd need to make up an event name or something, but....

That should be compatible with both the first and the second version of the spec, so if that is easy it seems like the ideal solution.
Comment 24 Boris Zbarsky [:bz] (still a bit busy) 2006-03-13 08:38:01 PST
> That should be compatible with both the first and the second version of the
> spec

It's not quite compatible with the first version (since the function _does_ get an argument), and it's not really compatible with the second unless we use the same event name or something... :(
Comment 25 Boris Zbarsky [:bz] (still a bit busy) 2006-03-13 08:38:43 PST
But yes, apart from the question of "what event?", making onreadystatechange be an EventHandler is pretty easy.
Comment 26 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-03-13 18:31:18 PST
The fact that the function gets an extra argument isn't something that should cause any problems i would think. In theory someone could be checking if an extra argument is provided and do different things depending on if it is or not, but that seems really unlikly.

However, it is a good point about the event name. I don't think the WG will want to commit to a name at this time, but we can always lead the way and hope that the WG adopts our name. That would seem likely if we use good name. 'readystatechange' would seem like the obvious name.
Comment 27 Boris Zbarsky [:bz] (still a bit busy) 2006-03-13 22:08:36 PST
Created attachment 214975 [details] [diff] [review]
Fix

This does it.  I cleaned up a tad while I was here -- the existing code hurt my eyes.  Still does in some ways, but I tried to keep the changes small.
Comment 28 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-03-20 16:48:55 PST
Comment on attachment 214975 [details] [diff] [review]
Fix

>Index: extensions/xmlextras/base/src/nsXMLHttpRequest.cpp

>+#define LOAD_STR "load"
>+#define ERROR_STR "error"
>+#define PROGRESS_STR "progress"
>+#define READYSTATE_STR "readystatechange"

Do we really need these defines? I'd just nuke them.


>+nsXMLHttpRequest::CreateDocument(nsIDOMDocument** aDocument)
>+    // XXXbz this is probably all wrong when not called from JS... and possibly
>+    // even then! Fixing that requires giving XMLHttpRequest some principals
>+    // when inited.  Until then, cases when we don't actually parse the
>+    // document will give our mDocument he wrong principal.  I'm just not sure
>+    // how wrong it can get...  Shouldn't be too bad as long as mScriptContext
>+    // is sane, I guess.
>+    nsCOMPtr<nsIDocument> doc = GetDocumentFromScriptContext(mScriptContext);
>+    nsIURI* uri = GetBaseURI();
>+    nsIPrincipal* principal = nsnull;
>+    if (doc) {
>+      principal = doc->GetNodePrincipal();
>+    }
>+    privImpl->Init(uri, uri, principal);
>+  }

This makes me very nervous. When do we need to do this? Could we get away with bailing of |doc| is false. Remember that GetBaseURI can return a uri that points to any server.
Comment 29 Boris Zbarsky [:bz] (still a bit busy) 2006-03-22 13:19:16 PST
> Do we really need these defines? I'd just nuke them.

I'd rather not duplicate those strings in 2-3 places; this makes sure there's no way they can be mistyped, etc.

> This makes me very nervous. When do we need to do this?

Which?  Create a document?  We need it to send the initial progress message, and to create the document we parse our stuff into when we get the response.

Note that I didn't change any of the logic in the latter case.

> Could we get away with bailing of |doc| is false.

If we want to declare that XMLHttpRequest is only usable from JS, sure.

> Remember that GetBaseURI can return a uri that points to any server.

Yeah, well.  I do think we should make the security stuff for XMLHttpRequest suck less; all I tried to do in this patch is not change it.
Comment 30 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-03-22 16:00:22 PST
(In reply to comment #29)
> > Do we really need these defines? I'd just nuke them.
> 
> I'd rather not duplicate those strings in 2-3 places; this makes sure there's
> no way they can be mistyped, etc.

But now you're retyping an even longer string (the name of the define). I guess it protects against mistyping though so if you want to keep it i'm fine with that.

> > This makes me very nervous. When do we need to do this?
> 
> Which?  Create a document?  We need it to send the initial progress message,
> and to create the document we parse our stuff into when we get the response.



> Note that I didn't change any of the logic in the latter case.

You added a new call to CreateDocument in nsXMLHttpRequest::CreateEvent so you're adding more callers to the potentially scary code. Could we in that code rather then creating a documet create an eventmanager?

> > Remember that GetBaseURI can return a uri that points to any server.
> 
> Yeah, well.  I do think we should make the security stuff for XMLHttpRequest
> suck less; all I tried to do in this patch is not change it.

could you change that to use document->GetDocumentURI instead? No need to keep scary code around longer then needed.
Comment 31 Boris Zbarsky [:bz] (still a bit busy) 2006-03-22 19:33:12 PST
> Could we in that code rather then creating a documet create an eventmanager?

No idea.  All I'm using the document for is a CreateEvent call; I don't believe we expose any other api for that other than nsIDOMDocument to non-layout code.  And given that that's all I'm using it for, it doesn't seem that scary....

> could you change that to use document->GetDocumentURI instead?

Which part?  We still need the base URI in some places in this code... do you just mean that CreateDocument() should use the "right" document URI?

Frankly, I would rather have a separate bug to make the security stuff in XMLHttpRequest sane; right now it's a mess any time it's used from C++.
Comment 32 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-04-03 18:06:46 PDT
Comment on attachment 214975 [details] [diff] [review]
Fix

r=me, but file a bug on using the right uri
Comment 33 Boris Zbarsky [:bz] (still a bit busy) 2006-04-30 17:27:03 PDT
Created attachment 220346 [details] [diff] [review]
Merged to tip

sicking, could you give this a once-over?  This makes us send upload progress events separately (bug in the original patch), and merges the event creation to the better option I have on tip.

I'm still talking to the WG about the final form this will take, but I'd really like to get this out of my tree for now.
Comment 34 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-05-03 02:13:08 PDT
Comment on attachment 220346 [details] [diff] [review]
Merged to tip

Looks good, though there seems to be little need to break out CreateDocument into its own function now that there is only one callsite. Might be good to revert that since that function doesn't seem like something we desire a lot of users of.

r=me either way
Comment 35 Boris Zbarsky [:bz] (still a bit busy) 2006-05-05 10:04:40 PDT
Fixed.
Comment 36 Boris Zbarsky [:bz] (still a bit busy) 2006-05-05 10:16:53 PDT
Created attachment 220951 [details] [diff] [review]
Patch that I checked in
Comment 37 Steve England [:stevee] 2006-05-06 06:38:22 PDT
Could this have caused bug 336877?
Comment 38 Peter Van der Beken [:peterv] 2006-07-13 10:49:57 PDT
*** Bug 341610 has been marked as a duplicate of this bug. ***
Comment 39 Eric Shepherd [:sheppy] 2007-03-06 08:31:44 PST
OK, looking through the code and reading hte comments, it looks like the developer-facing API changes are the addition of an onuploadprogress attribute and the change from nsIOnReadyStateChangeHandler to nsIDOMEventListener for the type of the onreadystatechange attribute.

However, I can't tell easily looking at the code what exactly has been changed in terms of what "this" will be.  Can someone give me a quick summary of the final state, or at least point me to the specific code that sets these values, as the lines that seem like they're responsible don't seem to have changed.
Comment 40 Boris Zbarsky [:bz] (still a bit busy) 2007-03-06 08:58:36 PST
> I can't tell easily looking at the code what exactly has been changed
> in terms of what "this" will be. 

In the onreadystatechanged handler it changed from whatever it used to be (the function itself?  Not sure) to the XMLHttpRequest object.
Comment 41 Hixie (not reading bugmail) 2007-03-06 12:13:23 PST
Note that the WebAPI WG recently decided to change the way upload progress is tracked to be more flexible for authors. This directly affects what the target of progress events are for XHR. See the WebAPI list for details.
Comment 42 Boris Zbarsky [:bz] (still a bit busy) 2007-03-06 13:17:29 PST
Yeah.  Like I keep saying, once that spec stabilizes we'll need to rewrite a good bit of this file.  It's worth only doing that once, though, hence the "once that spec stabilizes".
Comment 43 Eric Shepherd [:sheppy] 2007-03-06 13:53:29 PST
So should I be looking to update our documentation now, or wait until things settle down?  I don't know whether this change is already shipped, and if not, when it's expected to.
Comment 44 Boris Zbarsky [:bz] (still a bit busy) 2007-03-06 13:56:11 PST
The onreadystatechange parts of this change (which are the parts that changed event.target for those events) are not expected to change anymore.

This change is going to ship in Gecko 1.9.
Comment 45 Eric Shepherd [:sheppy] 2007-10-09 15:31:55 PDT
This change is now documented.

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