Support external scripts and stylesheets loading in ES5

* Adds JSONP support to cajoling service to proxy a url and return it as a json object
* Modifies caja.js to provide a default jsonp based fetcher for external urls using doc.write
* Modifies html-sanitizer to make the sax parser restartable in some places (eg. at the end of a tag)
* Modifies html-emitter to stop processing tokens until content is returned by fetcher and sandboxing the fetched content
* Adds tests for external stylesheets and scripts

@4879

[metaweta, 2012-05-02 18:58:23]

initial comments

http://codereview.appspot.com/6060046/diff/5001/src/com/google/caja/plugin/ca...
File src/com/google/caja/plugin/caja.js (right):

http://codereview.appspot.com/6060046/diff/5001/src/com/google/caja/plugin/ca...
src/com/google/caja/plugin/caja.js:43: callback(result);
wrap this in a try...finally so that the GC always happens, then rethrow if
necessary.

http://codereview.appspot.com/6060046/diff/5001/src/com/google/caja/plugin/ca...
src/com/google/caja/plugin/caja.js:256: full.cache = partial.cache || {};
Any worries about __proto__ here?  Does guest code control URLs coming to this
object?

http://codereview.appspot.com/6060046/diff/5001/src/com/google/caja/plugin/ca...
src/com/google/caja/plugin/caja.js:441: function installSyncScript(name, url) {
Add a comment that urls need to be escaped before passing them in here, since
they're controlled by guest code.  The defaultFetcher is escaping the url, so
we're ok by default.

http://codereview.appspot.com/6060046/diff/5001/src/com/google/caja/plugin/gu...
File src/com/google/caja/plugin/guest-manager.js (right):

http://codereview.appspot.com/6060046/diff/5001/src/com/google/caja/plugin/gu...
src/com/google/caja/plugin/guest-manager.js:147: setTimeout(opt_runDone, 1000);
Why a full second?

http://codereview.appspot.com/6060046/diff/5001/src/com/google/caja/plugin/ht...
File src/com/google/caja/plugin/html-emitter.js (right):

http://codereview.appspot.com/6060046/diff/5001/src/com/google/caja/plugin/ht...
src/com/google/caja/plugin/html-emitter.js:418: return srcIndex ?
attribs[srcIndex] : undefined;
Off by 1?

[metaweta, 2012-05-02 19:04:20]

On 2012/05/02 18:58:23, metaweta wrote:
>
http://codereview.appspot.com/6060046/diff/5001/src/com/google/caja/plugin/ht...
> src/com/google/caja/plugin/html-emitter.js:418: return srcIndex ?
> attribs[srcIndex] : undefined;
> Off by 1?

Ah, no: you're returning the value at the odd position in the key/value pair. 
Never mind.

[Jasvir, 2012-05-03 18:01:42]

http://codereview.appspot.com/6060046/diff/5001/src/com/google/caja/plugin/ca...
File src/com/google/caja/plugin/caja.js (right):

http://codereview.appspot.com/6060046/diff/5001/src/com/google/caja/plugin/ca...
src/com/google/caja/plugin/caja.js:43: callback(result);
Replaced with a try-finally.

On 2012/05/02 18:58:23, metaweta wrote:
> wrap this in a try...finally so that the GC always happens, then rethrow if
> necessary.

http://codereview.appspot.com/6060046/diff/5001/src/com/google/caja/plugin/ca...
src/com/google/caja/plugin/caja.js:256: full.cache = partial.cache || {};
No longer needed.

On 2012/05/02 18:58:23, metaweta wrote:
> Any worries about __proto__ here?  Does guest code control URLs coming to this
> object?

http://codereview.appspot.com/6060046/diff/5001/src/com/google/caja/plugin/ca...
src/com/google/caja/plugin/caja.js:441: function installSyncScript(name, url) {
On 2012/05/02 18:58:23, metaweta wrote:
> Add a comment that urls need to be escaped before passing them in here, since
> they're controlled by guest code.  The defaultFetcher is escaping the url, so
> we're ok by default.

Done.

http://codereview.appspot.com/6060046/diff/5001/src/com/google/caja/plugin/gu...
File src/com/google/caja/plugin/guest-manager.js (right):

http://codereview.appspot.com/6060046/diff/5001/src/com/google/caja/plugin/gu...
src/com/google/caja/plugin/guest-manager.js:147: setTimeout(opt_runDone, 1000);
No longer needed.

On 2012/05/02 18:58:23, metaweta wrote:
> Why a full second?

http://codereview.appspot.com/6060046/diff/5001/src/com/google/caja/plugin/ht...
File src/com/google/caja/plugin/html-emitter.js (right):

http://codereview.appspot.com/6060046/diff/5001/src/com/google/caja/plugin/ht...
src/com/google/caja/plugin/html-emitter.js:418: return srcIndex ?
attribs[srcIndex] : undefined;
On 2012/05/02 18:58:23, metaweta wrote:
> Off by 1?

I do not believe so - the array is alternately key/value pairs.

[metaweta, 2012-05-07 23:05:18]

lgtm

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/d...
File src/com/google/caja/plugin/domado.js (right):

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:86: return;
Since you're returning the result of this function below in domicile.fetchUri,
please provide an explicit return value.

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/g...
File src/com/google/caja/plugin/guest-manager.js (right):

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/g...
src/com/google/caja/plugin/guest-manager.js:147: setTimeout(opt_runDone, 1000);
Add a comment explaining why this delay is necessary.

[ihab.awad, 2012-05-08 21:43:26]

Very clever! :)

lgtm++

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/c...
File src/com/google/caja/plugin/caja.js (right):

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:46: window[rndName] = function (result) {
Is it not possible to install the named callback function *only* in the 'window'
of the loader document, rather than installing it in the top-level window as
well? The way we have it right here, it'll pollute the host frame....

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:243: server = full['server'] = String(
This should be done in 'initialize' or something. resolveConfig didn't side
effect anything....

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:456: var loaderDocument;
Per our conversation, perhaps add tests where the container-supplied fetcher
uses XHR and <script> tag appendChild, to make sure we're not depending on some
feature of document.write-ing <script> HTML.

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:463: .replace(/[$]name/g, name))
This assignment pins the parent's handler function and, iiuc, this reference is
never cleared out.

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:579: return caja;
Why was it necessary to move this down? Felix's recent refactoring put the
'return' where it was because he wanted each module to look like "bunch of
imperative code" followed by "bunch of function declarations". I'm fine with
either style, personally -- just wondering why the change.

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/d...
File src/com/google/caja/plugin/domado.js (right):

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:91: naiveUriPolicy.fetch(undefined, mime,
callback);
What does fetching 'undefined' do exactly? What is this case doing?

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:4919: return
domicile.writeHook.apply(undefined, arguments);
I don't think document.write is *supposed* to return anything....

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/g...
File src/com/google/caja/plugin/guest-manager.js (right):

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/g...
src/com/google/caja/plugin/guest-manager.js:147: setTimeout(opt_runDone, 1000);
!!

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/h...
File src/com/google/caja/plugin/html-emitter.js (right):

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-emitter.js:391: function
defineUntrustedExternalStylesheet(url, marker, continuation) {
This function and the next have a lot of commonality that can be abstracted
away.

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-emitter.js:431: var pendingExternal = undefined;
I'm interested in the assumption that seems to be made here that documentWriter
methods are never going to be called reentrantly.

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-emitter.js:435: startDoc: function() {
documentLoaded = Q.defer(); },
Ok, and just to clarify to myself as I read this, the "Q" you are using here is
in the taming frame. Which means its asynchrony ultimately bottoms out in the
setTimeout() function of the taming frame.

Given that any cross-iframe invocations are happening within the same domain,
and therefore in the same event loop, things should compose without surprises.

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/h...
File src/com/google/caja/plugin/html-sanitizer.js (right):

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-sanitizer.js:325: if (h.startDoc && initial ==
0) { h.startDoc(param); }
Instead of this conditional every time -- if h.doSomething { ... } -- why not
just wrap 'h' with something that contains the conditionals, then from that
point on just use it without checking?

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/s...
File src/com/google/caja/plugin/ses-frame-group.js (right):

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/s...
src/com/google/caja/plugin/ses-frame-group.js:378: });
indentation

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/service/...
File src/com/google/caja/service/AbstractCajolingHandler.java (right):

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/service/...
src/com/google/caja/service/AbstractCajolingHandler.java:150: 
stray whitespace

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/service/...
src/com/google/caja/service/AbstractCajolingHandler.java:165: protected static
void renderAsJSON(
un-indent 2 sp

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/service/...
File src/com/google/caja/service/ProxyHandler.java (right):

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/service/...
src/com/google/caja/service/ProxyHandler.java:1: // Copyright 2009 Google Inc.
All Rights Reserved.
Copyright year

http://codereview.appspot.com/6060046/diff/15001/tests/com/google/caja/plugin...
File tests/com/google/caja/plugin/GeneralBrowserTest.java (right):

http://codereview.appspot.com/6060046/diff/15001/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/GeneralBrowserTest.java:205:
runTestCase("es53-test-external-script.html");
For consistency, please name this es53-test-external-script-guest.html. (Yes I
know es53-test-inline-script.html is not consistent -- we can change that now or
later.)

[Jasvir, 2012-05-15 22:15:35]

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/c...
File src/com/google/caja/plugin/caja.js (right):

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:46: window[rndName] = function (result) {
Not easily - the document.write blows away the loader frame.  I've marked this
as a TODO.

On 2012/05/08 21:43:26, ihab.awad wrote:
> Is it not possible to install the named callback function *only* in the
'window'
> of the loader document, rather than installing it in the top-level window as
> well? The way we have it right here, it'll pollute the host frame....

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:243: server = full['server'] = String(
On 2012/05/08 21:43:26, ihab.awad wrote:
> This should be done in 'initialize' or something. resolveConfig didn't side
> effect anything....

Done.

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/c...
src/com/google/caja/plugin/caja.js:463: .replace(/[$]name/g, name))
Marked as TODO.  Will fix as part of the loaderFrame CL.

On 2012/05/08 21:43:26, ihab.awad wrote:
> This assignment pins the parent's handler function and, iiuc, this reference
is
> never cleared out.

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/d...
File src/com/google/caja/plugin/domado.js (right):

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:86: return;
On 2012/05/07 23:05:18, metaweta wrote:
> Since you're returning the result of this function below in domicile.fetchUri,
> please provide an explicit return value.

Done.

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/d...
src/com/google/caja/plugin/domado.js:91: naiveUriPolicy.fetch(undefined, mime,
callback);
Calls the callback with undefined which logs an error.
On 2012/05/08 21:43:26, ihab.awad wrote:
> What does fetching 'undefined' do exactly? What is this case doing?

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/h...
File src/com/google/caja/plugin/html-emitter.js (right):

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-emitter.js:391: function
defineUntrustedExternalStylesheet(url, marker, continuation) {
On 2012/05/08 21:43:26, ihab.awad wrote:
> This function and the next have a lot of commonality that can be abstracted
> away.

Done.

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/h...
src/com/google/caja/plugin/html-emitter.js:431: var pendingExternal = undefined;
This is a false assumption and causes a failure - I've marked this with a TODO
and will fix in a later CL.

On 2012/05/08 21:43:26, ihab.awad wrote:
> I'm interested in the assumption that seems to be made here that
documentWriter
> methods are never going to be called reentrantly.

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/s...
File src/com/google/caja/plugin/ses-frame-group.js (right):

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/plugin/s...
src/com/google/caja/plugin/ses-frame-group.js:378: });
On 2012/05/08 21:43:26, ihab.awad wrote:
> indentation

Done.

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/service/...
File src/com/google/caja/service/AbstractCajolingHandler.java (right):

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/service/...
src/com/google/caja/service/AbstractCajolingHandler.java:150: 
On 2012/05/08 21:43:26, ihab.awad wrote:
> stray whitespace

Done.

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/service/...
src/com/google/caja/service/AbstractCajolingHandler.java:165: protected static
void renderAsJSON(
On 2012/05/08 21:43:26, ihab.awad wrote:
> un-indent 2 sp

Done.

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/service/...
File src/com/google/caja/service/ProxyHandler.java (right):

http://codereview.appspot.com/6060046/diff/15001/src/com/google/caja/service/...
src/com/google/caja/service/ProxyHandler.java:1: // Copyright 2009 Google Inc.
All Rights Reserved.
On 2012/05/08 21:43:26, ihab.awad wrote:
> Copyright year

Done.

[Jasvir, 2012-05-16 00:04:17]

http://codereview.appspot.com/6060046/diff/15001/tests/com/google/caja/plugin...
File tests/com/google/caja/plugin/GeneralBrowserTest.java (right):

http://codereview.appspot.com/6060046/diff/15001/tests/com/google/caja/plugin...
tests/com/google/caja/plugin/GeneralBrowserTest.java:205:
runTestCase("es53-test-external-script.html");
On 2012/05/08 21:43:26, ihab.awad wrote:
> For consistency, please name this es53-test-external-script-guest.html. (Yes I
> know es53-test-inline-script.html is not consistent -- we can change that now
or
> later.)

Done.