Revision: 5701
Author: [email protected]
Date: Mon Oct 13 19:19:17 2014 UTC
Log: Expand and refactor window.onerror support.
https://codereview.appspot.com/153290043
Guest window.onerror is now invoked for these error cases:
* top-level code (that is, <script>throw ...;</script>). Fixes
https://code.google.com/p/google-caja/issues/detail?id=1433.
* setTimeout, setInterval, or requestAnimationFrame callbacks.
The existing calls to onerror have been changed to use a single
implementation, domicile.handleUncaughtException. This function
takes care to not throw even if the error object or onerror handler
cause errors when used, and passes the full set of standardized
arguments (still with dummy values).
Also reorganized the setTimeout-and-friends implementation
(tameSetAndClear) so that the early-exit path is more direct.
[email protected]
https://code.google.com/p/google-caja/source/detail?r=5701
Modified:
/trunk/src/com/google/caja/plugin/domado.js
/trunk/src/com/google/caja/plugin/html-emitter.js
/trunk/src/com/google/caja/plugin/ses-frame-group.js
/trunk/tests/com/google/caja/plugin/jsunit.js
/trunk/tests/com/google/caja/plugin/test-cajajs-invocation.js
/trunk/tests/com/google/caja/plugin/test-domado-events-guest.html
=======================================
--- /trunk/src/com/google/caja/plugin/domado.js Fri Aug 15 19:38:48 2014 UTC
+++ /trunk/src/com/google/caja/plugin/domado.js Mon Oct 13 19:19:17 2014 UTC
@@ -1420,40 +1420,49 @@
* value which would be evaluated outside the sandbox.
*/
function tameSetAndClear(target, set, clear, setName, clearName,
passArg,
- evalStrings, environment) {
+ evalStrings, environment, handleUncaughtException) {
var ids = new WeakMap();
function tameSet(action, delayMillis) {
// Existing browsers treat a timeout/interval of null or undefined
as a
// noop.
- var id;
- if (action) {
- if (typeof action === 'function') {
- // OK
- } else if (evalStrings) {
- // Note specific ordering: coercion to string occurs at time of
- // call, syntax errors occur async.
- var code = '' + action;
- action = function callbackStringWrapper() {
- cajaVM.compileModule(code)(environment);
- };
- } else {
- // Early error for usability -- we also defend below.
- // This check is not *necessary* for security.
- throw new TypeError(
- setName + ' called with a ' + typeof action + '.'
- + ' Please pass a function instead of a string of
JavaScript');
- }
- // actionWrapper defends against:
- // * Passing a string-like object which gets taken as code.
- // * Non-standard arguments to the callback.
- // * Non-standard effects of callback's return value.
- var actionWrapper = passArg
- ? function(time) { action(+time); } // requestAnimationFrame
- : function() { action(); }; // setTimeout, setInterval
- id = set(actionWrapper, delayMillis | 0);
+ if (!action) {
+ return undefined;
+ }
+
+ // Convert action to a function.
+ if (typeof action === 'function') {
+ // OK
+ } else if (evalStrings) {
+ // Note specific ordering: coercion to string occurs at time of
+ // call, syntax errors occur async.
+ var code = '' + action;
+ action = function callbackStringWrapper() {
+ cajaVM.compileModule(code)(environment);
+ };
} else {
- id = undefined;
+ // Early error for usability -- we also defend below.
+ // This check is not *necessary* for security.
+ throw new TypeError(
+ setName + ' called with a ' + typeof action + '.'
+ + ' Please pass a function instead of a string of
JavaScript');
}
+
+ // actionWrapper defends against:
+ // * Passing a string-like object which gets taken as code.
+ // * Non-standard arguments to the callback.
+ // * Non-standard effects of callback's return value.
+ function actionWrapper(arg) {
+ try {
+ if (passArg) {
+ action(+arg); // requestAnimationFrame
+ } else {
+ action(); // setTimeout, setInterval
+ }
+ } catch (e) {
+ handleUncaughtException(e, '<setName callback>');
+ }
+ }
+ var id = set(actionWrapper, delayMillis | 0);
var tamed = {};
ids.set(tamed, id);
// Freezing is not *necessary*, but it makes testing/reasoning
simpler
@@ -3618,14 +3627,8 @@
try {
Function.prototype.call.call(func, thisArg, tameEventObj);
} catch (e) {
- try {
- tameWindow.onerror(
- e.message,
- '<' + tameEventObj.type + ' handler>', // better than
nothing
- 0);
- } catch (e2) {
- console.error('onerror handler failed\n', e, '\n', e2);
- }
+ domicile.handleUncaughtException(e,
+ '<' + tameEventObj.type + ' listener>');
}
}
@@ -6711,6 +6714,58 @@
}
return null;
});
+
+ /**
+ * Invoke the possibly guest-supplied onerror handler due to an
uncaught
+ * exception. This wrapper exists to ensure consistent behavior
among the
+ * many places we need "top-level" catches.
+ *
+ * handleUncaughtException attempts to never throw, even if the
onerror
+ * handler is not a function, stringifying the exception throws, and
so
+ * on.
+ *
+ * Usage:
+ * try {
+ * ...guest callback of some sort, say an event handler...
+ * } catch (e) {
+ * domicile.handleUncaughtException(e, 'event handler');
+ * }
+ *
+ * @param {Error} error
+ * @param {string} context Script source URL if available, otherwise
+ * a user-facing explanation of what kind of top-level handler
caught
+ * this error, in angle brackets; e.g. '<event listener>' or
+ * '<setTimeout>'.
+ */
+ domicile.handleUncaughtException =
+ cajaVM.constFunc(function(error, context) {
+ // This is an approximate implementation of
+ //
https://html.spec.whatwg.org/multipage/webappapis.html#runtime-script-errors
+ // The error event object is implicit.
+ try {
+ // Call with this == tameWindow; this is intended behavior.
+ // Refs for arguments:
+ //
https://html.spec.whatwg.org/multipage/webappapis.html#event-handler-attributes:onerroreventhandler
+ //
https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers.onerror
+ tameWindow.onerror(
+ 'Uncaught ' + error,
+ context,
+ // TODO(kpreid): Once there is a standardized error stack trace
+ // interface, use it to recover more error information if we
can.
+ -1, // line number
+ -1, // column number
+ error);
+ } catch (e) {
+ if (typeof console !== 'undefined') {
+ try {
+ console.error('Error while reporting guest script error: ',
e);
+ } catch (metaError) {
+ console.error('Error while reporting error while reporting '
+
+ 'guest script error. Sorry.');
+ }
+ }
+ }
+ });
// Taming of Styles:
@@ -6995,13 +7050,13 @@
window.setTimeout,
window.clearTimeout,
'setTimeout', 'clearTimeout',
- false, true, this);
+ false, true, this, domicile.handleUncaughtException);
tameSetAndClear(
this,
window.setInterval,
window.clearInterval,
'setInterval', 'clearInterval',
- false, true, this);
+ false, true, this, domicile.handleUncaughtException);
if (window.requestAnimationFrame) {
tameSetAndClear(
this,
@@ -7009,7 +7064,7 @@
return window.requestAnimationFrame(code); },
window.cancelAnimationFrame,
'requestAnimationFrame', 'cancelAnimationFrame',
- true, false, undefined);
+ true, false, undefined, domicile.handleUncaughtException);
}
}
inertCtor(TameWindow, Object, 'Window');
@@ -7351,12 +7406,14 @@
var domicile = windowToDomicile.get(imports);
var node = domicile.tameNode(thisNode);
var isUserAction = eventIsUserAction(event, window);
+ var tameEventObj = domicile.tameEvent(event);
try {
return dispatch(
isUserAction, pluginId, handler,
- [ node, domicile.tameEvent(event) ]);
+ [ node, tameEventObj ]);
} catch (ex) {
- imports.onerror(ex.message, 'unknown', 0);
+ domicile.handleUncaughtException(ex,
+ '<' + tameEventObj.type + ' handler>');
}
}
=======================================
--- /trunk/src/com/google/caja/plugin/html-emitter.js Mon Mar 17 02:52:38
2014 UTC
+++ /trunk/src/com/google/caja/plugin/html-emitter.js Mon Oct 13 19:19:17
2014 UTC
@@ -466,23 +466,16 @@
// TODO(kpreid): Include error message appropriately
domicile.fireVirtualEvent(scriptNode, 'Event', 'error');
- // Dispatch to the onerror handler.
- try {
- // TODO(kpreid): This should probably become a full event dispatch.
- // TODO: Should this happen inline or be dispatched out of band?
- opt_domicile.window.onerror(
- errorMessage,
- // URL where error was raised.
- // If this is an external load, then we need to use that URL,
- // but for inline scripts we maintain the illusion by using the
- // domicile.pseudoLocation.href which was passed here.
- scriptBaseUri,
- 1 // Line number where error was raised.
- // TODO: remap by inspection of the error if possible.
- );
- } catch (_) {
- // Ignore problems dispatching error.
- }
+ // TODO(kpreid): How should the virtual event on the node interact
with
+ // this handling? Should they actually be the same event and cancel
here?
+ opt_domicile.handleUncaughtException(
+ // TODO(kpreid): should have an Error instance here, not a
string.
+ errorMessage,
+ // URL where error was raised.
+ // If this is an external load, then we need to use that URL,
+ // but for inline scripts we maintain the illusion by using the
+ // domicile.pseudoLocation.href which was passed here.
+ scriptBaseUri);
return errorMessage;
}
=======================================
--- /trunk/src/com/google/caja/plugin/ses-frame-group.js Mon Aug 4
17:31:39 2014 UTC
+++ /trunk/src/com/google/caja/plugin/ses-frame-group.js Mon Oct 13
19:19:17 2014 UTC
@@ -278,7 +278,12 @@
}
Q.when(promise, function (compiledFunc) {
- var result = compiledFunc(imports);
+ var result = undefined;
+ try {
+ result = compiledFunc(imports);
+ } catch (e) {
+ gman.domicile.handleUncaughtException(e, gman.getUrl());
+ }
if (opt_runDone) {
opt_runDone(result);
}
=======================================
--- /trunk/tests/com/google/caja/plugin/jsunit.js Fri Aug 30 17:29:12 2013
UTC
+++ /trunk/tests/com/google/caja/plugin/jsunit.js Mon Oct 13 19:19:17 2014
UTC
@@ -239,6 +239,8 @@
/** Run tests. */
function jsunitRun(opt_testNames, opt_asyncEval) {
+ // TODO(kpreid): Arguments are incoherent. opt_testNames actually
represents
+ // varargs, so the second argument is used as part of that AND
opt_asyncEval.
if (jsunit.alreadyRan) { return; }
jsunit.alreadyRan = true;
=======================================
--- /trunk/tests/com/google/caja/plugin/test-cajajs-invocation.js Tue Oct
8 16:46:28 2013 UTC
+++ /trunk/tests/com/google/caja/plugin/test-cajajs-invocation.js Mon Oct
13 19:19:17 2014 UTC
@@ -92,6 +92,33 @@
}
});
+ jsunitRegister('testScriptError', function testScriptError() {
+ caja.load(createDiv(), uriPolicy, jsunitCallback(function(frame) {
+ var url = 'http://caja-test-error-page.invalid';
+ var onerrorFired = 0;
+ frame.code(
+ url,
+ 'text/javascript',
+ 'throw new Error("toplevel error");')
+ .api({
+ onerror: frame.tame(frame.markFunction(
+ jsunitCallback(function(msg, loc, line, col, error) {
+ onerrorFired++;
+ assertEquals('Uncaught Error: toplevel error', msg);
+ assertEquals(url, loc);
+ assertEquals(-1, line);
+ assertEquals(-1, col);
+ assertEquals('[object Error]',
+ Object.prototype.toString.call(error));
+ })))
+ })
+ .run(jsunitCallback(function(result) {
+ assertEquals(1, onerrorFired);
+ jsunitPass();
+ }));
+ }));
+ });
+
jsunitRegister('testDefaultHeight', function testDefaultHeight() {
var hostPageDiv = createDiv();
=======================================
--- /trunk/tests/com/google/caja/plugin/test-domado-events-guest.html Tue
Dec 10 23:52:12 2013 UTC
+++ /trunk/tests/com/google/caja/plugin/test-domado-events-guest.html Mon
Oct 13 19:19:17 2014 UTC
@@ -39,12 +39,12 @@
</p>
<script type="text/javascript">
window.die = function die() {
- window.onerror = (function (onerror) {
- return function (message, url, line) {
- assertEquals('died!', message);
+ window.onerror = (function (savedOnerror) {
+ return jsunitCallback(function (message, url, line) {
+ assertEquals('Uncaught Error: died!', message);
pass('testExceptionInEventHandler');
- window.onerror = onerror;
- };
+ window.onerror = savedOnerror;
+ }, 'testExceptionInEventHandler');
})(onerror);
throw new Error('died!');
@@ -451,6 +451,23 @@
});
</script>
+<p class="testcontainer" id="testTimeoutError">
+ testTimeoutError (error handling in setTimeout)
+</p>
+<script type="text/javascript">
+ jsunitRegister('testTimeoutError', function() {
+ setTimeout(function() { // NOT a jsunitCallback, must let error out
+ var savedOnerror = window.onerror;
+ window.onerror = jsunitCallback(function(message, url, line) {
+ assertEquals('Uncaught Error: foo', message);
+ pass();
+ window.onerror = savedOnerror;
+ }, 'testTimeoutError');
+ throw new Error('foo');
+ }, 0);
+ });
+</script>
+
<p class="testcontainer" id="testRequestAnimationFrame">
requestAnimationFrame<br>
</p>
--
---
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.
