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.

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

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.

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

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.

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?

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.

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

For the latter definition, reference Appendix C of DOM Level 2 Events.

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

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.

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

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

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

BTW apparently this is already resolved for the DOM3 spec: http://lists.w3.org/Archives/Public/public-webapi/2006Mar/0101.html

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

> 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? ;)

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.

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

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

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

But yes, apart from the question of "what event?", making onreadystatechange be an EventHandler is pretty easy.

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.

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

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

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

> 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 on attachment 214975 [details] [diff] [review] Fix r=me, but file a bug on using the right uri

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

Fixed.

Could this have caused bug 336877?

*** Bug 341610 has been marked as a duplicate of this bug. ***

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.

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

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.

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

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.

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.

This change is now documented.