Fix XHR argument parsing and synchronous event callbacks. (issue 251030043 by kpreid@google.com)

[re...@codereview-hr.appspotmail.com]

Reviewers: MarkM,

Description:
Derived from James Keane's patches
<https://github.com/google/caja/pull/1965> -- thanks to him for spotting
the problem. Issues in our tests and in onreadystatechange were spotted
in testing and additionally fixed.

Allegedly fixes <https://github.com/google/caja/issues/1878>.

* Pass the correct number of arguments to native XMLHttpRequest, rather
than one too many. Among other things, this caused us to incorrectly
default to synchronous XHR.
* Fixed onreadystatechange firing twice after synchronous XHR on Chrome
due to using an unsound browser test rather than a feature test to
decide whether to apply a workaround.
* Fixed lack of argument coercion for some uses of the async parameter.
(Should have no observable effects, so this is just on general
code structure principles.)

Supporting changes:
* Fixed test-cajajs-invocation testBuilderApiNetNoFetch accidentally
depending on that synchronous XHR.
* Increased timeout for preliminary-meta from 10 to 30 seconds as
I have experienced timeouts on that test only frequently, and it is
inherently slow compared to others.

Please review this at https://codereview.appspot.com/251030043/

Affected files (+40, -16 lines):
M src/com/google/caja/plugin/domado.js
M tests/com/google/caja/plugin/MainBrowserTest.java
M tests/com/google/caja/plugin/test-cajajs-invocation.js
M tests/com/google/caja/plugin/test-domado-events-guest.html


Index: src/com/google/caja/plugin/domado.js
diff --git a/src/com/google/caja/plugin/domado.js
b/src/com/google/caja/plugin/domado.js
index
25518c1abb8e88ef50a8ea96b2cb5d556a28f112..67ef75dc64347b82402f1be78fdf51b64aba722a
100644
--- a/src/com/google/caja/plugin/domado.js
+++ b/src/com/google/caja/plugin/domado.js
@@ -951,6 +951,7 @@ var Domado = (function() {

privates.async = undefined;
privates.handler = undefined;
+ privates.nativeCompleteEventSeen = false;

Object.preventExtensions(privates);
});
@@ -964,6 +965,11 @@ var Domado = (function() {
// to DOM events.
var self = this;
privates.feral.onreadystatechange = function(event) {
+ if (privates.feral.readyState === 4) {
+ // Detect whether this event was fired. See open() for how
this
+ // is used.
+ privates.nativeCompleteEventSeen = true;
+ }
var evt = { target: self };
try {
return handler.call(void 0, evt);
@@ -1020,6 +1026,13 @@ var Domado = (function() {
privates, method, URL, opt_async, opt_userName, opt_password) {
method = String(method);
URL = URI.utils.resolve(getBaseURL(), String(URL));
+ // Sanitizing these optionals up front; but note that we switch on
+ // arguments.length below, so this does not mean that opt_async
+ // defaults to false.
+ opt_async = Boolean(opt_async);
+ opt_userName = String(opt_userName);
+ opt_password = String(opt_password);
+
// The XHR interface does not tell us the MIME type in advance, so
we
// must assume the broadest possible.
var safeUri = uriRewrite(
@@ -1036,28 +1049,27 @@ var Domado = (function() {
if ('string' !== typeof safeUri) {
throw 'URI violates security policy';
}
- switch (arguments.length) {
+ switch (arguments.length - 1) {
case 2:
privates.async = true;
privates.feral.open(method, safeUri);
break;
case 3:
privates.async = opt_async;
- privates.feral.open(method, safeUri, Boolean(opt_async));
+ privates.feral.open(method, safeUri, opt_async);
break;
case 4:
privates.async = opt_async;
privates.feral.open(
- method, safeUri, Boolean(opt_async), String(opt_userName));
+ method, safeUri, opt_async, opt_userName);
break;
case 5:
privates.async = opt_async;
privates.feral.open(
- method, safeUri, Boolean(opt_async), String(opt_userName),
- String(opt_password));
+ method, safeUri, opt_async, opt_userName, opt_password);
break;
default:
- throw 'XMLHttpRequest cannot accept ' + arguments.length +
+ throw 'XMLHttpRequest cannot accept ' + (arguments.length - 1) +
' arguments';
break;
}
@@ -1066,6 +1078,8 @@ var Domado = (function() {
privates.feral.setRequestHeader(String(label), String(value));
}),
send: Props.ampMethod(function(privates, opt_data) {
+ privates.nativeCompleteEventSeen = false;
+
if (arguments.length === 0) {
// TODO(ihab.awad): send()-ing an empty string because send()
with no
// args does not work on FF3, others?
@@ -1080,12 +1094,10 @@ var Domado = (function() {
// Firefox does not call the 'onreadystatechange' handler in
// the case of a synchronous XHR. We simulate this behavior by
// calling the handler explicitly.
- if (privates.feral.overrideMimeType) {
- // This is Firefox
- if (!privates.async && privates.handler) {
- var evt = { target: this };
- privates.handler.call(void 0, evt);
- }
+ if (!privates.async && !privates.nativeCompleteEventSeen &&
+ privates.handler) {
+ var evt = { target: this };
+ privates.handler.call(void 0, evt);
}
}),
abort: Props.ampMethod(function(privates) {
Index: tests/com/google/caja/plugin/MainBrowserTest.java
diff --git a/tests/com/google/caja/plugin/MainBrowserTest.java
b/tests/com/google/caja/plugin/MainBrowserTest.java
index
0426fac50865e775fc5e4f1b6c2057543245b80b..6379cb58b343abafda2a50a27a8dd3294c0e6bcf
100644
--- a/tests/com/google/caja/plugin/MainBrowserTest.java
+++ b/tests/com/google/caja/plugin/MainBrowserTest.java
@@ -36,6 +36,8 @@ public class MainBrowserTest extends CatalogTestCase {
protected int waitForCompletionTimeout() {
if (entry.getLabel().startsWith("guest-scan-")) {
return 800 * 1000; // milliseconds
+ } else if (entry.getLabel().startsWith("preliminary-meta-")) {
+ return 30 * 1000;
} else {
return super.waitForCompletionTimeout();
}
Index: tests/com/google/caja/plugin/test-cajajs-invocation.js
diff --git a/tests/com/google/caja/plugin/test-cajajs-invocation.js
b/tests/com/google/caja/plugin/test-cajajs-invocation.js
index
9114f0962814300a9095cecd3b65f704f532cc36..d3f1b5b07180c652ddbfa4ad180b9495390e52f1
100644
--- a/tests/com/google/caja/plugin/test-cajajs-invocation.js
+++ b/tests/com/google/caja/plugin/test-cajajs-invocation.js
@@ -340,10 +340,10 @@
'var xhr = new XMLHttpRequest();' +
'try {' +
' xhr.open("GET", "' + location.protocol + '//' + location.host +
- '/nonexistent");' +
+ '/nonexistent", false);' + // Note sync XHR so test can be sync.
'} catch (e) { r("" + e); }' +
'xhr.onreadystatechange = function() {' +
- ' r(xhr.readyState + xhr.responseText);' +
+ ' r(xhr.readyState + " " + xhr.status);' +
'};' +
'xhr.send();' +
'</script>';
@@ -388,7 +388,7 @@
assertStringContains('http://fake2.url/foo', div.innerHTML);
// TODO(kpreid): verify script did not load, as expected
// XHR is independent of fetcher
- assertEquals('init,4<html>\n', String(xhrRes).substr(0, 13));
+ assertEquals('init,4 404', String(xhrRes).substr(0, 13));
jsunitPass('testBuilderApiNetNoFetch');
}));
}));
Index: tests/com/google/caja/plugin/test-domado-events-guest.html
diff --git a/tests/com/google/caja/plugin/test-domado-events-guest.html
b/tests/com/google/caja/plugin/test-domado-events-guest.html
index
b7a7df61c5d3ae2427ab5ad0f5f1422c649ca851..3a35a001acba99c6607f6826047fef3494625ec8
100644
--- a/tests/com/google/caja/plugin/test-domado-events-guest.html
+++ b/tests/com/google/caja/plugin/test-domado-events-guest.html
@@ -756,17 +756,27 @@ TODO(felix8a): fix javascript url handling
assertEquals('The quick brown fox', xhr.responseText);

// Check that a synchronous XHR invokes its callback in ready state 4
+ var statesSeen = [];
var responseText;
xhr = new window.XMLHttpRequest();
// Note we specify test id since we get preempted by xhr.send()
xhr.onreadystatechange = jsunitCallback(
function onreadystatechangecb() {
- if (xhr.readyState == 4) { responseText = xhr.responseText; }
+ statesSeen.push(xhr.readyState);
+ if (xhr.readyState == 4) {
+ responseText = xhr.responseText;
+ }
},
'testXhrSync');
xhr.open('GET', './xhrTest.txt', false);
+ // May or may not get a state 1 callback.
+ assertContains('' + statesSeen, ['', '1']);
+ statesSeen = [];
+
console.log('About to do sync xhr.send() - may get preempted');
xhr.send();
+
+ assertEquals('4', '' + statesSeen);
assertEquals('The quick brown fox', responseText);

pass('testXhrSync');

[eri...@gmail.com]

LGTM


https://codereview.appspot.com/251030043/diff/1/src/com/google/caja/plugin/domado.js
File src/com/google/caja/plugin/domado.js (right):

https://codereview.appspot.com/251030043/diff/1/src/com/google/caja/plugin/domado.js#newcode1052
src/com/google/caja/plugin/domado.js:1052: switch (arguments.length - 1)
{
Is this really clearer than switching on argument.length and
incrementing the case labels? If not, please do that.

https://codereview.appspot.com/251030043/

[kpr...@google.com]

https://codereview.appspot.com/251030043/diff/1/src/com/google/caja/plugin/domado.js
File src/com/google/caja/plugin/domado.js (right):

https://codereview.appspot.com/251030043/diff/1/src/com/google/caja/plugin/domado.js#newcode1052
src/com/google/caja/plugin/domado.js:1052: switch (arguments.length - 1)
{

On 2015/07/08 22:12:51, MarkM wrote:
> Is this really clearer than switching on argument.length and
incrementing the
> case labels? If not, please do that.

I think that the numbers currently used are more relevant to _what we
are implementing_ (XHR API) as opposed to the +1 numbers which contain
an implementation detail (the privates arg).

Do you disagree?

https://codereview.appspot.com/251030043/

[eri...@gmail.com]

Yes, for this case I agree. Still LGTM


https://codereview.appspot.com/251030043/