Bug 16989 – Add send() flag checks in XmlHttpRequest

• Component: XML
• Status: RESOLVED FIXED

[Julien Chaffraix]

As mentioned in bug 13141, we should implement the send() flag algorithm given in the XHR specification:
http://www.w3.org/TR/XMLHttpRequest/

[Julien Chaffraix]

Created attachment 18632 [details]
Implementation + test case

[Alexey Proskuryakov]

Comment on attachment 18632 [details]
Implementation + test case

The send flag that the spec talks about was supposed to be m_loader being non-null in our implementation.

Is it possible to match the behavior in the spec without introducing m_sendFlag?

[Julien Chaffraix]

Created attachment 18633 [details]
Patch using m_loader as ap pointed out

(In reply to comment #2)
> (From update of attachment 18632 [details] [edit])
> The send flag that the spec talks about was supposed to be m_loader being
> non-null in our implementation.
> 
> Is it possible to match the behavior in the spec without introducing
> m_sendFlag?
> 

Indeed ! The attached patch display the right behaviour without the m_sendFlag.

[Alexey Proskuryakov]

Comment on attachment 18633 [details]
Patch using m_loader as ap pointed out

+        bug 16989: XmlHttpRequest should use the send() flag algorithm

It probably makes sense to rename this bug to make it clear what was fixed, as the send flag already worked. A detailed per-function ChangeLog would also help tremendously, since these changes are relatively risky, and will be studied by others many times if regressions come up.

+    m_state = Uninitialized;
...
+    m_loader = 0;

It seems that these changes result from a direct translation of the spec, and are not needed. For example, open() already asserts that m_state == Uninitialized, because this is ensured by other methods. Similarly, I do not see how m_loader can be non-null here.

-    if (m_state != Open) {
+    if (m_state != Open || m_loader) {

There is a check (with a FIXME) for m_loader just below this line; it should be removed.

-void XMLHttpRequest::abort()
+void XMLHttpRequest::abortRequest()

It's not clear from the name what the difference between these functions is; this will undoubtedly confuse someone sooner or later. Perhaps, it would be more straightforward to add a boolean flag with a default value to differentiate the two cases.

I'm not sure if I fully understood all the changes without a detailed ChangeLog, but looks like there are still a few improvements  to be made to this patch; r- for now.

[Alexey Proskuryakov]

Comment on attachment 18633 [details]
Patch using m_loader as ap pointed out

Also, this change doesn't seem to be supported by layout tests:

 void XMLHttpRequest::setRequestHeader(const String& name, const String& value, ExceptionCode& ec)
 {
-    if (m_state != Open) {
+    if (m_state != Open || m_loader) {

Do Firefox and IE work as the spec says?

[Julien Chaffraix]

(In reply to comment #5)
> (From update of attachment 18633 [details] [edit])
> Also, this change doesn't seem to be supported by layout tests:
> 
>  void XMLHttpRequest::setRequestHeader(const String& name, const String& value,
> ExceptionCode& ec)
>  {
> -    if (m_state != Open) {
> +    if (m_state != Open || m_loader) {
>

to catch the new case, we would need to have called send() to set m_loader but should not have finished send() because it would trigger m_state != Open. I will try to produce a test case but I do not know if it is possible to catch it.

> Do Firefox and IE work as the spec says?
> 

No. Here are the results of my tests (I tested only the behaviour of abort as it is the easiest way of determining the support) :

- IE & Opera seem to always set readyState to 0 without dispatching a readyState event
- The latest Firefox has a more complex algorithm : it first set readyState to 4 and dispatch a readyState event. 
  * If a listener send a new XmlHttpRequest, it considers that the current one is done and leaves the readyState to 4.
  * Otherwise, it sets readyState to 0 without dispatching a readyState event.

[Julien Chaffraix]

Created attachment 18887 [details]
updated patch

> +        bug 16989: XmlHttpRequest should use the send() flag algorithm

> It probably makes sense to rename this bug to make it clear what was fixed, as
> the send flag already worked. A detailed per-function ChangeLog would also help
> tremendously, since these changes are relatively risky, and will be studied by
> others many times if regressions come up.

Changed the name for "Add send() flag checks in XmlHttpRequest" which is more explicit. I have also added a detailed ChangeLog

> +    m_state = Uninitialized;

That one should be kept as we do not update the readystate in abort (it is more visible in the new patch)
> ...
> +    m_loader = 0;

Replaced by an ASSERT(!m_loader)

> It seems that these changes result from a direct translation of the spec, and
> are not needed. For example, open() already asserts that m_state ==
> Uninitialized, because this is ensured by other methods. Similarly, I do not
> see how m_loader can be non-null here.

The m_state == Uninitialized assertion is a guard that was introduced to replace a changeState(Uninitialized) that was moved in abort (see bug 13141).

> There is a check (with a FIXME) for m_loader just below this line; it should be
> removed.

Removed the FIXME

> It's not clear from the name what the difference between these functions is;
> this will undoubtedly confuse someone sooner or later. Perhaps, it would be
> more straightforward to add a boolean flag with a default value to
> differentiate the two cases.

I have merged the 2 methods and added a boolean parameter 'isJavascripAbort' which triggers the abort behaviour as specified in the spec. The default value does not change readystate (which was the behaviour before bug 13141).

>  void XMLHttpRequest::setRequestHeader(const String& name, const String& value,
> ExceptionCode& ec)
>  {
> -    if (m_state != Open) {
> +    if (m_state != Open || m_loader) {
>
> to catch the new case, we would need to have called send() to set m_loader but
> should not have finished send() because it would trigger m_state != Open. I
> will try to produce a test case but I do not know if it is possible to catch
> it.

I could not produce a test case for that part : send() is done completely before continuing Javascript execution which trigger m_state != Open.

[Darin Adler]

Comment on attachment 18887 [details]
updated patch

This looks great to me. I have some suggestions on how to improve it.

Instead of adding the boolean parameter to abort, I suggest we create a private function with a name like cancel() or kill() or stop() and use abort() only for the true public XMLHttpRequest.abort. Then abort() can call the other function first, then do the work you put inside your isJavascriptAbort if statement.

The language is JavaScript, not Javascript.

+    // Do not check for m_loader as required by the spec as it cannot be non null

I don't understand this comment. Are you saying that we could assert m_loader is not 0? If so, then please add that assert. Another thing I'd ask is: "How can the spec require this?" Since m_loader is an internal variable the specification should have nothing to say about it directly; if it says anything it would be indirect and I'd like to know what it says. I think you need a better comment, and I want to more clearly understand why we don't need the if statement any more.

 void XMLHttpRequest::setRequestHeader(const String& name, const String& value, ExceptionCode& ec)
 {
-    if (m_state != Open) {
+    if (m_state != Open || m_loader) {

You say that this cannot be covered by test cases. If not, then what is the check for? Why is this change needed? Why can't we test it? We can easily construct a load that won't complete with our test HTTP server, I believe.

I'm going to say review- for now, but this looks really good to me.

[Darin Adler]

(In reply to comment #4)
> It's not clear from the name what the difference between these functions is;
> this will undoubtedly confuse someone sooner or later. Perhaps, it would be
> more straightforward to add a boolean flag with a default value to
> differentiate the two cases.

I feel guilty because my comment is exactly the opposite of Alexey's.

If you decide to keep the boolean parameter, then I suggest that the default be the JavaScript behavior.

But on the other hand, it seems that the lower level version of abort() is a private function and I'd be happier not exposing both. Sorry for the back and forth. You may want to hear what Alexey has to say too.

Again, my apologies for saying the opposite of what Alexey said.

[Alexey Proskuryakov]

My suggestion to use a boolean flag was due to the fact I didn't have a good suggestion for another function's name. I am not sure if cancel, kill or stop really convey the difference either.

[Darin Adler]

(In reply to comment #10)
> My suggestion to use a boolean flag was due to the fact I didn't have a good
> suggestion for another function's name. I am not sure if cancel, kill or stop
> really convey the difference either.

If we can't find a good name, I suggest internalAbort().

[Alexey Proskuryakov]

(In reply to comment #11)
> If we can't find a good name, I suggest internalAbort().

Thanks, that sounds better than a boolean flag to me.


+    // Do not check for m_loader as required by the spec as it cannot be non null
     if (m_state != Open) {
         ec = INVALID_STATE_ERR;
         return;
     }
-  
-    // FIXME: Should this abort or raise an exception instead if we already have a m_loader going?
-    if (m_loader)
-        return;

Why this doesn't break the send() flag algorithm? Calling send() twice in a row was being caught by the if (m_loader) check - and in the previous version of the patch, you changed the behavior from silently ignoring that to raising an exception, as required by the spec.

[Julien Chaffraix]

(In reply to comment #12)
> (In reply to comment #11)
> > If we can't find a good name, I suggest internalAbort().
> 
> Thanks, that sounds better than a boolean flag to me.
> 
> 
> +    // Do not check for m_loader as required by the spec as it cannot be non
> null
>      if (m_state != Open) {
>          ec = INVALID_STATE_ERR;
>          return;
>      }
> -  
> -    // FIXME: Should this abort or raise an exception instead if we already
> have a m_loader going?
> -    if (m_loader)
> -        return;
> 
> Why this doesn't break the send() flag algorithm? Calling send() twice in a row
> was being caught by the if (m_loader) check - and in the previous version of
> the patch, you changed the behavior from silently ignoring that to raising an
> exception, as required by the spec.
> 

I got confused by comment #4 where you stated that m_loader could not be non null. It is valid to do that check and I will add it again in the next patch.

[Julien Chaffraix]

Created attachment 18924 [details]
Updated patch with all the required testcases

> You say that this cannot be covered by test cases. If not, then what is the
> check for? Why is this change needed? Why can't we test it? We can easily
> construct a load that won't complete with our test HTTP server, I believe.

You are right, I was able to produce the missing test cases. I was not looking in the right direction, thanks for pointing me to it.

> If we can't find a good name, I suggest internalAbort().

I have picked internalAbort() as other names do not fit or could be misleading (we already have cancelRequests for example).

There was a big misinterpretation of the specification in the previous iterations which is corrected in that patch (abort was not doing step 3 & 4 correctly).

I have been able to produce all the required test cases (I hope) and trying them on the other browser does provide more information about the current implementation :

- Opera does not have the send() flag checks in setRequestHeader and open
- Firefox does have the 2 checks

I could not check IE for the send() flag checks but I should be able to test tomorrow. I will update the results here as soon as I can.

[Julien Chaffraix]

Created attachment 18933 [details]
Correct mistakes in the ChangeLog

[Alexey Proskuryakov]

Comment on attachment 18933 [details]
Correct mistakes in the ChangeLog

r=me

[Alp Toker]

Landed in r30362.