Implement load events for <script>; fix existing script nodes getting rewritten.

* A <script> element evaluated in the sandbox now fires a 'load' event
  after successful eval and an 'error' event otherwise. No error
  details are yet included.
* Taming a <script> element which is already in the document does not
  replace its content with a caja_dynamic_script*___ hook. This includes
  script elements that are static HTML. The problematic behavior was
  introduced in r5358.

Supporting changes:
* The onerror= attribute is now whitelisted everywhere, per HTML5.
* Refactoring in html-emitter.js: resolveUntrustedExternal now
  passes its results to the continuation function rather than taking
  a separate handler function, and is no longer responsible for
  providing substitute content on error.

Fixes <https://code.google.com/p/google-caja/issues/detail?id=1763>.

@r5455

[felix8a, 2013-06-19 20:01:18]

lgtm

https://codereview.appspot.com/10370047/diff/1/src/com/google/caja/plugin/dom...
File src/com/google/caja/plugin/domado.js (right):

https://codereview.appspot.com/10370047/diff/1/src/com/google/caja/plugin/dom...
src/com/google/caja/plugin/domado.js:6151: postInitCreatedElement.call(tameEl); 
// Hook for <script>
not sure why this isn't inlined? if it's just about keeping related code
together, I'm not sure why postInitCreatedElement is a nodeAmp function rather
than just an ordinary function, since you already have the privates here

https://codereview.appspot.com/10370047/diff/1/src/com/google/caja/plugin/htm...
File src/com/google/caja/plugin/html-emitter.js (right):

https://codereview.appspot.com/10370047/diff/1/src/com/google/caja/plugin/htm...
src/com/google/caja/plugin/html-emitter.js:563: url, scriptNode, marker,
opt_continuation, delayed) {
bad form in general to have optional args before non-optional args. (and
jscompiler disallows it, but there are a lot of things we do that jscompiler
won't like, so that's not a real pressing concern).

https://codereview.appspot.com/10370047/diff/1/tests/com/google/caja/plugin/e...
File tests/com/google/caja/plugin/es53-test-external-script-guest.html (right):

https://codereview.appspot.com/10370047/diff/1/tests/com/google/caja/plugin/e...
tests/com/google/caja/plugin/es53-test-external-script-guest.html:115: if
(willRun) {
the willRun flag bothers me. this function is very specific to just two tests,
it's not more general than that, and generally I find that factoring out common
code for just 2 cases obscures logic more than helps. probably not worth
worrying about in this case.

[kpreid2, 2013-06-19 20:33:27]

https://codereview.appspot.com/10370047/diff/1/src/com/google/caja/plugin/dom...
File src/com/google/caja/plugin/domado.js (right):

https://codereview.appspot.com/10370047/diff/1/src/com/google/caja/plugin/dom...
src/com/google/caja/plugin/domado.js:6151: postInitCreatedElement.call(tameEl); 
// Hook for <script>
On 2013/06/19 20:01:19, felix8a wrote:
> not sure why this isn't inlined? if it's just about keeping related code
> together,

Yes. I think it is important to have all of the magic for a given element in one
place. (We don't fully stick to that today, and that's a bug.)

> I'm not sure why postInitCreatedElement is a nodeAmp function rather
> than just an ordinary function, since you already have the privates here

What we already have is the privates of the document, not the new element.
Otherwise I would agree and change it now.

https://codereview.appspot.com/10370047/diff/1/src/com/google/caja/plugin/htm...
File src/com/google/caja/plugin/html-emitter.js (right):

https://codereview.appspot.com/10370047/diff/1/src/com/google/caja/plugin/htm...
src/com/google/caja/plugin/html-emitter.js:563: url, scriptNode, marker,
opt_continuation, delayed) {
On 2013/06/19 20:01:19, felix8a wrote:
> bad form in general to have optional args before non-optional args. (and
> jscompiler disallows it, but there are a lot of things we do that jscompiler
> won't like, so that's not a real pressing concern).

continuation was already optional, just not marked as such.

https://codereview.appspot.com/10370047/diff/1/tests/com/google/caja/plugin/e...
File tests/com/google/caja/plugin/es53-test-external-script-guest.html (right):

https://codereview.appspot.com/10370047/diff/1/tests/com/google/caja/plugin/e...
tests/com/google/caja/plugin/es53-test-external-script-guest.html:115: if
(willRun) {
On 2013/06/19 20:01:19, felix8a wrote:
> the willRun flag bothers me. this function is very specific to just two tests,
> it's not more general than that, and generally I find that factoring out
common
> code for just 2 cases obscures logic more than helps. probably not worth
> worrying about in this case.

I disagree: there's a lot of subtlety in this checking of event dispatch, and
having even 15 lines of almost duplicated (but slightly different due to what
would be willRun) code is too much.

I think there are other places, too, in our existing test suite where our tests
are insufficiently rigorous, simply because we have no abstractions for testing
the correct behavior and so doing so in the specific case would be tedious.

[felix8a, 2013-06-19 20:50:18]

https://codereview.appspot.com/10370047/diff/1/tests/com/google/caja/plugin/e...
File tests/com/google/caja/plugin/es53-test-external-script-guest.html (right):

https://codereview.appspot.com/10370047/diff/1/tests/com/google/caja/plugin/e...
tests/com/google/caja/plugin/es53-test-external-script-guest.html:115: if
(willRun) {
On 2013/06/19 20:33:27, kpreid2 wrote:
> On 2013/06/19 20:01:19, felix8a wrote:
> > the willRun flag bothers me. this function is very specific to just two
tests,
> > it's not more general than that, and generally I find that factoring out
> common
> > code for just 2 cases obscures logic more than helps. probably not worth
> > worrying about in this case.
> 
> I disagree: there's a lot of subtlety in this checking of event dispatch, and
> having even 15 lines of almost duplicated (but slightly different due to what
> would be willRun) code is too much.
> 
> I think there are other places, too, in our existing test suite where our
tests
> are insufficiently rigorous, simply because we have no abstractions for
testing
> the correct behavior and so doing so in the specific case would be tedious.

I don't disagree that the common code is complex. I'm saying, the weird
specificity of "willRun" seems like maybe the wrong type of abstraction is being
applied. but again, this case is not worth worrying about.