Update httpBackend, check window.XMLHttpRequest #5679

jorgt · 2014-01-08T01:26:18Z

As per this issue: #5677

window.XMLHttpRequest is not always available in IE8 despite it not running in quirks mode, in which case Angular should be using the ActiveXObject instead.

As per this issue: angular#5677 window.XMLHttpRequest is not always available in IE8 despite it not running in quirks mode, in which case Angular should be using the ActiveXObject instead. Just checking the browser version is taking too many shortcuts.

IgorMinar · 2014-01-08T02:13:15Z

I'm sorry, but I wasn't able to verify your CLA signature. CLA signature is required for any code contributions to AngularJS.

Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match.

If you signed the CLA as a corporation, please let me know the company's name.

Thanks a bunch!

PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR.
PS2: If you are a Googler, please sign the CLA as well to simplify the CLA verification process.

jorgt · 2014-01-08T02:16:52Z

I've just signed the CLA

IgorMinar · 2014-01-08T03:13:16Z

CLA signature verified! Thank you!

Someone from the team will now triage your PR and it will be processed based on the determined priority (doc updates and fixes with tests are prioritized over other changes).

IgorMinar · 2014-01-08T07:52:40Z

Is the activex xhr a drop in replacement of the regular xhr object in IE? I know that it works well for patch requests but I don't know of all the other circumstances.

IgorMinar · 2014-01-08T07:54:07Z

src/ng/httpBackend.js

@@ -3,7 +3,7 @@
 function createXhr(method) {
  // IE8 doesn't support PATCH method, but the ActiveX object does
  /* global ActiveXObject */
-  return (msie <= 8 && lowercase(method) === 'patch')
+  return ((msie <= 8 && lowercase(method) === 'patch') || isUndefined(window.XMLHttpRequest))


this change means that we should use activex even for non-ie browsers if window.xhr is not defined. that's not right

According to https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest it's only IE that needs a backfill (see Browser Compatibility). My test case is admittedly limited. Besides, right now on browsers that do not define window.xhr, the entire $http module fails which is worse.

Why not use a try...catch?

Assuming you mean this method (simplified):

var xhr; try { xhr = new ActiveXObject('Microsoft.XMLHTTP'); } catch(e) { xhr = new window.XMLHttpRequest(); } return xhr;

This always uses ActiveX in IE instead of XMLHttpRequest which is not really in the spirit of the original code of using ActiveX only when necessary. This was the simplest change I could make without rewriting the method. There are try / catch blocks higher up in the tree and I'm not sure what removing an exception here will do.

Not quite.

var xhr; if( msie <= 8 && lowercase(method) === 'patch' ) { xhr = new ActiveXObject('Microsoft.XMLHTTP'); } else { try { xhr = new window.XMLHttpRequest(); } catch( e ) { xhr = new ActiveXObject('Microsoft.XMLHTTP'); } } return xhr;

Will that not only throw an error when XMLHttpRequest is undefined, which is what was checked already?

I don't mind changing that though, if it's considered more sturdy.

jorgt · 2014-01-08T08:27:57Z

@IgorMinar Yes, activex is used in IE5-7 because they does not implement XMLHttpRequest. Apparently - and this was new to me - it also fails in certain IE8 security settings. I think this is also the way jQuery implements their xhr instantiation. From their comments:

    /* Microsoft failed to properly
     * implement the XMLHttpRequest in IE7 (can't request local files),
     * so we use the ActiveXObject when it is available
     * Additionally XMLHttpRequest can be disabled in IE7/IE8 so
     * we need a fallback.

after which they either use the activex or the regular xhr requests.

caitp · 2014-01-22T23:36:24Z

Things we know:

IE8 can't send PATCH requests without XMLHttpRequest
XMLHttpRequest might not be available on IE if security policy is bad
Everywhere else should just use XHR or XHR2 because they exist and they're nice

So, we can do it like this:

if (window.XMLHttpRequest) {
  return new window.XMLHttpRequest();
} else if (window.ActiveXObject && method !== 'patch') {
  // Try to find an ActiveXObject we can use...
} else {
  // We don't know how
  throw $httpMinErr('noxhr', ...);
}

I'm not sure if there is any reason not to use XHR on IE if it's available, if someone has this knowledge please fill me in on it. Otherwise, I think this solution should just be compressed and shipped

caitp · 2014-01-22T23:48:42Z

jQuery's fix for this issue (apparently), courtessy of Wesley Cho jquery/jquery@06ee2c1

To analyze this:

- If ActiveXObject exists (IE):
    - If the method type is "get", "post", "head", "put", "delete", or "options"
      create XMLHttpRequest
    - Otherwise, create ActiveX XHR instance
- Otherwise, create XMLHttpRequest

We need to also check if XMLHttpRequest exists in the IE branch, because it may not be due to security policy settings

Finally, for error reporting, it would be better if we could throw noxhr when we need to use a patch request (for instance) but can't. Finally, if we don't have any way of creating an XHR object, we should also throw noxhr (but I'm not sure this one is a big deal since everything should have it these days)

If the code can be re-arranged to fit all of these requirements, it should be good to merge I think

jorgt · 2014-01-23T00:58:50Z

@caitp Thanks for the suggestion. However, if I'm not mistaken point 1 should be: IE8 does PATCH requests better using ActiveX

Here is some interesting material on IE's implementations: http://msdn.microsoft.com/en-us/library/ms537505(vs.85).aspx, http://snook.ca/archives/javascript/xmlhttprequest_activex_ie

How far back in IE's murky history do you suggest we go in finding an ActiveXObject? We have ActiveXObject("Msxml2.XMLHTTP.6.0"), ActiveXObject("Msxml2.XMLHTTP.3.0"). Apparently ActiveXObject("Microsoft.XMLHTTP") maps to 3.0, which seems universally available from IE7 on, but does not have some bug fixes present in 6.0.

I'm not familiar with $httpMinErr's syntax...

How about:

function createXHR(method) {
    var xhr;
    //if activeXObject is present (IE) and method is PATCH, or if XMLHttpRequest
    //is not available, try getting the ActiveXObject. Otherwise, use XMLHttpRequest
    //if it is available
    if ((window.ActiveXObject && method === 'patch') || !window.XMLHttpRequest) {
        try {
            //version 6.0 is newer, but not present on every machine
            xhr = new ActiveXObject("MSXML2.XMLHTTP.6.0");
        } catch (e) {
            try {
                //the version independent XMLHTTP object maps to version 3.0
                //which should be available on current MS supported OS'
                xhr = new ActiveXObject("Microsoft.XMLHTTP");
            } catch (e) {
            }
        }
    } else if (window.XMLHttpRequest) {
        xhr = new window.XMLHttpRequest();
    }

    if (!xhr) {
        throw $httpMinErr('noxhr', '');
    } else {
        return xhr;
    }
}

caitp · 2014-01-23T01:01:48Z

@jorgt you can see an example of the minerr syntax you'd want to use for this here

I like jQuery's approach of not being specific to the PATCH method, but saying "if it's not an RFC2616 method, use ActiveX", I think this would be a good way to go. Basically, If it's an RFC2616 method and we have XHR, use XHR, otherwise use ActiveX

jorgt · 2014-01-23T01:32:12Z

@caitp That is agreeable... Also borrowing from the link you provided:

function createXhr(method) {
    //if IE and the method is not RFC2616 compliant, or if XMLHttpRequest
    //is not available, try getting an ActiveXObject. Otherwise, use XMLHttpRequest
    //if it is available
    if ((window.ActiveXObject && !method.match(/^(get|post|head|put|delete|options)$/i)) 
        || !window.XMLHttpRequest) {

        try { return new ActiveXObject("Msxml2.XMLHTTP.6.0"); } catch (e1) {}
        try { return new ActiveXObject("Msxml2.XMLHTTP.3.0"); } catch (e2) {}
        try { return new ActiveXObject("Msxml2.XMLHTTP"); } catch (e3) {}
    } else if (window.XMLHttpRequest) {
        return new window.XMLHttpRequest();
    }

    throw $httpMinErr('noxhr', 'This browser does not support XMLHttpRequest.');
}

caitp · 2014-01-23T01:38:42Z

I think it was decided that just using Microsoft.XMLHTTP is probably fine... I don't have a very good way to test this (just the SauceLabs browsers...) but honestly I think we should probably just stick to using "Microsoft.XMLHTTP" and address problems with it after they turn up, because they seem to not exist so far.

caitp · 2014-01-23T01:42:23Z

@juliemr if you have time, could you give us a hand setting up a protractor test to verify that it works correctly for all browsers? I am totally lost when it comes to the new protractor E2E tests in core. I don't think we need to make a remote request like that one ngScenario test that keeps flaking out lately is doing, but it would be good to make sure we get some reasonable results from this. Let me know if it's out of the question for the time being

jorgt · 2014-01-23T01:43:01Z

Just ActiveXObject("Microsoft.XMLHTTP")? I'll submit a new pull request...

As per this issue: angular#5677

caitp · 2014-01-23T01:47:29Z

No need to submit a new PR, just a new commit is fine ;) which it looks like you did.

jorgt · 2014-01-29T06:33:42Z

@caitp so... what happens now?

IgorMinar · 2014-01-31T11:55:46Z

you also need to add the ngdoc file for the error. this file was deleted in 6c17d02

window.XMLHttpRequest is not always available in IE8 despite it not running in quirks mode, in which case Angular should be using the ActiveXObject instead. Just checking the browser version is taking too many shortcuts. Closes angular#5677 Closes angular#5679

IgorMinar reviewed Jan 8, 2014
View reviewed changes

ghost assigned caitp Jan 22, 2014

Update httpBackend, check window.XMLHttpRequest

5638bf5

As per this issue: angular#5677

IgorMinar closed this in ef210e5 Feb 1, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update httpBackend, check window.XMLHttpRequest #5679

Update httpBackend, check window.XMLHttpRequest #5679

jorgt commented Jan 8, 2014

IgorMinar commented Jan 8, 2014

jorgt commented Jan 8, 2014

IgorMinar commented Jan 8, 2014

IgorMinar commented Jan 8, 2014

IgorMinar Jan 8, 2014

jorgt Jan 8, 2014

KendrickIW Jan 8, 2014

jorgt Jan 8, 2014

KendrickIW Jan 9, 2014

jorgt Jan 10, 2014

jorgt commented Jan 8, 2014

caitp commented Jan 22, 2014

caitp commented Jan 22, 2014

jorgt commented Jan 23, 2014

caitp commented Jan 23, 2014

jorgt commented Jan 23, 2014

caitp commented Jan 23, 2014

caitp commented Jan 23, 2014

jorgt commented Jan 23, 2014

caitp commented Jan 23, 2014

jorgt commented Jan 29, 2014

IgorMinar commented Jan 31, 2014

Update httpBackend, check window.XMLHttpRequest #5679

Update httpBackend, check window.XMLHttpRequest #5679

Conversation

jorgt commented Jan 8, 2014

IgorMinar commented Jan 8, 2014

jorgt commented Jan 8, 2014

IgorMinar commented Jan 8, 2014

IgorMinar commented Jan 8, 2014

IgorMinar Jan 8, 2014

Choose a reason for hiding this comment

jorgt Jan 8, 2014

Choose a reason for hiding this comment

KendrickIW Jan 8, 2014

Choose a reason for hiding this comment

jorgt Jan 8, 2014

Choose a reason for hiding this comment

KendrickIW Jan 9, 2014

Choose a reason for hiding this comment

jorgt Jan 10, 2014

Choose a reason for hiding this comment

jorgt commented Jan 8, 2014

caitp commented Jan 22, 2014

caitp commented Jan 22, 2014

jorgt commented Jan 23, 2014

caitp commented Jan 23, 2014

jorgt commented Jan 23, 2014

caitp commented Jan 23, 2014

caitp commented Jan 23, 2014

jorgt commented Jan 23, 2014

caitp commented Jan 23, 2014

jorgt commented Jan 29, 2014

IgorMinar commented Jan 31, 2014