Opened 10 years ago

Closed 10 years ago

#9434 closed defect (fixed)

Unnecessary code in xhr.js?

Reported by: Eugene Lazutkin Owned by: James Burke
Priority: low Milestone: 1.4
Component: Core Version: 1.3.1
Keywords: Cc:
Blocked By: Blocking:

Description

We have code that tries to parse XML when XML is expected from a server: http://bugs.dojotoolkit.org/browser/dojo/trunk/_base/xhr.js#L262

For your convenience these are the lines:

xml: function(xhr){
  var result = xhr.responseXML;
  //>>excludeStart("webkitMobile", kwArgs.webkitMobile);
  if(_d.isIE && (!result || !result.documentElement)){
    var ms = function(n){ return "MSXML" + n + ".DOMDocument"; }
    var dp = ["Microsoft.XMLDOM", ms(6), ms(4), ms(3), ms(2)];
    _d.some(dp, function(p){
      try{
        var dom = new ActiveXObject(p);
        dom.async = false;
        dom.loadXML(xhr.responseText);
        result = dom;
      }catch(e){ return false; }
      return true;
    });
  }
  //>>excludeEnd("webkitMobile");
  return result; // DOMDocument
}

This code is called when handleAs == "xml".

If you look at the code framed by comments, which exclude it from webkitMobile, it is supposed to run on IE only and when no xhr.responseXML nor result.documentElement are available.

Let's take a look at Microsoft's documentation: http://msdn.microsoft.com/en-us/library/ms534370(VS.85).aspx. It states that "responseXML was introduced in Windows Internet Explorer 7". So we are talking about IE6.

The code in question looks for a presence of a working version of XMLHttpRequest on every single call. My original idea was to optimize it a bit, remove an unnecessary dynamic creation of ms() function, and cache the successful result.

But first I ran this code on IE6 to see how bad it is and ... responseXML was here, and the code was never executed. I guess that problem was fixed long time ago by Microsoft with some service pack or an individual fix. After all XML is very important for IT, and IT is the main cash cow of Microsoft.

Amazed I decided to look how our friends at prototype, jquery, and mootols do that conversion --- they don't. These frameworks assume that responseXML is available:

The only postprocessing is done by prototype: it checks the result for undefined, and nullifies it. No voodoo with dynamic instantiation, no XML parsing, no XML checks.

So my question is: do we really have a use case for this code? We are talking about ~10 dense lines of unused code in the Base.

Change History (8)

comment:1 Changed 10 years ago by James Burke

It looks like #3294 is the origin of that code block. Apparently and issue with IE 6 and local files. You can see if it is still an issue with the test case in that ticket, or maybe we re-evaluate if we need to support that scenario.

comment:2 Changed 10 years ago by Eugene Lazutkin

Heh. I tested a slightly modified example from #3294. It failed like described on IE6, IE7, and IE8. So all IE are affected. Just for kicks I rewrote it with jquery, and it failed too.

Actually it sounds logical: opening files from the file system means no headers are set => most probably IE uses a MIME type to trigger the conversion process, it is not there, so the result is unprocessed.

So it looks like we have two ways to go from there:

  1. Keep it as it is, but refresh the code a little bit per my original idea.
  2. Ditch that code because running from a file system with XHR is not a viable environment for any realistic web development.

Let's not forget that there are security implications for reading local files with XHR, e.g., Firefox 3+ refuses to open files that are not in the same directory as the web page or in any of its subdirectories.

comment:3 Changed 10 years ago by James Burke

Since using a browser with the local file system has a problem in IE 7 and Firefox, let's pull the code out. If there are reports of issues, we can either suggest an easy monkey patch, or if there is enough of a call for it, we can put it back in a point release.

comment:4 Changed 10 years ago by virsir

I encountered the problem some time ago.
IE, use xhrGet to load xml from server side(not a local file system), the responseXML node's documentElement is null.

comment:5 in reply to:  4 Changed 10 years ago by Eugene Lazutkin

Replying to virsir:

I encountered the problem some time ago. IE, use xhrGet to load xml from server side(not a local file system), the responseXML node's documentElement is null.

Can you explain how to reproduce the problem? I suspect it has to deal with wrong MIME type. Or was it something else?

comment:6 Changed 10 years ago by Sam Foster

for the responseXML property to be populated (with the XMLDocument representation) the response needs to have been sent with an xml Content-type header. This is irritating, as IE has a sometimes-annoying ability to infer content type from the content: if you send html as text/plain, it will still load up as html in the browser. That's not true of XHR however.

There are plenty of poeple using dojo and xhr over the file:// protocol. *someone* will complain about removing this, but I agree this usage was deprecated/broken for us by the browser vendors, and it shouldnt be in base.

comment:7 Changed 10 years ago by James Burke

Milestone: tbd1.4

comment:8 Changed 10 years ago by James Burke

Resolution: fixed
Status: newclosed

(In [20355]) Fixes #9434, remove bloat code from xhr.js, adjusted unit test to do something more realistic to test xml content handling.

Note: See TracTickets for help on using tickets.