96459 - Need progress indicators for XMLHttpRequest

Status: VERIFIED FIXED

(Core :: XML, defect, P2)

Description

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Hopefully "onreadystatechange" and "readyState" members would be enough, as per
the MS IDL.

Comment 1

[Heikki Toivonen (remove -bugzilla when emailing directly)]

DIGBug

Comment 2

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: readyState and some other improvements

Comment 3

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Hmm... I am not sure how to implement "onreadystatechange" at this point... On
the other hand, I am not sure if that is absolutely required. My limited testing
implies that with IE the function that has been set will only be called once for
INTERACTIVE data, which would seem to indicate that you'd need to set up a
timeout to see if responseText grows. I originally thought INTERACTIVE message
would be gotten several times (once for each chunk of data read from network).
It could be that my testcase never hit a large enough chunk or something (tried
with about 26kB).

The above patch fixes these things:
* Implement readyState member, and allow reading of responseText when readyState
is INTERACTIVE or COMPLETED.
* Throw error if setRequestHeader() is called before open() (same as IE).
* Throw error if send() is called 2nd time without open() (same as IE).

I think this would be the absolute minimum to get going, when used together with
a timeout to check readyState and responseText occasionally.

Greg, would this be sufficient for your purposes? Are some of my
assumptions/observations about IE incorrect?

Comment 4

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Additionally onreadystatechange, testcases and SOAP update

Comment 5

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Ok, the latest patch also implements onreadystatechange member. It seems to work
in my testcases. I have no idea if it would be a security risk the way it is
implemented. I have no readystatechange event, I just do some tricks to call the
callback function directly. For onload and onerror I go through the event
system. Thanks to vidur for this trick. 

Mitch, could you take a look at the patch, is it safe?

I don't know if I should fire onreadystatechange with readyState==3
(INTERACTIVE) only once, or for each chunk of data I am reading. With the patch
it is fired for each chunk, but maybe IE is not doing it (so I should change to
fire it only once)? Greg, anyone?

The patch also includes some testcase updates and fix SOAP so that it compiles
(patch for SOAP from peterv).

Comment 6

[Greg White]

I don't know whar 'each chunk' means, but would like to be called more than once 
if there is a long download with a lot of data.  I don't know how IE is doing it 
now.

Comment 7

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Necko is feeding my code with roughly 4kB 'chunks' of data, and I can fire
onreadystatechange callback for each ~4kB chunk. I know from my testing that IE
called the callback only once for my testcase where the data was about 26kB. It
could be that IE has a 'chunk'/buffer size bigger than that or uses some smart
buffer size and only gave me one when it saw I had fast connection. Or it could
be that IE calls the callback only once, when it starts seeing some data from
the network (besides headers).

Based on your wishes I can leave that part of the code as is, so you will get a
callback for every ~4kB of data. You may still need to do a timer based polling
function for IE, though. Or you could use the same timer polling for both IE and
Mozilla-based browsers.
Do you need this fix for the 0.9.2 branch or would trunk/0.9.4 be enough? If you
need it for 0.9.2, please add topembed keyword.

Comment 8

[Greg White]

Well, our group probably won't change our current code to use the new API, so I 
don't think it would be a good idea for me to set the topembed keyword.  Thanks 
for adding it for the future though!

Comment 9

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Proposed fix 1

Comment 10

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Allrighty! Here is now a proposed fix. 

I noticed one additional problem that is now fixed: in case of an XML parser
error, we were firing the load event too soon. With the fix we store the load
event we get in Load(), and act on it in OnStopRequest() when we can be certain
there is no more data coming in. I noticed the problem with the non-well-formed
XML when playing around with the OASIS XML test suite (Mozilla version by Bob
Clary) at http://green/heikki/oasis/bug57673.html

As it happens, this fix also fixes DIG crasher bug 96307 (aka bugscape 8288).

Comment 11

[harishd]

I'm not sure what's the purpose of JSContextStack. I don't see it being used
anywhere in the function. 

+  nsCOMPtr<nsIJSContextStack> stack;
+  JSContext *cx = nsnull;
+
+  if (mScriptContext) {
+    stack = do_GetService("@mozilla.org/js/xpc/ContextStack;1");
........

Typos:
1) + * compare to Internet Eplorer:
2) +   *   0 UNITIALIZED open() has not been called yet.

Other than that r=harishd

Comment 12

[vidur (gone)]

Pushing and popping the JS context that corresponds to the event handlers is a
necessary part of invoking them. The context on the top of the JS context stack
is used by XPConnect for JS invocation. The context is also used by the security
manager to determine subject principals. In fact, the same should be done when
invoking the onreadystatechanged handler. sr=vidur after onreadystatechanged
invocation is fixed.

Comment 13

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Fixed typos and added the content stack pushes as vidur suggested

Comment 14

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Fixed.

Comment 15

[Chris Petersen]

Since I 'm not exactly sure how to test this, I'm marking verified based on the
last comments