Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

Commit

Permalink
fix($httpBackend): only IE8 and below can't use script.onload for J…
Browse files Browse the repository at this point in the history
…SONP

IE8, IE9 and IE10 can use `script.onreadystate` so up till now we have been using this
if the sniffer says we are on IE.
But IE11 now does not support `script.onreadystate` and only supports the more standard
`script.onload` and `script.onerror`.
IE9 and IE10 do support `script.onload` and `script.onerror`. So now we only test whether
we are on IE8 or earlier before using `script.onreadystate`.
See http://pieisgood.org/test/script-link-events/

jQuery just uses all these handlers at once and hopes for the best, but since IE9 and IE10
support both sets of handlers, this could cause the handlers to be run more than once.

jQuery also notes that there is a potential memory leak in IE unless we remove the handlers
from the script object once they are run.  So we are doing this too, now.

Closes #4523
Closes #4527
Closes #4922
  • Loading branch information
petebacondarwin committed Nov 22, 2013
1 parent 84c408c commit a3172a2
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 6 deletions.
11 changes: 8 additions & 3 deletions src/ng/httpBackend.js
Original file line number Diff line number Diff line change
Expand Up @@ -136,19 +136,24 @@ function createHttpBackend($browser, XHR, $browserDefer, callbacks, rawDocument,
// - adds and immediately removes script elements from the document
var script = rawDocument.createElement('script'),
doneWrapper = function() {
script.onreadystatechange = script.onload = script.onerror = null;
rawDocument.body.removeChild(script);
if (done) done();
};

script.type = 'text/javascript';
script.src = url;

if (msie) {
if (msie && msie <= 8) {

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Nov 22, 2013

Contributor

it should be enough to just check if msie <= 8

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Nov 22, 2013

Author Contributor

You are right sorry.

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Nov 22, 2013

Author Contributor

As long as msie is not null. I think it is NaN if we are not on Internet Explorer.

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Nov 22, 2013

Contributor

yes. NaN was picked specifically so that we can use simple comparisons.

can you please change this in a follow up commit after the 1.2.2 release?

script.onreadystatechange = function() {
if (/loaded|complete/.test(script.readyState)) doneWrapper();
if (/loaded|complete/.test(script.readyState)) {
doneWrapper();
}
};
} else {
script.onload = script.onerror = doneWrapper;
script.onload = script.onerror = function() {
doneWrapper();
};

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Nov 22, 2013

Contributor

This change wasn't necessary. Was it?

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Nov 22, 2013

Author Contributor

Again you are right, sorry. I originally added the line to set onload = onerror = null in this function but then moved it to doneWrapper, since we return doneWrapper from this function and it may be called externally.

This comment has been minimized.

Copy link
@IgorMinar

IgorMinar Nov 22, 2013

Contributor

no worries, just change it after 1.2.2 please

}

rawDocument.body.appendChild(script);
Expand Down
38 changes: 35 additions & 3 deletions test/ng/httpBackendSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@ describe('$httpBackend', function() {
script.readyState = 'complete';
script.onreadystatechange();
} else {
script.onload()
script.onload();
}

expect(callback).toHaveBeenCalledOnce();
Expand All @@ -313,14 +313,46 @@ describe('$httpBackend', function() {
script.readyState = 'complete';
script.onreadystatechange();
} else {
script.onload()
script.onload();
}

expect(callbacks[callbackId]).toBeUndefined();
expect(fakeDocument.body.removeChild).toHaveBeenCalledOnceWith(script);
});


if(msie<=8) {

it('should attach onreadystatechange handler to the script object', function() {
$backend('JSONP', 'http://example.org/path?cb=JSON_CALLBACK', null, noop);

expect(fakeDocument.$$scripts[0].onreadystatechange).toEqual(jasmine.any(Function));

var script = fakeDocument.$$scripts[0];

script.readyState = 'complete';
script.onreadystatechange();

expect(script.onreadystatechange).toBe(null);
});

} else {

it('should attach onload and onerror handlers to the script object', function() {
$backend('JSONP', 'http://example.org/path?cb=JSON_CALLBACK', null, noop);

expect(fakeDocument.$$scripts[0].onload).toEqual(jasmine.any(Function));
expect(fakeDocument.$$scripts[0].onerror).toEqual(jasmine.any(Function));

var script = fakeDocument.$$scripts[0];
script.onload();

expect(script.onload).toBe(null);
expect(script.onerror).toBe(null);
});

}

it('should call callback with status -2 when script fails to load', function() {
callback.andCallFake(function(status, response) {
expect(status).toBe(-2);
Expand All @@ -335,7 +367,7 @@ describe('$httpBackend', function() {
script.readyState = 'complete';
script.onreadystatechange();
} else {
script.onload()
script.onload();
}
expect(callback).toHaveBeenCalledOnce();
});
Expand Down

0 comments on commit a3172a2

Please sign in to comment.