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

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

BUG=
R=sra@google.com

Committed: https://github.com/dart-lang/sdk/commit/efb738f6335c89f91b796c80c53c19371b8b6e3b

[Alan Knight, 2015-05-19 17:50:37]

alanknight@google.com changed reviewers:
+ sra@google.com

[Alan Knight, 2015-05-19 17:50:37]

And in general, trust nothing about the element.

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

lgtm, esp if you can make the 'ok' path faster by moving all try-catch out into
separate function(s)

https://codereview.chromium.org/1146753004/diff/1/tools/dom/src/Validators.dart
File tools/dom/src/Validators.dart (right):

https://codereview.chromium.org/1146753004/diff/1/tools/dom/src/Validators.da...
tools/dom/src/Validators.dart:190: try {
Try to move try-catches to helper functions.
They inhibit optimizations in the entire containing function, both for dart2js
and then V8.

https://codereview.chromium.org/1146753004/diff/1/tools/dom/src/Validators.da...
tools/dom/src/Validators.dart:194: corrupted = element._hasCorruptedAttributes;
It seems dangerous to store this information of element.
Can it be stored elsewhere?

https://codereview.chromium.org/1146753004/diff/1/tools/dom/src/Validators.da...
tools/dom/src/Validators.dart:204: if (corrupted) {
corrupted could be anything from the assignment at 194.
Test
  if (false != corrupted) ..

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

https://codereview.chromium.org/1146753004/diff/1/tools/dom/src/Validators.dart
File tools/dom/src/Validators.dart (right):

https://codereview.chromium.org/1146753004/diff/1/tools/dom/src/Validators.da...
tools/dom/src/Validators.dart:194: corrupted = element._hasCorruptedAttributes;
On 2015/05/19 19:54:38, sra1 wrote:
> It seems dangerous to store this information of element.
> Can it be stored elsewhere?

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

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

PTAL

https://codereview.chromium.org/1146753004/diff/1/tools/dom/src/Validators.dart
File tools/dom/src/Validators.dart (right):

https://codereview.chromium.org/1146753004/diff/1/tools/dom/src/Validators.da...
tools/dom/src/Validators.dart:190: try {
On 2015/05/19 19:54:38, sra1 wrote:
> Try to move try-catches to helper functions.
> They inhibit optimizations in the entire containing function, both for dart2js
> and then V8.

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.

https://codereview.chromium.org/1146753004/diff/1/tools/dom/src/Validators.da...
tools/dom/src/Validators.dart:204: if (corrupted) {
On 2015/05/19 19:54:38, sra1 wrote:
> corrupted could be anything from the assignment at 194.
> Test
>   if (false != corrupted) ..

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:02]

lgtm.

Other questions about the loop are puzzles with the existing code so I'm not too
worried about them.

https://codereview.chromium.org/1146753004/diff/1/tools/dom/src/Validators.dart
File tools/dom/src/Validators.dart (right):

https://codereview.chromium.org/1146753004/diff/1/tools/dom/src/Validators.da...
tools/dom/src/Validators.dart:194: corrupted = element._hasCorruptedAttributes;
On 2015/05/19 21:54:55, Alan Knight wrote:
> On 2015/05/19 19:54:38, sra1 wrote:
> > It seems dangerous to store this information of element.
> > Can it be stored elsewhere?
> 
> We're not storing it, it's a temp, and it's only used in the if statement
below.
> What do you mean?

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

https://codereview.chromium.org/1146753004/diff/1/tools/dom/src/Validators.da...
tools/dom/src/Validators.dart:204: if (corrupted) {
On 2015/05/19 23:12:12, Alan Knight wrote:
> On 2015/05/19 19:54:38, sra1 wrote:
> > corrupted could be anything from the assignment at 194.
> > Test
> >   if (false != corrupted) ..
> 
> 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?

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.

https://codereview.chromium.org/1146753004/diff/20001/tools/dom/src/Validator...
File tools/dom/src/Validators.dart (right):

https://codereview.chromium.org/1146753004/diff/20001/tools/dom/src/Validator...
tools/dom/src/Validators.dart:164: // If we have the parent, it's presumably
already passed more sanitization or
line length

https://codereview.chromium.org/1146753004/diff/20001/tools/dom/src/Validator...
tools/dom/src/Validators.dart:236: for (var i = attrs.length - 1; i >= 0; --i) {
Why not keys.length?
Why do we go backwards?
It looks to me like one could just write:

for (var name in attrs.keys.toList()) {
  ...
}

[Alan Knight, 2015-05-21 20:38:41]

https://codereview.chromium.org/1146753004/diff/20001/tools/dom/src/Validator...
File tools/dom/src/Validators.dart (right):

https://codereview.chromium.org/1146753004/diff/20001/tools/dom/src/Validator...
tools/dom/src/Validators.dart:164: // If we have the parent, it's presumably
already passed more sanitization or
On 2015/05/19 23:45:02, sra1 wrote:
> line length

Done.

https://codereview.chromium.org/1146753004/diff/20001/tools/dom/src/Validator...
tools/dom/src/Validators.dart:236: for (var i = attrs.length - 1; i >= 0; --i) {
On 2015/05/19 23:45:02, sra1 wrote:
> Why not keys.length?
> Why do we go backwards?
> It looks to me like one could just write:
> 
> for (var name in attrs.keys.toList()) {
>   ...
> }

I don't know. But it makes me a little nervous that the code is so contrived.
There's either a subtle reason or it's just silly, but I don't know which, so
I'm reluctant to change it.

https://codereview.chromium.org/1146753004/diff/40001/tools/dom/templates/htm...
File tools/dom/templates/html/impl/impl_Element.darttemplate (right):

https://codereview.chromium.org/1146753004/diff/40001/tools/dom/templates/htm...
tools/dom/templates/html/impl/impl_Element.darttemplate:1417: static bool
_hasCorruptedAttributes(Element element) => false;
Made this static, as suggested.

[Alan Knight, 2015-05-21 20:54:55]

Committed patchset #3 (id:40001) manually as
efb738f6335c89f91b796c80c53c19371b8b6e3b (presubmit successful).