Add option to not mitigateGotchas for scripts that do not need it.

Adds a third option to the uri policy to give hosts the option to:
  * use an alternate, pre-gotcha-mitigated script for a given url
  * not perform any gotcha-mitigation for scripts like jquery which do not need it

(This improves the render time for scripts that use jquery by ~500ms on MBP OS 10.8.2 Chrome M24)

@5403

[MarkM, 2013-02-23 17:36:17]

https://codereview.appspot.com/7381051/diff/1/src/com/google/caja/ses/startSE...
File src/com/google/caja/ses/startSES.js (right):

https://codereview.appspot.com/7381051/diff/1/src/com/google/caja/ses/startSE...
src/com/google/caja/ses/startSES.js:652: function compileExpr(src,
doesNotNeedMitigation, opt_sourcePosition) {
I don't like this API. mitigateGotchas is already parameterized by an options
object, and is already conditioned on the presence of ses.mitigateGotchas. In
the absence of mitigation, this parameter here, if false, cannot be obeyed.
Also, phrasing boolean flags as a negative often leads to cognitive overhead,
such as the double-negative reasoning in the previous sentence.

I don't understand all the considerations that led to this API so I will not
suggest another one here. But please let's talk or chat before you submit this.

https://codereview.appspot.com/7381051/diff/1/src/com/google/caja/ses/startSE...
src/com/google/caja/ses/startSES.js:655: exprSrc = doesNotNeedMitigation ?
exprSrc : mitigateGotchas(exprSrc);
On the mitigateGotchas function's doccomment above it says "Passing no option
performs no mitigation.". However, I see that all actual calls to
mitigateGotchas, as here, in fact pass no options, so something is clearly
amiss.

[ihab.awad, 2013-02-28 07:26:51]

lgtm

https://codereview.appspot.com/7381051/diff/1/src/com/google/caja/demos/playg...
File src/com/google/caja/demos/playground/client/ui/PlaygroundView.java (right):

https://codereview.appspot.com/7381051/diff/1/src/com/google/caja/demos/playg...
src/com/google/caja/demos/playground/client/ui/PlaygroundView.java:541: return
null;
This returns null unconditionally -- leftover from debugging?

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

https://codereview.appspot.com/7381051/diff/1/src/com/google/caja/plugin/doma...
src/com/google/caja/plugin/domado.js:4775: subDomicile.htmlEmitterTarget,
naiveUriPolicy.cajole, subDomicile);
Not naiveUriPolicy.mitigate?

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

https://codereview.appspot.com/7381051/diff/1/src/com/google/caja/plugin/html...
src/com/google/caja/plugin/html-emitter.js:47: *     to load serverside
mitigated scripts (used only in SES)
It just rewrites URLs, right?

[Jasvir Nagra, 2013-05-01 11:04:26]

Adds a third option to the uri policy to give hosts the option to:
  * use an alternate, pre-gotcha-mitigated script for a given url
  * not perform any gotcha-mitigation for scripts like jquery which do not need
it

(This improves the render time for scripts that use jquery by ~500ms on MBP OS
10.8.2 Chrome M24)

[Jasvir Nagra, 2013-05-01 14:37:01]

Adds a third option to the uri policy to give hosts the option to:
  * use an alternate, pre-gotcha-mitigated script for a given url
  * not perform any gotcha-mitigation for scripts like jquery which do not need
it

(This improves the render time for scripts that use jquery by ~500ms on MBP OS
10.8.2 Chrome M24)

[Jasvir Nagra, 2013-05-01 14:43:33]

https://codereview.appspot.com/7381051/diff/1/src/com/google/caja/demos/playg...
File src/com/google/caja/demos/playground/client/ui/PlaygroundView.java (right):

https://codereview.appspot.com/7381051/diff/1/src/com/google/caja/demos/playg...
src/com/google/caja/demos/playground/client/ui/PlaygroundView.java:541: return
null;
On 2013/02/28 07:26:52, ihab.awad wrote:
> This returns null unconditionally -- leftover from debugging?

Done.

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

https://codereview.appspot.com/7381051/diff/1/src/com/google/caja/plugin/doma...
src/com/google/caja/plugin/domado.js:4775: subDomicile.htmlEmitterTarget,
naiveUriPolicy.cajole, subDomicile);
On 2013/02/28 07:26:52, ihab.awad wrote:
> Not naiveUriPolicy.mitigate?

Done.

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

https://codereview.appspot.com/7381051/diff/1/src/com/google/caja/plugin/html...
src/com/google/caja/plugin/html-emitter.js:47: *     to load serverside
mitigated scripts (used only in SES)
On 2013/02/28 07:26:52, ihab.awad wrote:
> It just rewrites URLs, right?

Done.

[ihab.awad, 2013-05-01 15:48:26]

https://codereview.appspot.com/7381051/diff/13001/src/com/google/caja/demos/p...
File src/com/google/caja/demos/playground/client/ui/PlaygroundView.java (right):

https://codereview.appspot.com/7381051/diff/13001/src/com/google/caja/demos/p...
src/com/google/caja/demos/playground/client/ui/PlaygroundView.java:541: // Skip
rewriting jquery and jqueryui when loaded from the google cdn
long line

https://codereview.appspot.com/7381051/diff/13001/src/com/google/caja/plugin/...
File src/com/google/caja/plugin/domado.js (right):

https://codereview.appspot.com/7381051/diff/13001/src/com/google/caja/plugin/...
src/com/google/caja/plugin/domado.js:5269: subDomicile.htmlEmitterTarget,
Indent 2 more spaces

https://codereview.appspot.com/7381051/diff/13001/src/com/google/caja/plugin/...
src/com/google/caja/plugin/domado.js:6230:
URI.utils.resolve(domicile.pseudoLocation.href, uri),
Indent 2 more spaces.

https://codereview.appspot.com/7381051/diff/13001/src/com/google/caja/plugin/...
src/com/google/caja/plugin/domado.js:6230:
URI.utils.resolve(domicile.pseudoLocation.href, uri),
Why was this resolution not being done before, and why is now the time to start
doing it? It looks reasonable enough, but I'm wondering what the backstory is.

https://codereview.appspot.com/7381051/diff/13001/src/com/google/caja/plugin/...
File src/com/google/caja/plugin/es53-frame-group.js (right):

https://codereview.appspot.com/7381051/diff/13001/src/com/google/caja/plugin/...
src/com/google/caja/plugin/es53-frame-group.js:305: undefined /* cajoling proxy
unused in es53 */, domicile, imports);
Long line.

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

https://codereview.appspot.com/7381051/diff/13001/src/com/google/caja/plugin/...
src/com/google/caja/plugin/html-emitter.js:49: *     fly (used only in SES)
It's not really a proxy because it doesn't actually fetch or intermediate any
content. It's more just a URI rewriter. I would not call it a "proxy" so as to
avoid confusion with real HTTP proxy type stuff.

https://codereview.appspot.com/7381051/diff/13001/src/com/google/caja/plugin/...
src/com/google/caja/plugin/html-emitter.js:55: 
Extra blank line.

https://codereview.appspot.com/7381051/diff/13001/src/com/google/caja/plugin/...
src/com/google/caja/plugin/html-emitter.js:58: opt_mitigatingProxy,
opt_domicile, opt_guestGlobal) {
Indent 2 spaces.

https://codereview.appspot.com/7381051/diff/13001/src/com/google/caja/plugin/...
src/com/google/caja/plugin/html-emitter.js:547: evaluateUntrustedScript(url,
src, true, doesNotNeedMitigation);
Passing 'doesNotNeedMitigation' as the value of formal 'opt_mitigate'?

https://codereview.appspot.com/7381051/diff/13001/src/com/google/caja/ses/sta...
File src/com/google/caja/ses/startSES.js (right):

https://codereview.appspot.com/7381051/diff/13001/src/com/google/caja/ses/sta...
src/com/google/caja/ses/startSES.js:734: function compileExpr(src,
doesNotNeedMitigation, opt_sourcePosition) {
Please change throughout to 'mitigate' with appropriate boolean sense. Otherwise
it's too confusing to keep track of everything.

https://codereview.appspot.com/7381051/diff/13001/tests/com/google/caja/plugi...
File tests/com/google/caja/plugin/Es5BrowserTest.java (right):

https://codereview.appspot.com/7381051/diff/13001/tests/com/google/caja/plugi...
tests/com/google/caja/plugin/Es5BrowserTest.java:34: public void
testCajolingProxy() throws Exception {
s/Cajole/Mitigate/ ?

https://codereview.appspot.com/7381051/diff/13001/tests/com/google/caja/plugi...
File tests/com/google/caja/plugin/es53-test-cajoling-proxy.js (right):

https://codereview.appspot.com/7381051/diff/13001/tests/com/google/caja/plugi...
tests/com/google/caja/plugin/es53-test-cajoling-proxy.js:19: * @requires caja,
jsunitRun, readyToTest
document, jsunitRegister

[ihab.awad, 2013-05-01 15:52:45]

Assuming fixing the boolean sense of "mitigate" and not calling it a mitigating
proxy -- if we don't fix confusing naming, it will remain 4eva in our codebase
-- this CL is lgtm++.

[MarkM, 2013-05-01 16:59:03]

Not LGTM. Please don't commit this until the API is improved as discussed in my
previous comments and verbally.

[Jasvir Nagra, 2013-05-01 17:53:08]

Adds a third option to the uri policy to give hosts the option to:
  * use an alternate, pre-gotcha-mitigated script for a given url
  * not perform any gotcha-mitigation for scripts like jquery which do not need
it

(This improves the render time for scripts that use jquery by ~500ms on MBP OS
10.8.2 Chrome M24)

[Jasvir Nagra, 2013-05-01 17:53:15]

https://codereview.appspot.com/7381051/diff/13001/src/com/google/caja/demos/p...
File src/com/google/caja/demos/playground/client/ui/PlaygroundView.java (right):

https://codereview.appspot.com/7381051/diff/13001/src/com/google/caja/demos/p...
src/com/google/caja/demos/playground/client/ui/PlaygroundView.java:541: // Skip
rewriting jquery and jqueryui when loaded from the google cdn
On 2013/05/01 15:48:26, ihab.awad wrote:
> long line

Done.

https://codereview.appspot.com/7381051/diff/13001/src/com/google/caja/plugin/...
File src/com/google/caja/plugin/domado.js (right):

https://codereview.appspot.com/7381051/diff/13001/src/com/google/caja/plugin/...
src/com/google/caja/plugin/domado.js:5269: subDomicile.htmlEmitterTarget,
On 2013/05/01 15:48:26, ihab.awad wrote:
> Indent 2 more spaces

Done.

https://codereview.appspot.com/7381051/diff/13001/src/com/google/caja/plugin/...
src/com/google/caja/plugin/domado.js:6230:
URI.utils.resolve(domicile.pseudoLocation.href, uri),
On 2013/05/01 15:48:26, ihab.awad wrote:
> Indent 2 more spaces.

Done.

https://codereview.appspot.com/7381051/diff/13001/src/com/google/caja/plugin/...
src/com/google/caja/plugin/domado.js:6230:
URI.utils.resolve(domicile.pseudoLocation.href, uri),
I just noticed it was not being done when I added a test for it.

https://codereview.appspot.com/7381051/diff/13001/src/com/google/caja/plugin/...
File src/com/google/caja/plugin/es53-frame-group.js (right):

https://codereview.appspot.com/7381051/diff/13001/src/com/google/caja/plugin/...
src/com/google/caja/plugin/es53-frame-group.js:305: undefined /* cajoling proxy
unused in es53 */, domicile, imports);
On 2013/05/01 15:48:26, ihab.awad wrote:
> Long line.

Done.

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

https://codereview.appspot.com/7381051/diff/13001/src/com/google/caja/plugin/...
src/com/google/caja/plugin/html-emitter.js:49: *     fly (used only in SES)
On 2013/05/01 15:48:26, ihab.awad wrote:
> It's not really a proxy because it doesn't actually fetch or intermediate any
> content. It's more just a URI rewriter. I would not call it a "proxy" so as to
> avoid confusion with real HTTP proxy type stuff.

Done.

https://codereview.appspot.com/7381051/diff/13001/src/com/google/caja/plugin/...
src/com/google/caja/plugin/html-emitter.js:55: 
On 2013/05/01 15:48:26, ihab.awad wrote:
> Extra blank line.

Done.

https://codereview.appspot.com/7381051/diff/13001/src/com/google/caja/plugin/...
src/com/google/caja/plugin/html-emitter.js:58: opt_mitigatingProxy,
opt_domicile, opt_guestGlobal) {
On 2013/05/01 15:48:26, ihab.awad wrote:
> Indent 2 spaces.

Done.

https://codereview.appspot.com/7381051/diff/13001/src/com/google/caja/plugin/...
src/com/google/caja/plugin/html-emitter.js:547: evaluateUntrustedScript(url,
src, true, doesNotNeedMitigation);
On 2013/05/01 15:48:26, ihab.awad wrote:
> Passing 'doesNotNeedMitigation' as the value of formal 'opt_mitigate'?

Done.

https://codereview.appspot.com/7381051/diff/13001/src/com/google/caja/ses/sta...
File src/com/google/caja/ses/startSES.js (right):

https://codereview.appspot.com/7381051/diff/13001/src/com/google/caja/ses/sta...
src/com/google/caja/ses/startSES.js:734: function compileExpr(src,
doesNotNeedMitigation, opt_sourcePosition) {
On 2013/05/01 15:48:26, ihab.awad wrote:
> Please change throughout to 'mitigate' with appropriate boolean sense.
Otherwise
> it's too confusing to keep track of everything.

Done.

https://codereview.appspot.com/7381051/diff/13001/tests/com/google/caja/plugi...
File tests/com/google/caja/plugin/Es5BrowserTest.java (right):

https://codereview.appspot.com/7381051/diff/13001/tests/com/google/caja/plugi...
tests/com/google/caja/plugin/Es5BrowserTest.java:34: public void
testCajolingProxy() throws Exception {
On 2013/05/01 15:48:26, ihab.awad wrote:
> s/Cajole/Mitigate/ ?

Done.

https://codereview.appspot.com/7381051/diff/13001/tests/com/google/caja/plugi...
File tests/com/google/caja/plugin/es53-test-cajoling-proxy.js (right):

https://codereview.appspot.com/7381051/diff/13001/tests/com/google/caja/plugi...
tests/com/google/caja/plugin/es53-test-cajoling-proxy.js:19: * @requires caja,
jsunitRun, readyToTest
On 2013/05/01 15:48:26, ihab.awad wrote:
> document, jsunitRegister

Done.

[Jasvir Nagra, 2013-05-01 18:00:29]

Adds a third option to the uri policy to give hosts the option to:
  * use an alternate, pre-gotcha-mitigated script for a given url
  * not perform any gotcha-mitigation for scripts like jquery which do not need
it

(This improves the render time for scripts that use jquery by ~500ms on MBP OS
10.8.2 Chrome M24)

[Jasvir Nagra, 2013-05-01 18:00:38]

https://codereview.appspot.com/7381051/diff/1/src/com/google/caja/ses/startSE...
File src/com/google/caja/ses/startSES.js (right):

https://codereview.appspot.com/7381051/diff/1/src/com/google/caja/ses/startSE...
src/com/google/caja/ses/startSES.js:652: function compileExpr(src,
doesNotNeedMitigation, opt_sourcePosition) {
Corrected the double negative in the body of the method but I'd like the default
(ie if no argument is provided) to be needsMitigation.

https://codereview.appspot.com/7381051/diff/1/src/com/google/caja/ses/startSE...
src/com/google/caja/ses/startSES.js:655: exprSrc = doesNotNeedMitigation ?
exprSrc : mitigateGotchas(exprSrc);
The doc comment is incorrect - if no options are provided, all mitigation is
performed.  Corrected the doc.

[MarkM, 2013-05-05 08:44:29]

Should note here what Jas and I chatted about. some mitigations must still be
mandatory, since Safari and some old Chromes still suffer from the Function
parsing bug https://code.google.com/p/google-caja/issues/detail?id=1616

This may not be the last time we find out that some mitigations must be
mandatory, so we should fix this to support future decisions about which
mitigations are mandatory on what browsers. This will be easier given Kevin's
suggestion that it should be up to SES clients which test failures they are
willing to tolerate, and how they wish to tolerate them.

[Jasvir Nagra, 2013-05-06 10:52:28]

Adds a third option to the uri policy to give hosts the option to:
  * use an alternate, pre-gotcha-mitigated script for a given url
  * not perform any gotcha-mitigation for scripts like jquery which do not need
it

(This improves the render time for scripts that use jquery by ~500ms on MBP OS
10.8.2 Chrome M24)

[Jasvir Nagra, 2013-05-06 22:48:03]

Ack.  I've updated the CL such that parsing is not optional even though the
rewriting is and merged the API for compileExpr and for mitigateGotchas.

[MarkM, 2013-05-07 01:33:45]

https://codereview.appspot.com/7381051/diff/41001/src/com/google/caja/ses/mit...
File src/com/google/caja/ses/mitigateGotchas.js (right):

https://codereview.appspot.com/7381051/diff/41001/src/com/google/caja/ses/mit...
src/com/google/caja/ses/mitigateGotchas.js:201: return (options && opt in
options) ? opt : defaultOption;
If opt is in options, this returns opt itself. From looking at how it is used, I
suspect this is not what you intend.

https://codereview.appspot.com/7381051/diff/41001/src/com/google/caja/ses/mit...
src/com/google/caja/ses/mitigateGotchas.js:213: resolved.parseProgram = true;
Please add a TODO that we should only mandate this if a to-be-added test in
repairES5.js indicates that this platform has the relevant problem.

https://codereview.appspot.com/7381051/diff/41001/src/com/google/caja/ses/sta...
File src/com/google/caja/ses/startSES.js (right):

https://codereview.appspot.com/7381051/diff/41001/src/com/google/caja/ses/sta...
src/com/google/caja/ses/startSES.js:844: function compileModule(modSrc,
doesNotNeedMitigation, opt_sourcePosition) {
Make the same change to compileModule that you made to compileExpr. Exactly the
same issues apply.

On the doc-comment, probably better to modify this one to delegate to
compileExpr rather than duplicating. But either way.

[ihab.awad, 2013-05-07 02:28:00]

lgtm++

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

https://codereview.appspot.com/7381051/diff/41001/src/com/google/caja/plugin/...
src/com/google/caja/plugin/html-emitter.js:541: parseProgram : true,
Hm.  Perhaps "parseProgram: false" here then override it for the bugfix in only
one place? But either way. I guess belt & suspenders can't hurt. :)

[Jasvir Nagra, 2013-05-08 16:40:45]

Adds a third option to the uri policy to give hosts the option to:
  * use an alternate, pre-gotcha-mitigated script for a given url
  * not perform any gotcha-mitigation for scripts like jquery which do not need
it

(This improves the render time for scripts that use jquery by ~500ms on MBP OS
10.8.2 Chrome M24)

[Jasvir Nagra, 2013-05-08 16:41:21]

https://codereview.appspot.com/7381051/diff/41001/src/com/google/caja/ses/mit...
File src/com/google/caja/ses/mitigateGotchas.js (right):

https://codereview.appspot.com/7381051/diff/41001/src/com/google/caja/ses/mit...
src/com/google/caja/ses/mitigateGotchas.js:201: return (options && opt in
options) ? opt : defaultOption;
On 2013/05/07 01:33:45, MarkM wrote:
> If opt is in options, this returns opt itself. From looking at how it is used,
I
> suspect this is not what you intend.

Done.

https://codereview.appspot.com/7381051/diff/41001/src/com/google/caja/ses/mit...
src/com/google/caja/ses/mitigateGotchas.js:201: return (options && opt in
options) ? opt : defaultOption;
On 2013/05/07 01:33:45, MarkM wrote:
> If opt is in options, this returns opt itself. From looking at how it is used,
I
> suspect this is not what you intend.

Done.

https://codereview.appspot.com/7381051/diff/41001/src/com/google/caja/ses/mit...
src/com/google/caja/ses/mitigateGotchas.js:213: resolved.parseProgram = true;
On 2013/05/07 01:33:45, MarkM wrote:
> Please add a TODO that we should only mandate this if a to-be-added test in
> repairES5.js indicates that this platform has the relevant problem.

Done.

https://codereview.appspot.com/7381051/diff/41001/src/com/google/caja/ses/sta...
File src/com/google/caja/ses/startSES.js (right):

https://codereview.appspot.com/7381051/diff/41001/src/com/google/caja/ses/sta...
src/com/google/caja/ses/startSES.js:844: function compileModule(modSrc,
doesNotNeedMitigation, opt_sourcePosition) {
On 2013/05/07 01:33:45, MarkM wrote:
> Make the same change to compileModule that you made to compileExpr. Exactly
the
> same issues apply.
> 
> On the doc-comment, probably better to modify this one to delegate to
> compileExpr rather than duplicating. But either way.

Done.

[MarkM, 2013-05-08 17:06:44]

Lgtm on ses/*. Leaving rest to others.