Scanner rejects functions which always throw; fix revealed bugs.

The scanner now fails if all of its invocations of a given function
throw (and this is not expected); this catches bugs in the functions
and the scanner providing incorrect arguments. Other than this,

* Fixed setting <button>.type failing on Safari.
* ES5/3 implementation of Function.prototype.bind does not leave a
  .prototype value if deleting the property fails.
* ES5/3 now uses a consistent error message for
  'Property name may not end in double underscore.'
* Scanner: Added/fixed invocations to satisfy non-throwing requirement,
  notably including Array methods.
* Scanner: Added 'Ref' abstraction to generalize the argsBy* shortcuts.
  It is not yet used everywhere it ought to be.
* Scanner: Fix bug in handling of prototype methods from the taming
  frame where the prototype would be used as 'this' rather than an
  instance.

@r5569

[felix8a, 2013-08-16 23:07:10]

es53.js and domado.js look fine to me. still working through test-scan-guest.js

[felix8a, 2013-08-20 22:58:37]

about halfway through the test-scan-guest changes

https://codereview.appspot.com/13024043/diff/1/tests/com/google/caja/plugin/t...
File tests/com/google/caja/plugin/test-scan-guest.js (right):

https://codereview.appspot.com/13024043/diff/1/tests/com/google/caja/plugin/t...
tests/com/google/caja/plugin/test-scan-guest.js:412: forEach: function(f) {
this seems to me a very weird place to have a forEach. It ends up being called
only once, and it's not immediately obvious why other types of Refs don't have a
forEach, or why Ref.is does have a forEach.

maybe RefAnyFrame should be split into two functions, one that returns an Array
and another that returns a Ref based on the Array?

[kpreid2, 2013-08-21 18:27:22]

The scanner now fails if all of its invocations of a given function
throw (and this is not expected); this catches bugs in the functions
and the scanner providing incorrect arguments. Other than this,

* Fixed setting <button>.type failing on Safari.
* ES5/3 implementation of Function.prototype.bind does not leave a
  .prototype value if deleting the property fails.
* ES5/3 now uses a consistent error message for
  'Property name may not end in double underscore.'
* Scanner: Added/fixed invocations to satisfy non-throwing requirement,
  notably including Array methods.
* Scanner: Added 'Ref' abstraction to generalize the argsBy* shortcuts.
  It is not yet used everywhere it ought to be.
* Scanner: Fix bug in handling of prototype methods from the taming
  frame where the prototype would be used as 'this' rather than an
  instance.

[kpreid2, 2013-08-21 18:29:24]

new snapshot

https://codereview.appspot.com/13024043/diff/1/tests/com/google/caja/plugin/t...
File tests/com/google/caja/plugin/test-scan-guest.js (right):

https://codereview.appspot.com/13024043/diff/1/tests/com/google/caja/plugin/t...
tests/com/google/caja/plugin/test-scan-guest.js:412: forEach: function(f) {
On 2013/08/20 22:58:38, felix8a wrote:
> this seems to me a very weird place to have a forEach. It ends up being
> called only once, and it's not immediately obvious why other types of Refs
> don't have a  forEach, or why Ref.is does have a forEach.
>
> maybe RefAnyFrame should be split into two functions, one that returns an
> Array and another that returns a Ref based on the Array?

I was going to argue against 'called only once' as evidence because I
deliberately didn't fully refactor to use Ref everywhere simply to avoid large
merge conflicts with other scanner changes in my queue, but I now see that there
really is only one use of it. And multiple frames will go away when ES5/3 does.
So, changed as you suggest.

[felix8a, 2013-08-21 19:28:35]

lgtm

vague meta-concern: I find it very hard to audit the scanner rules meaningfully,
and I'm not sure what would help. it seems that there are two types of bugs we'd
care about:
1. wrongly marking a bad situation as ok.
2. failing to exercise a function in some meaningful way.
and I have little confidence I'll notice either type of bug. I think this is the
third or fourth time I've looked at the scanner in detail, and it hasn't really
gotten easier with practice.

https://codereview.appspot.com/13024043/diff/7001/tests/com/google/caja/plugi...
File tests/com/google/caja/plugin/test-scan-guest.js (right):

https://codereview.appspot.com/13024043/diff/7001/tests/com/google/caja/plugi...
tests/com/google/caja/plugin/test-scan-guest.js:1062: G.apply(function() {
return new RegExp('f(.*)o'); }));
maybe also interesting to have a regex with the /g flag?

[kpreid2, 2013-08-21 19:30:26]

https://codereview.appspot.com/13024043/diff/7001/tests/com/google/caja/plugi...
File tests/com/google/caja/plugin/test-scan-guest.js (right):

https://codereview.appspot.com/13024043/diff/7001/tests/com/google/caja/plugi...
tests/com/google/caja/plugin/test-scan-guest.js:1062: G.apply(function() {
return new RegExp('f(.*)o'); }));
On 2013/08/21 19:28:35, felix8a wrote:
> maybe also interesting to have a regex with the /g flag?

Yes, but in general the scanner does not attempt to exercise stateful interfaces
(at most, it generates fresh objects which are in a chosen state). So I think
that would be out of scope.

[kpreid2, 2013-08-21 19:38:04]

On 2013/08/21 19:28:35, felix8a wrote:
> lgtm
> 
> vague meta-concern: I find it very hard to audit the scanner rules
meaningfully,
> and I'm not sure what would help. it seems that there are two types of bugs
we'd
> care about:
> 1. wrongly marking a bad situation as ok.
> 2. failing to exercise a function in some meaningful way.
> and I have little confidence I'll notice either type of bug. I think this is
the
> third or fourth time I've looked at the scanner in detail, and it hasn't
really
> gotten easier with practice.

I agree — I see it as a messy solution to a messy problem. A couple of thoughts:

1. I'm planning to refactor to always group together one type of object's
annotations, rather than having the args separate from the frozenness and the
obtainInstance. (The 'Ref' thing is part of that — to have a uniform vocabulary
for pointing at objects, not tied to individual lookup tables.)

2. This particular change is actually about automatically detecting failure to
exercise a function — if all the calls to it throw, then the successful case is
not being exercised (or it is broken).

I'd love to hear any ideas for better, less error-prone language for expressing
the rules. (Note that one of the constraints is that we don't exercise anywhere
near all likely-interesting cases, because that would take too much time. A lot
of the rules could be a lot more precise, otherwise.)

[felix8a, 2013-08-21 19:38:08]

On 2013/08/21 19:30:26, kpreid2 wrote:
>
https://codereview.appspot.com/13024043/diff/7001/tests/com/google/caja/plugi...
> File tests/com/google/caja/plugin/test-scan-guest.js (right):
> 
>
https://codereview.appspot.com/13024043/diff/7001/tests/com/google/caja/plugi...
> tests/com/google/caja/plugin/test-scan-guest.js:1062: G.apply(function() {
> return new RegExp('f(.*)o'); }));
> On 2013/08/21 19:28:35, felix8a wrote:
> > maybe also interesting to have a regex with the /g flag?
> 
> Yes, but in general the scanner does not attempt to exercise stateful
interfaces
> (at most, it generates fresh objects which are in a chosen state). So I think
> that would be out of scope.

right, but I was thinking it's plausible for a function that takes a regex to
return a different type of result for a /g regex, and that might potentially be
useful to test.

[felix8a, 2013-08-21 19:44:56]

On 2013/08/21 19:38:04, kpreid2 wrote:
> On 2013/08/21 19:28:35, felix8a wrote:
> > lgtm
> > 
> > vague meta-concern: I find it very hard to audit the scanner rules
> meaningfully,
> > and I'm not sure what would help. it seems that there are two types of bugs
> we'd
> > care about:
> > 1. wrongly marking a bad situation as ok.
> > 2. failing to exercise a function in some meaningful way.
> > and I have little confidence I'll notice either type of bug. I think this is
> the
> > third or fourth time I've looked at the scanner in detail, and it hasn't
> really
> > gotten easier with practice.
> 
> I agree — I see it as a messy solution to a messy problem. A couple of
thoughts:
> 
> 1. I'm planning to refactor to always group together one type of object's
> annotations, rather than having the args separate from the frozenness and the
> obtainInstance. (The 'Ref' thing is part of that — to have a uniform
vocabulary
> for pointing at objects, not tied to individual lookup tables.)
> 
> 2. This particular change is actually about automatically detecting failure to
> exercise a function — if all the calls to it throw, then the successful case
is
> not being exercised (or it is broken).
> 
> I'd love to hear any ideas for better, less error-prone language for
expressing
> the rules. (Note that one of the constraints is that we don't exercise
anywhere
> near all likely-interesting cases, because that would take too much time. A
lot
> of the rules could be a lot more precise, otherwise.)

I'd be ok with a more exhaustive scanner that's run separately on demand. We
could setup a farm of browsers somewhere that run nothing but the scanner. Up to
1day-ish of scanning per browser seems plausible to me, since that would mesh ok
with daily build cycles. Under an hour is friendlier, but for what the scanner
is guarding against, several hours seems ok.

[kpreid2, 2013-08-22 18:15:59]

On 2013/08/21 19:44:56, felix8a wrote:
> I'd be ok with a more exhaustive scanner that's run separately on demand. We
> could setup a farm of browsers somewhere that run nothing but the scanner. Up
to
> 1day-ish of scanning per browser seems plausible to me, since that would mesh
ok
> with daily build cycles. Under an hour is friendlier, but for what the scanner
> is guarding against, several hours seems ok.

That would be nice to have, but:

1. We would still need to maintain the short list in addition to the long list.

2. From experience, there's a significant chance that a sufficiently large scan
will hang/crash the browser/tab, or simply run into slowdowns (i.e. performance
is worse than O(n)), and the scan cannot be simply split across multiple browser
runs, because it is traversing a highly cyclic graph without reliable unique
node labels.

(Given the ability to provoke crashes, I've even been giving some thought to
repurposing it for stress testing browsers.)