372964 - Make XMLHttpRequest to work more like a normal event target

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P3)

Description

[Olli Pettay [:smaug][[e-mail]]]

Attachment: WIP

The patch is just a idea what could be done.
It is a bit scary, especially because with it event listeners aren't
removed automatically all the time like they are currently.
Though, GMail and GCalendar etc seem to work ok.
I'm not quite sure what we want to do with mScriptContext.
Currently the handling of it is a mess, IMO, but in the patch it is 
updated only when XHR is created.

Uploading the patch so that I don't lose it.

Comment 1

[Boris Zbarsky [:bzbarsky]]

mScriptContext will more or less need to go away anyway once we implement the W3C XMLHttpRequest spec -- we'll want to hold a pointer to a window at that point.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: WIP2

Some ideas to remove mScriptContext.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: WIP3

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

bz, or anyone, any comments on this one?

(I've been using the patch and the patch for 373998 for few days now.
 So far everything seems to work ok; gmail, gmaps etc.)

Comment 5

[Boris Zbarsky [:bzbarsky]]

I'm not going to be in a position to look at this in the foreseeable future... (weeks, probably).

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: WIP4 - up-to-date-with-trunk

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: WIP5

Do you have any comments on the nsIJSNativeInitializer.h/nsDOMClassInfo.cpp changes?
According to the current spec there must be the pointer from xhr to 
the window and that simplifies scriptcontext mess.

One thing about the events, the patch changes the name of the uploadprogress events to be really "uploadprogress", not "progress".
Will have to check WebAPI WG what the final event name / API will be.

Comment 8

[Nickolay_Ponomarev]

I think this will fix bug 295422, since window.close relies on termination functions being called, and current nsXMLHttpRequest::NotifyEventListeners implementation doesn't do that.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Adds support for 4th parameter of addEventListener + tests

The patch depends on bug 435656.
nsEventTargetSH will be useful with xhr.upload.addEventListener too.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

I'm not quite sure what to do with ClearEventListeners.
It should be removed, but that might break something.
The patch changes few cases when ClearEventListeners is called.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 322610 [details] [diff] [review]
Adds support for 4th parameter of addEventListener + tests

I think I found a solution to have even better backward compatibility.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: trying to keep the old ClearEventListeners behavior

I'll make still a different patch without the whole clear-event-listeners-mess.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Attachment: without ClearEventListeners

Comment 14

[Boris Zbarsky [:bzbarsky]]

Most of the listener-clearing was there to avoid leaks.  That's less of an issue now.  So we could try only clearing what the xhr spec says to clear.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Yes, I think so too. If there are regressions found (during 1.9.1pre), clearing 
event listeners could be added back. Currently XHR1&2 don't seem to specify any 
clearing.
[14:25]	<smaug>	anne: have you tested IE and event listeners?
[14:25]	<smaug>	afaik it clears XHR event listeners after "completition"
[14:25]	<smaug>	and gecko has had to follow that
[14:26]	<smaug>	the spec doesn't say anything about it
[14:26]	<anne>	we had that but removed it because the IE team acknowledged it was a bug

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Note, this may break progress events which will be fixed and modified a bit in the patch for bug 435425.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

wanted1.9.1? because this blocks wanted1.9.1+ bug 435425.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm a little scared about making a functional change that could break webpages at the same time as a cleanup patch, but since it aligns us with the spec, hopefully few pages will break...

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 322640 [details] [diff] [review]
without ClearEventListeners

What is the purpose of the nsDOMEventListenerWrapper business? Why not store the direct pointers to the listeners in the XHR members instead? The only effect seems to be that you can add the same listener twice, once through .onX and once through AddEventListener. Is this defined by spec? Otherwise this seems like undesired behavior?

> nsXMLHttpRequest::SetOnreadystatechange(nsIDOMEventListener * aOnreadystatechange)
> {
>-  mOnReadystatechangeListener = aOnreadystatechange;
>-  return NS_OK;
>+  NS_NAMED_LITERAL_STRING(rs, READYSTATE_STR);
>+  return RemoveAddEventListener(rs, mOnReadystatechangeListener,
>+                                aOnreadystatechange);
> }

Why not just use NS_LITERAL_STRING inline? Same in the other setters.

>+nsXMLHttpRequest::GetListenerManager(PRBool aCreateIfNotFound,
>+                                     nsIEventListenerManager** aResult)
>+{
>+  if (!mListenerManager) {
>+    if (!aCreateIfNotFound) {
>+      *aResult = nsnull;
>+      return NS_OK;
>+    }
>+    nsresult rv = NS_NewEventListenerManager(getter_AddRefs(mListenerManager));
>+    NS_ENSURE_SUCCESS(rv, rv);

This can return with out setting *aResult if things fail here, no?

I'd just set *aResult up front to nsnull;


Looks great otherwise!

The only thing that I'm worried about is would this make it harder for us to use the XHR object off the main thread when doing worker threads? All objects would still just be used on a single thread, but that thread wouldn't always be the main thread.

I suspect that we'd still want this cleanup though, and we could make whatever changes were needed to the ELM code to make it able to live off the main thread.

Want to understand the wrapper stuff before marking r+

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #19)
> (From update of attachment 322640 [details] [diff] [review])
> What is the purpose of the nsDOMEventListenerWrapper business? Why not store
> the direct pointers to the listeners in the XHR members instead? The only
> effect seems to be that you can add the same listener twice, once through .onX
> and once through AddEventListener. Is this defined by spec? Otherwise this
> seems like undesired behavior?
The main reasons are backward compatibility and to be able to remove 
.onfoo listeners by setting a new value to .onfoo.
If the wrapper was added to ELM but XHR would keep pointer to the original
listener, then removing the .onfoo listener wouldn't be possible, since the
pointer to the wrapper is lost somewhere in ELM's internal data structures.

Why would this be undesired behavior?

> Why not just use NS_LITERAL_STRING inline? Same in the other setters.
There used to be some reason :) Left-overs from some version of the patch.

> This can return with out setting *aResult if things fail here, no?
True. Especially because NS_NewEventListenerManager doesn't set its
out param to null.

> The only thing that I'm worried about is would this make it harder for us to
> use the XHR object off the main thread when doing worker threads? All objects
> would still just be used on a single thread, but that thread wouldn't always be
> the main thread.
AFAIS the additional objects for cycle collector are the only parts which can 
make things harder for worker threads. But that is an issue even before this 
patch - DOM/CC is currently for main thread.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #20)
> (In reply to comment #19)
> > (From update of attachment 322640 [details] [diff] [review] [details])
> > What is the purpose of the nsDOMEventListenerWrapper business? Why not store
> > the direct pointers to the listeners in the XHR members instead? The only
> > effect seems to be that you can add the same listener twice, once through .onX
> > and once through AddEventListener. Is this defined by spec? Otherwise this
> > seems like undesired behavior?
> The main reasons are backward compatibility and to be able to remove 
> .onfoo listeners by setting a new value to .onfoo.
> If the wrapper was added to ELM but XHR would keep pointer to the original
> listener, then removing the .onfoo listener wouldn't be possible, since the
> pointer to the wrapper is lost somewhere in ELM's internal data structures.
To clarify this some more, using wrappers .removeEventListener doesn't work
if listener is added using onfoo. That is how XHR works currently and that is
how also other DOM objects work

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Attachment: with nits fixed

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 329075 [details] [diff] [review]
with nits fixed

I think I addressed Jonas' comments.

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to comment #20)
> (In reply to comment #19)
> > (From update of attachment 322640 [details] [diff] [review] [details])
> > What is the purpose of the nsDOMEventListenerWrapper business? Why not store
> > the direct pointers to the listeners in the XHR members instead? The only
> > effect seems to be that you can add the same listener twice, once through .onX
> > and once through AddEventListener. Is this defined by spec? Otherwise this
> > seems like undesired behavior?
> The main reasons are backward compatibility and to be able to remove 
> .onfoo listeners by setting a new value to .onfoo.

That would still work, no? I.e. if you just put the original listener in the member as well as added it to the ELM, then you could still remove the listener by setting .onfoo = null.

> If the wrapper was added to ELM but XHR would keep pointer to the original
> listener, then removing the .onfoo listener wouldn't be possible, since the
> pointer to the wrapper is lost somewhere in ELM's internal data structures.

Sorry, I was unclear. What I propose is that we keep a pointer to the real listener in XHR, as well as add the real listener to the ELM. I.e. kill the whole wrapper class.

> To clarify this some more, using wrappers .removeEventListener doesn't work
> if listener is added using onfoo. That is how XHR works currently and that is
> how also other DOM objects work

Ah, this is indeed true. I.e. if we didn't have wrappers someone could do:

xhr.onfoo = mylistener;
xhr.removeEventListener(mylistener, ...);

which would result in no listener being registered. I don't really feel strongly about if this is desired or not. I think it would be totally ok if the above removed the listener, as long .onfoo got cleared too.

But I guess we can keep the wrappers for now, if nothing else for consistency with nodes .onfoo behavior.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

jst? This is blocking rest of the progress event stuff.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Attachment: additional fix

bah, I should have remembered this. The old code has inner window check
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/base/src/nsXMLHttpRequest.cpp&rev=1.241&mark=1008#997

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 333934 [details] [diff] [review]
additional fix

Tue Aug 19 16:02:02 2008 +0300 (at Tue Aug 19 16:02:02 2008 +0300)
changeset 17034	81c9a977f5e3

Comment 28

[Henri Sivonen (:hsivonen)]

Why does test_bug372964-2.html expect the first call to document.open() to blow away the old inner window? I'm trying to understand why my patch for bug 543435 broke things here. I suspect it has something to do with the generated about:blank doc getting doc->SetIsInitialDocument(PR_TRUE); called on it.

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Because that is what has at least traditionally happened;
document.open() after load event blows away the old inner window.

I thought HTML spec even specified this
(but I didn't verify that now - need to run to a meeting).

Comment 30

[Henri Sivonen (:hsivonen)]

Thanks. I guess I've misunderstood the semantics of IsInitialDocument() and the initial about:blank should stop claiming to be initial after load in order to have this stuff work.

Comment 31

[Boris Zbarsky [:bzbarsky]]

IsInitialDocument is the first document ever loaded in the window, and in particular one whose inner window might need to be persisted across document loads.  This used to be about:blank in some cases...  but with your changes we might have more than one initial document, depending on how they are done.  :(