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');
--
---
You received this message because you are subscribed to the Google Groups "Google Caja Discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
For more options, visit https://groups.google.com/d/optout.
