Revision: 5623
Author: [email protected]
Date: Tue Nov 5 20:07:08 2013 UTC
Log: Remove special handling of "simple" event handler attributes.
https://codereview.appspot.com/22060043
Since eval is now always available, the notion of simple event handlers
which are rewritten and executed without eval is unnecessary complexity,
which we now remove.
Note this has an additional cost: event handler attributes are compiled
and kept around in the domicile.handlers table and never deleted from
it, whereas simple event handlers would be stored inline in the
rewritten handler and thus not accumulate garbage if repeatedly created.
This could be avoided by storing the code inline as well (as a string)
and either recompiling each time the handler is invoked or using a
bounded cache.
This change affects only event handlers expressed as attributes
(<span onclick="..."> or el.setAttribute('onclick', ...)), not those
expressed as functions (el.onclick = function () {...}).
[email protected]
http://code.google.com/p/google-caja/source/detail?r=5623
Modified:
/trunk/src/com/google/caja/plugin/domado.js
/trunk/tests/com/google/caja/plugin/test-domado-forms-guest.html
=======================================
--- /trunk/src/com/google/caja/plugin/domado.js Mon Oct 28 23:24:18 2013 UTC
+++ /trunk/src/com/google/caja/plugin/domado.js Tue Nov 5 20:07:08 2013 UTC
@@ -1348,16 +1348,6 @@
var JS_SPACE = '\t\n\r ';
// An identifier that does not end with __.
var JS_IDENT = '(?:[a-zA-Z_][a-zA-Z0-9$_]*[a-zA-Z0-9$]|[a-zA-Z])_?';
- var SIMPLE_HANDLER_PATTERN = new RegExp(
- '^[' + JS_SPACE + ']*'
- + '(return[' + JS_SPACE + ']+)?' // Group 1 is present if it
returns.
- + '(' + JS_IDENT + ')[' + JS_SPACE + ']*' // Group 2 is a func
name.
- // Which can be passed optionally the event, and optionally this
node.
- + '\\((event' // Group 3 is present if 1 arg
- + '([' + JS_SPACE + ']*,[' + JS_SPACE + ']*this)?' //Group 4 if
2 arg
- + '[' + JS_SPACE + ']*)?\\)'
- // And it can end with a semicolon.
- + '[' + JS_SPACE + ']*(?:;?[' + JS_SPACE + ']*)$');
// These id patterns match the ones in HtmlAttributeRewriter.
@@ -1437,7 +1427,7 @@
if (action) {
if (typeof action === 'function') {
// OK
- } else if (evalStrings && cajaVM.compileModule) {
+ } else if (evalStrings) {
// Note specific ordering: coercion to string occurs at time of
// call, syntax errors occur async.
var code = '' + action;
@@ -2281,39 +2271,19 @@
return null;
case html4.atype.SCRIPT:
value = String(value);
- var fnNameExpr, doesReturn, argCount;
- var match = value.match(SIMPLE_HANDLER_PATTERN);
- if (match) {
- // Translate a handler that calls a simple function like
- // return foo(this, event)
- doesReturn = !!match[1];
- fnNameExpr = '"' + match[2] + '"';
- // safe because match[2] must be an identifier
- argCount =
- match[4] !== undefined ? 2 :
- match[3] !== undefined ? 1 : 0;
- } else if (cajaVM.compileExpr) {
- // Compile arbitrary handler code (only in SES mode)
- doesReturn = true;
- var handlerFn = cajaVM.compileExpr(
- '(function(event) { ' + value + ' })'
- )(tameWindow);
- fnNameExpr = domicile.handlers.push(handlerFn) - 1;
- argCount = 1;
- } else {
- if (typeof console !== 'undefined') {
- console.log('Cannot emulate complex event handler ' +
tagName +
- ' ' + attribName + '="' + value + '" in ES5/3 mode');
- }
- return "/*not supported*/";
- }
- var trustedHandler = (doesReturn ? 'return ' : '')
- + '___.plugin_dispatchEvent___('
- + 'this, event, ' + pluginId + ', '
- + fnNameExpr + ', ' + argCount + ');';
+
+ var handlerFn = cajaVM.compileExpr(
+ '(function(event) { ' + value + ' })'
+ )(tameWindow);
+ var fnNameExpr = domicile.handlers.push(handlerFn) - 1;
+
+ var trustedHandler = '___.plugin_dispatchEvent___(this,
event, ' +
+ pluginId + ', ' + fnNameExpr + ');';
if (attribName === 'onsubmit') {
trustedHandler =
'try { ' + trustedHandler + ' } finally { return false; }';
+ } else {
+ trustedHandler = 'return ' + trustedHandler;
}
return trustedHandler;
case html4.atype.URI:
@@ -3631,9 +3601,7 @@
ensureValidCallback(listener);
function wrapper(event) {
return plugin_dispatchEvent(
- thisNode, event, getId(tameWindow), listener,
- // indicate that we want an event argument only
- 1);
+ thisNode, event, getId(tameWindow), listener);
}
return wrapper;
}
@@ -4679,31 +4647,11 @@
// because it generates event-handler strings, not functions
--
// and for the TameWindow there is no real element to hang
those
// handler strings on. TODO(kpreid): refactor to fix that.
- if (cajaVM.compileExpr) { // ES5 case: eval available
- // Per
http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.html#event-handler-attributes
- tameWindow[attribName] = cajaVM.compileExpr(
- 'function cajaEventHandlerAttribFn_' + attribName +
- '(event) {\n' + value + '\n}')(tameWindow);
- } else {
- var match = value.match(SIMPLE_HANDLER_PATTERN);
- if (!match) { return; }
- //var doesReturn = match[1]; // not currently used
- var fnName = match[2];
- // TODO(kpreid): Synthesize a load event object.
- // Select appropriate arguments
- var wrapper;
- if (match[3] !== undefined) {
- wrapper = function() {
- tameWindow[fnName].call(this, {}, this); };
- } else if (match[2] !== undefined) {
- wrapper = function() {
- tameWindow[fnName].call(this, {}); };
- } else {
- wrapper = function() {
- tameWindow[fnName].call(this); };
- }
- tameWindow[attribName] = wrapper;
- }
+
+ // Per
http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.html#event-handler-attributes
+ tameWindow[attribName] = cajaVM.compileExpr(
+ 'function cajaEventHandlerAttribFn_' + attribName +
+ '(event) {\n' + value + '\n}')(tameWindow);
}
}))
}; }
@@ -7297,8 +7245,7 @@
/**
* Function called from rewritten event handlers to dispatch an event
safely.
*/
- function plugin_dispatchEvent(thisNode, event, pluginId, handler,
- argCount) {
+ function plugin_dispatchEvent(thisNode, event, pluginId, handler) {
var window = bridalMaker.getWindow(thisNode);
event = event || window.event;
// support currentTarget on IE[678]
@@ -7312,7 +7259,7 @@
try {
return dispatch(
isUserAction, pluginId, handler,
- [ node, domicile.tameEvent(event), node ].slice(0, argCount +
1));
+ [ node, domicile.tameEvent(event) ]);
} catch (ex) {
imports.onerror(ex.message, 'unknown', 0);
}
=======================================
--- /trunk/tests/com/google/caja/plugin/test-domado-forms-guest.html Tue
Oct 8 16:46:28 2013 UTC
+++ /trunk/tests/com/google/caja/plugin/test-domado-forms-guest.html Tue
Nov 5 20:07:08 2013 UTC
@@ -188,10 +188,12 @@
+ '<input type="submit" value="Submit"></form>';
// TODO(kpreid): This is matching implementation details. What do we
- // actually want to test for here?
+ // actually want to test for here? At least:
+ // * autocomplete="off"
+ // * try/finally forcing false return value
assertEquals('<form autocomplete="off" onsubmit="'
+ 'try { ___.plugin_dispatchEvent___'
- + '(this, event, 0, "foo", 0);'
+ + '(this, event, 0, 0);' // serial #s may change
+ ' } finally { return false; }" target="_blank">'
+ '<input autocomplete="off" type="submit"
value="Submit">'
+ '</form>',
--
---
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/groups/opt_out.
