Closed Bug 198595 Opened 21 years ago Closed 18 years ago

event.target, event.currentTarget and the this object are not set consistently in onload handlers of XMLHttpRequest and XMLDocument

Categories

(Core :: XML, defect)

x86
Windows 98
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: martin.honnen, Assigned: bzbarsky)

References

Details

(Keywords: dev-doc-complete)

Attachments

(5 files, 2 obsolete files)

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.
Depends on: 295340
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.
Attached patch Fix (obsolete) — Splinter Review
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.
Attachment #214975 - Flags: superreview?(peterv)
Attachment #214975 - Flags: review?(bugmail)
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++.
Depends on: 332528
Comment on attachment 214975 [details] [diff] [review]
Fix

r=me, but file a bug on using the right uri
Attachment #214975 - Flags: review?(bugmail) → review+
Blocks: 310602
Depends on: 334368
Depends on: 324865
Blocks: 312213
Attachment #214975 - Flags: superreview?(peterv) → superreview+
Attached patch Merged to tip (obsolete) — Splinter Review
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.
Attachment #220346 - Flags: review?(bugmail)
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
Attachment #220346 - Flags: review?(bugmail) → review+
Assignee: hjtoi-bugzilla → bzbarsky
Fixed.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Attachment #214975 - Attachment is obsolete: true
Attachment #220346 - Attachment is obsolete: true
Could this have caused bug 336877?
Depends on: 337374
*** Bug 341610 has been marked as a duplicate of this bug. ***
Keywords: dev-doc-needed
Depends on: 361811
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.
Blocks: 290127
This change is now documented.
Depends on: 437651
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: