Bug 11610 – XMLHttpRequest.onreadystatechange doesn't provide access to the request object

• Status: RESOLVED
• Resolution: FIXED
• Component: XML

[Jamie Pate]

when an XMLHttpRequest object calls the onreadystatechange 'event' or 'callback' function that is assigned to it, the 'event' object contains no reference to the calling object (event.type == readystatechange) 
event.srcElement should be the calling XMLHttpRequest 
(or some other method should be made available to the function being called)

[Alexey Proskuryakov]

Testing with IE6, I'm seeing that Safari works more or less like it - in the handler, "this" points to the global object, and event.srcElement is also unavailable.

Is this something that changed in IE7?

[Jamie Pate]

no, but IE7 references the 'this' object as the XMLHttpRequest object that is calling. Safari has no way to reference the calling XMLHttpRequest object (this refers to window). I was just suggesting what I think would be the 'proper' and cleanest way to do it since the event object type is already 'readystatechange'

[Jamie Pate]

(In reply to comment #2)
> no, but IE7 references the 'this' object as the XMLHttpRequest object that is
> calling. Safari has no way to reference the calling XMLHttpRequest object (this
> refers to window). I was just suggesting what I think would be the 'proper' and
> cleanest way to do it since the event object type is already 'readystatechange'
> 
sorry, upon further testing IE6 also has the same problem as safari (in that you can't get a hold of the XMLHttpRequest object from inside the onreadystatechange callback)
At least IE6 will be mass-replaced soon with IE7, (and incidentally my target audience is a controlled group so I don't have to support IE6 at all)

Either way, a reference to the XMLHttpRequest object that isn't global is all I'm looking for. Executing in the context of the XMLHttpRequest object (making 'this' a reference to said object) would be equally acceptable, but may break backwards compatibility if someone relies on it (illogical, but possible) 

[Alexey Proskuryakov]

Interestingly, the XHR draft spec doesn't seem to cover this issue, although it uses "this" in a non-normative example. Does this somehow follow from other requirements?

This may be the single most dangerous change that needs to be made in our XHR implementation, so CC'ing Geoff, too.

[Jamie Pate]

(In reply to comment #4)
> Interestingly, the XHR draft spec doesn't seem to cover this issue, although it
> uses "this" in a non-normative example. Does this somehow follow from other
> requirements?
> 
> This may be the single most dangerous change that needs to be made in our XHR
> implementation, so CC'ing Geoff, too.
> 
It may be covered under the 
'Objects implementing the XMLHttpRequest interface must also implement the EventTarget interface [DOM3Events].' bit, which would make the Event.target == the calling XMLHttpRequest object. (from what i've skimmed out of the spec)

[Alexey Proskuryakov]

(In reply to comment #5)
> It may be covered under the 
> 'Objects implementing the XMLHttpRequest interface must also implement the
> EventTarget interface [DOM3Events].' bit, which would make the Event.target ==
> the calling XMLHttpRequest object. (from what i've skimmed out of the spec)

Yes, Anne van Kesteren has commented on IRC that this is indeed the case. Currently, "this" works in Opera, and will be fixed in Firefox, too. Also, looks like this hasn't caused compatibility problems for Opera.

[Alexey Proskuryakov]

Created attachment 11838 [details]
proposed fix

First attempt.

[Darin Adler]

Comment on attachment 11838 [details]
proposed fix

I don't like the fact that adding any new classes derived from EventTarget will require changes to the toJS function in kjs_dom.cpp. Is there any way we can do better?

Also, is there a way we can avoid adding code to kjs_dom.cpp -- where's the newfangled place for code like this?

Can someone rename intenalHeaderContent to internalHeaderContent?

As for all the functions in EventTarget that call isNode, why not use virtual functions for that?

If you don't want EventTarget's ref() and deref() themselves to be virtual you can have them be inlines that call pure virtual functions. I think if you design EventTarget properly you won't have to have any code except for the JavaScript binding that uses isNode().

I haven't yet given up hope of sharing some of the event dispatch code, but that can wait.

Since toNode returns 0 if it's not a node, I think functions that call both isNode and toNode should perhaps be changed to just call toNode and check for nil.

+        if (!element || (evt->target() && element->contains(evt->target()->toNode())))

Why this new check for a nil target? When can that happen?

+        Event* evt = new Event(readystatechangeEvent, true, true);

This should be a RefPtr, and you should call release when calling handleEvent. It's not good to rely on the fact that setTarget and setCurrentTarget won't ref/deref. This idiom occurs two more times a few lines later.

+    using Shared<XMLHttpRequest>::ref;
+    using Shared<XMLHttpRequest>::deref;

I believe you can just say using Shared::ref here.

I'm going to say review+ here, but feel free to clear the review flag if you want a review of a new version of the patch.

[Alexey Proskuryakov]

Created attachment 11865 [details]
proposed fix

(In reply to comment #8)
> I don't like the fact that adding any new classes derived from EventTarget will
> require changes to the toJS function in kjs_dom.cpp. Is there any way we can do
> better?

  I have made it more symmetrical by adding a toXMLHttpRequest() method to EventTarget. I don't see how it could automatically support new subclasses - and other toJS functions (for Document, Node, etc.) have same or similar design.

> Also, is there a way we can avoid adding code to kjs_dom.cpp -- where's the
> newfangled place for code like this?

  All other toJS() functions are also in kjs_* files now, except for a single static one in JSCanvasRenderingContext2DCustom.cpp.

> Can someone rename intenalHeaderContent to internalHeaderContent?

  OK.

> As for all the functions in EventTarget that call isNode, why not use virtual
> functions for that?
> 
> If you don't want EventTarget's ref() and deref() themselves to be virtual you
> can have them be inlines that call pure virtual functions. I think if you
> design EventTarget properly you won't have to have any code except for the
> JavaScript binding that uses isNode().

  I have actually removed isNode() in favor of directly calling toNode()/toXMLHttpRequest().

  Implemented ref()/deref() as discussed on IRC, but actually defined them in the header, even though they won't be inlined. I think that this will improve readability, since the design is not particularly obvious.

> I haven't yet given up hope of sharing some of the event dispatch code, but
> that can wait.
> 
> Since toNode returns 0 if it's not a node, I think functions that call both
> isNode and toNode should perhaps be changed to just call toNode and check for
> nil.

  Done (removed isNode() altogether).

> +        if (!element || (evt->target() &&
> element->contains(evt->target()->toNode())))
> 
> Why this new check for a nil target? When can that happen?

  The check used to be in HTMLElement::contains(); I don't know if this can happen, but I wanted to preserve the semantics intact.

> +        Event* evt = new Event(readystatechangeEvent, true, true);
> 
> This should be a RefPtr, and you should call release when calling handleEvent.
> It's not good to rely on the fact that setTarget and setCurrentTarget won't
> ref/deref. This idiom occurs two more times a few lines later.

  Done (but with get() instead of release() - handleEvent() doesn't take ownership AFAICT).

> +    using Shared<XMLHttpRequest>::ref;
> +    using Shared<XMLHttpRequest>::deref;
> 
> I believe you can just say using Shared::ref here.

  I think this shortcut can only be used for template parameters of the class itself, not for its superclasses. I tried, but it didn't work.

[Darin Adler]

Comment on attachment 11865 [details]
proposed fix

+#include "config.h"
+#include "EventTarget.h"
+#include "xmlhttprequest.h"
+
+#include "EventTargetNode.h"

Space should be after EventTarget.h.

refEventTarget and derefEventTarget could be private instead of protected. Derived classes need to override them, but that can be done regardless of private vs. protected. And derived classes do not need to call them. So private would be slightly better.

r=me

[Darin Adler]

Comment on attachment 11865 [details]
proposed fix

That was r=me, so review+, not -!

[Alexey Proskuryakov]

Committed revision 18238.