Sanitization should reject elements that we can't examine (e.g. embed/object on FF)

tools/dom/src/Validators.dart

Index: tools/dom/src/Validators.dart
diff --git a/tools/dom/src/Validators.dart b/tools/dom/src/Validators.dart
index ac51da13e6b62e1485283415d8d32f8647881883..dbc625e07d1699e7cb8acb44a7359dfd62bf59a7 100644
--- a/tools/dom/src/Validators.dart
+++ b/tools/dom/src/Validators.dart
@@ -175,24 +175,49 @@ class _ValidatingTreeSanitizer implements NodeTreeSanitizer {
     switch (node.nodeType) {
       case Node.ELEMENT_NODE:
         Element element = node;
-        if (element._hasCorruptedAttributes) {
-          window.console.warn('Removing element due to corrupted attributes on <${element}>');
+        // If the _hasCorruptedAttributes does not successfully return false,
+        // then we consider it corrupted and remove.
+        // TODO(alanknight): This is a workaround because on Firefox
+        // embed/object
+        // tags typeof is "function", not "object". We don't recognize them, and
+        // can't call methods. This does mean that you can't explicitly allow an
+        // embed tag. The only thing that will let it through is a null
+        // sanitizer that doesn't traverse the tree at all. But sanitizing while
+        // allowing embeds seems quite unlikely.
+        var corrupted = true;
+        var attrs;
+        var isAttr;
+        try {

[sra1, 2015/05/19 19:54:38]

Try to move try-catches to helper functions.
They inhibit optimizations in the entire containing function, both for dart2js
and then V8.

[Alan Knight, 2015/05/19 23:12:12]

Split it out into two additional functions, one which extracts the values in
try/catch blocks and it then calls the one that does the actual work, passing in
those values.

+          // If getting/indexing attributes throws, count that as corrupt.
+          attrs = element.attributes;
+          isAttr = attrs['is'];
+          corrupted = element._hasCorruptedAttributes;

[sra1, 2015/05/19 19:54:38]

It seems dangerous to store this information of element.
Can it be stored elsewhere?

[Alan Knight, 2015/05/19 21:54:55]

We're not storing it, it's a temp, and it's only used in the if statement below.
What do you mean?

[sra1, 2015/05/19 23:45:02]

Sorry, I thought that _hasCorruptedAttributes was a field.
I see it is a getter.

+        } catch(e) {}
+        var elementText = 'element unprintable';
+        try {
+          elementText = element.toString();
+        } catch(e) {}
+        var elementTagName = 'element tag unavailable';
+        try {
+          elementTagName = element.tagName;
+        } catch(e) {}
+        if (corrupted) {

[sra1, 2015/05/19 19:54:38]

corrupted could be anything from the assignment at 194.
Test
  if (false != corrupted) ..

[Alan Knight, 2015/05/19 23:12:12]

Done. But did it as (true = corrupted). I assume that's ok.

But how can it be anything if it's the result of calling a method with one
implementation which is return JS('bool', ...  ? We don't use the JS return
type?

[sra1, 2015/05/19 23:45:01]

Sorry, this again came from the misunderstanding that _hasCorruptedAttributes
was a field which could have any value.

It might be an idea to make _hasCorruptedAttributes be a static method to avoid
the need to dispatch (getInterceptor) to get to the code.

+          window.console.warn(
+              'Removing element due to corrupted attributes on <$elementText>');
           _removeNode(node, parent);
           break;
         }
-        var attrs = element.attributes;
         if (!validator.allowsElement(element)) {
           window.console.warn(
-              'Removing disallowed element <${element.tagName}>');
+              'Removing disallowed element <$elementTagName>');
           _removeNode(node, parent);
           break;
         }
 
-        var isAttr = attrs['is'];
         if (isAttr != null) {
           if (!validator.allowsAttribute(element, 'is', isAttr)) {
             window.console.warn('Removing disallowed type extension '
-                '<${element.tagName} is="$isAttr">');
+                '<$elementTagName is="$isAttr">');
             _removeNode(node, parent);
             break;
           }