Ticket #2065 (reopened bug)

Opened 4 years ago

Last modified 3 years ago

XMLHttpRequest: abort(read()) error

Reported by: fredj Owned by:
Priority: major Milestone: 2.13 Release
Component: Protocol.HTTP Version: 2.8 RC1
Keywords: Cc:
State: Needs Discussion

Description

The following html: 

<html>
  <head>
    <script src="http://www.openlayers.org/dev/lib/OpenLayers.js"></script>
    <script type="text/javascript">
        var protocol
        function init() {
          protocol = new OpenLayers.Protocol.HTTP();
          protocol.abort(protocol.read());
        }
    </script>
  </head>
  <body onload="init()"></body>
</html>

Raises an error on IE 7 (popup window), is silenced ignored in FF 3.0.9 without firebug and an error message is shown if firebug is installed. Both error messages blame browser:/trunk/openlayers/lib/OpenLayers/Request/XMLHttpRequest.js line 200

Seems to be related to http://openlayers.org/pipermail/dev/2009-April/004788.html

Attachments

2065.0.patch Download (5.9 KB) - added by fredj 4 years ago.
patch from Kris Geusebroek that fixes the problem.
2065.1.patch Download (1.2 KB) - added by rdewit 4 years ago.
XMLHttpRequest.patch Download (0.5 KB) - added by pmartin 3 years ago.
Fix NS_ERROR_NOT_INITIALIZED error
2065_abort.patch Download (1.2 KB) - added by bartvde 3 years ago.
using Eric's suggestion, and now includes a testcase
openlayers-2065-reopened.patch Download (2.4 KB) - added by ahocevar 3 years ago.
Use IE workaround from XMLHttpRequest.js and revert code that we added to Request.js to fix #1896
openlayers-2065-reopened.2.patch Download (1.5 KB) - added by ahocevar 3 years ago.

Change History

Changed 4 years ago by fredj

patch from Kris Geusebroek that fixes the problem.

  Changed 4 years ago by john_diss

Patch works well for me

  Changed 4 years ago by rdewit

  • state set to Review

Since it looks like we cannot use Kris' patch because of possible GPL infringement, it took another approach and implemented Kris' suggestion not to call the abort method on the underlying object when the readyState is 0 or 4. Instead of using 0, I needed to exclude 1 (OPENED) as well.

See patch 2065.1.patch

The following tests pass in FF3.5 and IE7:

  • Control/GetFeature.html
  • Protocol/WFS.html
  • Request.html
  • Request/XMLHttpRequest.html
  • Strategy/BBOX.html
  • Tile/WFS.html
  • manual/ajax.hml

Changed 4 years ago by rdewit

Changed 3 years ago by pmartin

Fix NS_ERROR_NOT_INITIALIZED error

  Changed 3 years ago by bartvde

The patch by pmartin is for a similar issue with abort(write()) which might deserve its own ticket?

  Changed 3 years ago by bartvde

  • milestone changed from 2.9 Release to 2.10 Release

Not a regression from OL 2.8 so bumping to 2.10.

  Changed 3 years ago by bartvde

see also ticket:2625 which should fix this issue as well

  Changed 3 years ago by bartvde

I've got a reply from Sergey about this issue, and he states it is actually an invalid use of his library. I've pasted his very thorough explanation: 

Hi Bart,

I made an investigation and found out that the reason my library throws an exception is caused on your side by improper execution flow, to be more concrete - method "send" (since the call you made is async) is called after "abort", which is illegal - the object must first be re-open.

There is a nice feature of my implementation which helped me with debugging: requests sniffing, look how the code of the test could be modified in order to demonstrate the problematic execution flow in your source code:

        var protocol
        function init() {
          OpenLayers.Request.XMLHttpRequest.onopen = function(args) {console.log("open", args)}
          OpenLayers.Request.XMLHttpRequest.onabort = function(args) {console.log("abort", args)}
          OpenLayers.Request.XMLHttpRequest.onsend = function(args) {console.log("send", args)}

          protocol = new OpenLayers.Protocol.HTTP();
          protocol.abort(protocol.read());
        }

This will output:

open GET
abort undefined
send null
uncaught exception: [Exception... "Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIXMLHttpRequest.send]" nsresult: "0xc1f30001 (NS_ERROR_NOT_INITIALIZED)" location: "JS frame :: http://www.openlayers.org/dev/lib/OpenLayers/Request/XMLHttpRequest.js :: anonymous :: line 200" data: no]



By the way, your test indeed also throws when using browser native XMLHttpRequest object implementation (this can be verified by excluding XMLHttpRequest.js library from OpenLayers.js and pointing OpenLayers.Request.XMLHttpRequest to the browser-based native object implementation)

Let me know if I can help further. Please also note that I will not "fix" my library to support the invalid scenario since this will contradict to what specification of XMLHttpRequest says and what browsers implement.

Sergey/

follow-up: ↓ 8   Changed 3 years ago by bartvde

Tests still pass in FF3 and IE6, please review my latest patch

in reply to: ↑ 7   Changed 3 years ago by elemoine

  • state changed from Review to Needs Discussion

Replying to bartvde:

Tests still pass in FF3 and IE6, please review my latest patch

Bart, if abort is called we don't want to call send, no? I'd rather see this: 

window.setTimeout(function() {
    if(request._aborted !== true) {
        request.send(config.data);
    }
});

unless I'm missing something...

Thanks,

  Changed 3 years ago by elemoine

also I think it'd be good to add a unit test for that issue, based on the response from Sergey

  Changed 3 years ago by bartvde

You're right Eric, I'll adapt the patch.

Changed 3 years ago by bartvde

using Eric's suggestion, and now includes a testcase

  Changed 3 years ago by bartvde

  • state changed from Needs Discussion to Review

I've adapted the patch and it now includes a testcase Eric.

  Changed 3 years ago by elemoine

  • state changed from Review to Commit

Confirming that the test fails in FF3, Chromium5 and IE7 when the changes to Request.js aren't applied, and pass in these browsers when the changes are applied. I think the patch does improve the situation. Thanks Bart, please commit.

  Changed 3 years ago by bartvde

  • status changed from new to closed
  • state changed from Commit to Complete
  • resolution set to fixed

(In [10341]) do not send request if abort has been called, r=elemoine (closes #2065)

follow-up: ↓ 15   Changed 3 years ago by pgiraud

  • status changed from closed to reopened
  • resolution fixed deleted

The tests pass for me on Chrome but fail on FF3 and IE8.

in reply to: ↑ 14 ; follow-up: ↓ 16   Changed 3 years ago by elemoine

Replying to pgiraud:

The tests pass for me on Chrome but fail on FF3 and IE8.

It seems that [10345] is guilty.

in reply to: ↑ 15   Changed 3 years ago by elemoine

Replying to elemoine:

Replying to pgiraud:

The tests pass for me on Chrome but fail on FF3 and IE8.

It seems that [10345] is guilty.

I think the problem is due to onreadystatechanged now defined after calling _object.open.

follow-up: ↓ 18   Changed 3 years ago by ahocevar

I don't think that checking for request._aborted is an option, because _aborted is not part of the XMLHttpRequest API. What if the browser's native XMLHttpRequest object is used?

in reply to: ↑ 17   Changed 3 years ago by bartvde

Replying to ahocevar:

I don't think that checking for request._aborted is an option, because _aborted is not part of the XMLHttpRequest API. What if the browser's native XMLHttpRequest object is used?

It did come to my mind after committing this patch, but is there then a way to solve this outside of the XMLHttpRequest library?

Changed 3 years ago by ahocevar

Use IE workaround from XMLHttpRequest.js and revert code that we added to Request.js to fix #1896

  Changed 3 years ago by ahocevar

  • state changed from Complete to Review

The above patch uncomments an optional code sequence in XMLHttpRequest.js which fixes #1896, without the need to send the request in setTimeout handler. Please review.

  Changed 3 years ago by bartvde

  • state changed from Review to Commit

Excellent solution Andreas, all the pieces are coming together now. Please commit.

  Changed 3 years ago by ahocevar

  • status changed from reopened to closed
  • state changed from Commit to Complete
  • resolution set to fixed

(In [10347]) Using the IE cache fix from XMLHttpRequest.js allows us to send asynchronous requests without a setTimeout handler. r=bartvde (closes #2065)

  Changed 3 years ago by tschaub

  • status changed from closed to reopened
  • state changed from Complete to Needs More Work
  • resolution fixed deleted

I haven't stepped through the code to know exactly what is going on, but I'm seeing double POST requests for all responses that don't contain a Date header - in Firefox. At a glance, this change looks far more reaching than the original intention.

Changed 3 years ago by ahocevar

  Changed 3 years ago by ahocevar

  • state changed from Needs More Work to Review

Another patch. This time the issue from #1896 is fixed on the application level. In Ext JS, every request gets a disable cache param (_dc) appended unless the request is configured with disableCache: false. I guess that is their way to fix the same issue, and usually we deal with servers that send the proper Cache headers anyway.

  Changed 3 years ago by elemoine

  • state changed from Review to Needs Discussion

We're also seeing double (GET) requests here, and it's breaking our application.

Andreas, adding a random param (_dc in your example) in the query string disables caching, but do we always want that? There are cases when we do want the browser to get responses from the cache, and for thoses cases we also do want resp.priv to be set when the callback is called. It looks to me that your patch suggests to disable caching to not run into the "resp.priv is undefined" issue, I'd rather fix the actual issue and the use of setTimeout that we had previously did resolve the issue, I think.

So I'm +1 on reverting [10347].

follow-up: ↓ 26   Changed 3 years ago by bartvde

@erilem: the setTimeout solution caused other problems, even with the browser's native XMLHttpRequest object, like the original subject of this ticket.

in reply to: ↑ 25   Changed 3 years ago by elemoine

Replying to bartvde:

@erilem: the setTimeout solution caused other problems, even with the browser's native XMLHttpRequest object, like the original subject of this ticket.

I don't get it. Andreas says 

I don't think that checking for request._aborted is an option,
because _aborted is not part of the XMLHttpRequest API. What if
the browser's native XMLHttpRequest object is used?

but actually OpenLayers.Request relies on OpenLayers.Request.XMLHttpRequest and not on the browser's native XMLHttpRequest object, so why can't we rely on the _aborted property?

  Changed 3 years ago by elemoine

and the test_abort test that we added in [10341] showed that relying on _aborted worked in every browser (FF3, IE7 and Chromium actually). Please tell I'm missing something here.

follow-up: ↓ 29   Changed 3 years ago by bartvde

Hi Eric, I think you are right provided that we don't allow people to use the native XMLHTTPRequest from the browser (by excluding XMLHTTPRequest.js from the OL build and pointing to the native browser XMLHTTPRequest object). Not sure if anybody out there is doing that right now. I assume that is what Andreas was pointing at.

in reply to: ↑ 28   Changed 3 years ago by elemoine

Replying to bartvde:

Hi Eric, I think you are right provided that we don't allow people to use the native XMLHTTPRequest from the browser (by excluding XMLHTTPRequest.js from the OL build and pointing to the native browser XMLHTTPRequest object). Not sure if anybody out there is doing that right now. I assume that is what Andreas was pointing at.

I see, so this is about making Request independent from the underlying XMLHttpRequest implementation, so Request should stick to the standard XMLHttpRequest interface.

  Changed 3 years ago by elemoine

I want to raise this issue again. Can we revert [10347]?

follow-up: ↓ 32   Changed 3 years ago by ahocevar

No objections from me against reverting r10347

in reply to: ↑ 31   Changed 3 years ago by elemoine

Replying to ahocevar:

No objections from me against reverting r10347

Ok thanks Andreas. I'll do it when I get to the office in a few hours.

Note: See TracTickets for help on using tickets.