fix `The onreadystatechange property of XMLHttpRequest.prototype redefined by Angular` (close #1283) #1287

LavrovArtem · 2017-08-29T14:19:44Z

/cc @miherlosev @churkin @AlexanderMoskovkin

testcafe-build-bot · 2017-08-29T14:43:36Z

❌ Tests for the commit 46d6244 have failed. See details:

miherlosev · 2017-08-30T08:22:24Z

test/client/fixtures/sandbox/xhr-test.js

@@ -201,3 +205,26 @@ asyncTest('authorization headers by client should be processed (GH-1016)', funct
    });
    xhr.send();
 });
+
+asyncTest('our internal the onreadystatechange handler must be first (GH-1283)', function () {


'XHR_COMPLETED_EVENT' for completed requests should be raised before calling outer handlers (GH-1283)

miherlosev · 2017-08-30T08:22:41Z

test/client/fixtures/sandbox/xhr-test.js

+
+asyncTest('our internal the onreadystatechange handler must be first (GH-1283)', function () {
+    var xhr      = new XMLHttpRequest();
+    var timeout  = null;


timeoutId

miherlosev · 2017-08-30T08:24:42Z

src/client/sandbox/xhr.js

-        const xmlHttpRequestProto = window.XMLHttpRequest.prototype;
+        const xhrSandbox           = this;
+        const xmlHttpRequestProto  = window.XMLHttpRequest.prototype;
+        const xhrConstructorString = nativeMethods.XMLHttpRequest.toString();


Move to the nativeMethods and rename to the xmlHttpRequestToString.
Same as https://github.com/DevExpress/testcafe-hammerhead/blob/master/src/client/sandbox/native-methods.js#L212

But it is string, it is not a function.
And it is used only in this place.

Ok. Don't move it. But rename.

xmlHttpRequestString ?

xmlHttpRequestToString

sounds like a function

Maybe.
You can use any notation.
xmlHttpRequestString of xmlHttpRequestToString
We already using ToString notation:
https://github.com/DevExpress/testcafe-hammerhead/blob/master/src/client/sandbox/node/window.js#L18

miherlosev · 2017-08-30T08:27:36Z

src/client/sandbox/xhr.js

@@ -73,6 +81,20 @@ export default class XhrSandbox extends SandboxBase {
            nativeMethods.xhrRemoveEventListener.call(this, 'readystatechange', syncCookieWithClient);
        };

+        const xhrConstructorWrapper = function () {


xmlHttpRequestWrapper

miherlosev · 2017-08-30T08:31:36Z

test/client/fixtures/sandbox/xhr-test.js

+
+    xhr.open('GET', '/xhr-test/', true);
+    xhr.send();
+});


You change prototype chain for XMLHttpRequest and override ToString method.
Let's add test code to the following cases:

var xhr = new XMLHttpRequest(); xhr instanceof XMLHttpRequest;

xhr.toString() === nativeXhr.ToString()

miherlosev · 2017-08-30T09:14:32Z

src/client/sandbox/xhr.js


            if (xhrSandbox.corsSupported)
-                nativeMethods.xhrSetRequestHeader.call(this, XHR_HEADERS.corsSupported, 'true');
+                nativeMethods.xhrSetRequestHeader.call(xhr, XHR_HEADERS.corsSupported, 'true');


We don't need the additional variable xhr.
Another methods (abort, open) don't declare an additional variable.
So, drop const xhr = this; line and change xhr to this for places in send method.

miherlosev · 2017-08-30T09:34:16Z

test/client/fixtures/sandbox/xhr-test.js

+
+    var readyStateChangeHandler = function (e) {
+        if (this.readyState === this.DONE)
+            e.stopImmediatePropagation();


add ok(false, 'outer handler is called before XHR_COMPLETED_EVENT');

this handler will be called in all cases

testcafe-build-bot · 2017-08-31T09:30:46Z

❌ Tests for the commit d60e74c have failed. See details:

testcafe-build-bot · 2017-08-31T10:58:33Z

✅ Tests for the commit b4b835f have passed. See details:

LavrovArtem · 2017-08-31T11:31:31Z

FPR

miherlosev · 2017-08-31T12:02:49Z

src/client/sandbox/xhr.js

+            value: xmlHttpRequestWrapper
+        });
+
+        const defineConstructorConst = (name, value) => {


defineProperty(obj, name, value)

miherlosev · 2017-08-31T12:06:04Z

src/client/sandbox/xhr.js

+            });
+        };
+
+        defineConstructorConst('UNSENT', 0);


defineProperty(xmlHttpRequestWrapper, 'UNSENT', XMLHttpRequest.UNSENT)
and etc.

I propose this variant because it is more readable.

Also XMLHttpRequest has another static properties such as name, valueOf and etc.
But I think we can skip it.

miherlosev · 2017-08-31T12:15:23Z

test/client/fixtures/sandbox/xhr-test.js

@@ -69,10 +73,23 @@ test('createNativeXHR', function () {
    }
 });

+test('constructor', function () {


toString, instanceof, constructor and static properties
or simple properties

miherlosev · 2017-08-31T14:32:29Z

src/client/sandbox/xhr.js

+            return xhr;
+        };
+
+        for (const prop of ['UNSENT', 'OPENED', 'HEADERS_RECEIVED', 'LOADING', 'DONE']) {


Store as `const XHR_READY_STATES = ['UNSENT', 'OPENED', 'HEADERS_RECEIVED', 'LOADING', 'DONE'] outside of module

and rewrite as

for (const readyState of XML_HTTP_REQUEST_READY_STATES)...

testcafe-build-bot · 2017-08-31T15:31:35Z

❌ Tests for the commit 0a4d37c have failed. See details:

miherlosev · 2017-08-31T15:32:55Z

@testcafe-build-bot \retest

testcafe-build-bot · 2017-08-31T16:17:58Z

❌ Tests for the commit 0a4d37c have failed. See details:

LavrovArtem · 2017-08-31T19:22:36Z

@testcafe-build-bot \retest

testcafe-build-bot · 2017-08-31T19:47:35Z

✅ Tests for the commit 0a4d37c have passed. See details:

LavrovArtem · 2017-08-31T20:07:14Z

FPR

miherlosev · 2017-09-01T08:33:54Z

test/client/fixtures/sandbox/xhr-test.js

 module('regression');

 asyncTest('unexpected text modifying during typing text in the search input on the http://www.google.co.uk (B238528)', function () {
-    var timeout            = 100;
+    var timeoutId          = 100;


Revert back timeoutId to timeout for this test.

miherlosev · 2017-09-01T10:09:16Z

test/client/fixtures/sandbox/xhr-test.js

@@ -201,3 +218,27 @@ asyncTest('authorization headers by client should be processed (GH-1016)', funct
    });
    xhr.send();
 });
+
+asyncTest('"XHR_COMPLETED_EVENT" for completed requests should be raised before calling outer handlers (GH-1283)', function () {


'XHR_COMPLETED_EVENT' should be raised when xhr is prevented

testcafe-build-bot · 2017-09-01T11:36:14Z

✅ Tests for the commit 2045e6b have passed. See details:

…fined by Angular` (close DevExpress#1283)

testcafe-build-bot · 2017-09-04T10:59:57Z

❌ Tests for the commit acb6938 have failed. See details:

LavrovArtem · 2017-09-04T11:17:00Z

@testcafe-build-bot \retest

testcafe-build-bot · 2017-09-04T12:06:53Z

❌ Tests for the commit acb6938 have failed. See details:

LavrovArtem · 2017-09-04T12:19:10Z

@testcafe-build-bot \retest

testcafe-build-bot · 2017-09-04T12:54:27Z

✅ Tests for the commit acb6938 have passed. See details:

…fined by Angular` (close DevExpress#1283) (DevExpress#1287) * fix `The onreadystatechange property of XMLHttpRequest.prototype redefined by Angular` (close DevExpress#1283) * fix * fix review's issues * some fix * refactoring * refactoring * fix review's issues * refactoring

LavrovArtem requested review from miherlosev, churkin and AlexanderMoskovkin August 29, 2017 14:19

miherlosev requested changes Aug 30, 2017

View reviewed changes

miherlosev requested changes Aug 31, 2017

View reviewed changes

AlexanderMoskovkin approved these changes Aug 31, 2017

View reviewed changes

miherlosev requested changes Aug 31, 2017

View reviewed changes

miherlosev requested changes Sep 1, 2017

View reviewed changes

miherlosev approved these changes Sep 1, 2017

View reviewed changes

AlexanderMoskovkin approved these changes Sep 1, 2017

View reviewed changes

LavrovArtem added 8 commits September 4, 2017 13:03

fix `The onreadystatechange property of XMLHttpRequest.prototype rede…

2cb0448

…fined by Angular` (close DevExpress#1283)

fix

2dd7777

fix review's issues

35e7172

some fix

2d6e5ac

refactoring

3419977

refactoring

da91078

fix review's issues

8a95868

refactoring

acb6938

LavrovArtem force-pushed the i1283 branch from 2045e6b to acb6938 Compare September 4, 2017 10:31

churkin approved these changes Sep 4, 2017

View reviewed changes

miherlosev merged commit 397a0d4 into DevExpress:master Sep 4, 2017

LavrovArtem deleted the i1283 branch September 4, 2017 12:56

miherlosev mentioned this pull request Sep 13, 2017

MacOS: setFilesToUpload() gives "Uncaught RangeError: Maximum call stack size exceeded" DevExpress/testcafe#1731

Closed

LavrovArtem mentioned this pull request May 30, 2018

Hammerhead throw an error, when "xhr.onreadystatechange" property is broken. #1012

Closed

fix The onreadystatechange property of XMLHttpRequest.prototype redefined by Angular (close #1283) #1287

fix The onreadystatechange property of XMLHttpRequest.prototype redefined by Angular (close #1283) #1287

Conversation

LavrovArtem commented Aug 29, 2017

testcafe-build-bot commented Aug 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LavrovArtem Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

testcafe-build-bot commented Aug 31, 2017

testcafe-build-bot commented Aug 31, 2017

LavrovArtem commented Aug 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miherlosev Aug 31, 2017 • edited Loading

Choose a reason for hiding this comment

testcafe-build-bot commented Aug 31, 2017

miherlosev commented Aug 31, 2017

testcafe-build-bot commented Aug 31, 2017

LavrovArtem commented Aug 31, 2017

testcafe-build-bot commented Aug 31, 2017

LavrovArtem commented Aug 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

testcafe-build-bot commented Sep 1, 2017

testcafe-build-bot commented Sep 4, 2017

LavrovArtem commented Sep 4, 2017

testcafe-build-bot commented Sep 4, 2017

LavrovArtem commented Sep 4, 2017

testcafe-build-bot commented Sep 4, 2017

fix `The onreadystatechange property of XMLHttpRequest.prototype redefined by Angular` (close #1283) #1287

fix `The onreadystatechange property of XMLHttpRequest.prototype redefined by Angular` (close #1283) #1287

LavrovArtem Aug 31, 2017 •

edited

Loading

miherlosev Aug 31, 2017 •

edited

Loading