Freeze globals successfully even on standards-conformant browsers.

Prior to this CL, SES assumes it can freeze a property on the global
object straightforwardly, by doing 
Object.defineProperty(.., .., {.., configurable: false})

However, because of the browser split between Window and WindowProxy,
any browser that allows this breaks the ES6 invariants. Instead, on
browsers that implement the draft spec
https://github.com/domenic/window-proxy-spec
it is still possible to freeze these properties by roundabout
means. At the time of this writing, FF Nightly is the only browser
that obeys part of this new behavior though it does not implement all
of it. This CL changes the logic of freezing global properties so that
it works on 

  * browsers implementing the old behavior (currently, all but FF
    Nightly)
  * FF Nightly
  * browsers implementing the new draft spec (which do not exist,
    making this assertion hard to test)
  * non-browsers, where there is no split of the global object.

Fixes 
https://github.com/google/caja/issues/1974
See
https://esdiscuss.org/topic/figuring-out-the-behavior-of-windowproxy-in-the-face-of-non-configurable-properties
https://esdiscuss.org/topic/a-dom-use-case-that-can-t-be-emulated-with-direct-proxies
https://github.com/domenic/window-proxy-spec

[MarkM, 2015-08-04 18:23:35]

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

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:1595: * <i>name</i> property in all three
cases. For the legacy or simple
delete "three"

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:1620: *     case above, so we'll go ahead
and first try to delete wp.f
delete "so"

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:1658: // Might be specced, simple, legacy,
or mized
"mized" --> "mixed"

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:1662: // In case it is simple or legacy,
then we try to freeze it
delete "then"

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:1717: return;
Delete "return;". Not needed with this control flow.

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:1729: * <p>Since we can't use
getOwnPropertyDescriptor to see if we
Delete "Since"

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:1761: } catch (err) {}
Always insert a comment in an empty catch clause.

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:1764: } catch (err) {}
Always insert a comment in an empty catch clause.

[kpreid_google, 2015-08-05 17:46:53]

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

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:263: * the time of this writing, with
TRY_GLOBAL_SIMPLE_FREEZE_FIRST
Say "as of 2015-08-05" instead of "at the time of this writing". (The
information is of course available through history, but giving a timestamp makes
staleness instantly understandable.)

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:1523: * Although, in the official spec
language, only objects are
I suggest swapping these two sentences and putting the new second one in
parentheses. This way, the comment begins with what it's actually about.

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:1533: * the EcmaScript notion of "global
object" is split into two
capitalization: "ECMAScript"

Do you mean to say it is split _in browsers_ into ...?

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:1535: * from one url to another, a fresh
realm (set of primordials) is
"URL"

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:1597: * <tt>Object.defineProperty</tt>.
use <code> not <tt> (here and below).

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:1658: // Might be specced, simple, legacy,
or mized
Can be one sentence per line. Rewrap so it is.

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:1667: if ('document' in global) {
Extract this test into a function returning boolean, so it's less likely to be
reinvented or copied, and the comments on how it's a bad test can go in there
and not distract from what this is doing.

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:1729: * <p>Since we can't use
getOwnPropertyDescriptor to see if we
Explain why we can't, at least as "See comments on freezeGlobalProp".

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:1736: if (!(ses.is(oldValue, desc.value))) {
This will give a false negative if the property's value is undefined.

This will throw an unhelpful error if the property does not exist (because desc
is not an object).

Parentheses are unnecessary.

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:1806: var newDesc = {
Inline this literal into the defProp statement below, so that it can be read in
context of it being the descriptor for sharedImports.

[MarkM, 2015-08-05 21:25:21]

Prior to this CL, SES assumes it can freeze a property on the global
object straightforwardly, by doing 
Object.defineProperty(.., .., {.., configurable: false})

However, because of the browser split between Window and WindowProxy,
any browser that allows this breaks the ES6 invariants. Instead, on
browsers that implement the draft spec
https://github.com/domenic/window-proxy-spec
it is still possible to freeze these properties by roundabout
means. At the time of this writing, FF Nightly is the only browser
that obeys part of this new behavior though it does not implement all
of it. This CL changes the logic of freezing global properties so that
it works on 

  * browsers implementing the old behavior (currently, all but FF
    Nightly)
  * FF Nightly
  * browsers implementing the new draft spec (which do not exist,
    making this assertion hard to test)
  * non-browsers, where there is no split of the global object.

Fixes 
https://github.com/google/caja/issues/1974
See
https://esdiscuss.org/topic/figuring-out-the-behavior-of-windowproxy-in-the-f...
https://esdiscuss.org/topic/a-dom-use-case-that-can-t-be-emulated-with-direct...
https://github.com/domenic/window-proxy-spec

[MarkM, 2015-08-05 21:28:49]

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

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:263: * the time of this writing, with
TRY_GLOBAL_SIMPLE_FREEZE_FIRST
On 2015/08/05 17:46:53, kpreid_google wrote:
> Say "as of 2015-08-05" instead of "at the time of this writing". (The
> information is of course available through history, but giving a timestamp
makes
> staleness instantly understandable.)

Done.

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:1523: * Although, in the official spec
language, only objects are
On 2015/08/05 17:46:53, kpreid_google wrote:
> I suggest swapping these two sentences and putting the new second one in
> parentheses. This way, the comment begins with what it's actually about.

Done.

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:1533: * the EcmaScript notion of "global
object" is split into two
On 2015/08/05 17:46:53, kpreid_google wrote:
> capitalization: "ECMAScript"
> 
> Do you mean to say it is split _in browsers_ into ...?

Done.

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:1535: * from one url to another, a fresh
realm (set of primordials) is
On 2015/08/05 17:46:53, kpreid_google wrote:
> "URL"

Done.

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:1595: * <i>name</i> property in all three
cases. For the legacy or simple
On 2015/08/04 18:23:35, MarkM wrote:
> delete "three"

Done.

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:1597: * <tt>Object.defineProperty</tt>.
On 2015/08/05 17:46:53, kpreid_google wrote:
> use <code> not <tt> (here and below).

Done. But why? "<tt>" is much less noisy in the source.

(Makes we wonder whether we should switch to markdown, but of course not in this
CL.)

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:1620: *     case above, so we'll go ahead
and first try to delete wp.f
On 2015/08/04 18:23:34, MarkM wrote:
> delete "so"

Done.

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:1658: // Might be specced, simple, legacy,
or mized
On 2015/08/04 18:23:34, MarkM wrote:
> "mized" --> "mixed"

Done.

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:1658: // Might be specced, simple, legacy,
or mized
On 2015/08/05 17:46:53, kpreid_google wrote:
> Can be one sentence per line. Rewrap so it is.

Done.

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:1662: // In case it is simple or legacy,
then we try to freeze it
On 2015/08/04 18:23:34, MarkM wrote:
> delete "then"

Done.

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:1667: if ('document' in global) {
On 2015/08/05 17:46:53, kpreid_google wrote:
> Extract this test into a function returning boolean, so it's less likely to be
> reinvented or copied, and the comments on how it's a bad test can go in there
> and not distract from what this is doing.

Done. We were testing whether we're in a browser in two places in repairES5.js
as well, which were using slightly different tests. I consolidated all these to
use the new ses.isInBrowser() test.

In adding ses.isInBrowser to the "// * provides" and "// * requires" lists at
the top of repairES5.js and startSES.js, I noticed various anomalies there which
I also cleaned up.

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:1717: return;
On 2015/08/04 18:23:34, MarkM wrote:
> Delete "return;". Not needed with this control flow.

Done.

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:1729: * <p>Since we can't use
getOwnPropertyDescriptor to see if we
On 2015/08/04 18:23:34, MarkM wrote:
> Delete "Since"

Done.

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:1729: * <p>Since we can't use
getOwnPropertyDescriptor to see if we
On 2015/08/05 17:46:53, kpreid_google wrote:
> Explain why we can't, at least as "See comments on freezeGlobalProp".

Done.

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:1736: if (!(ses.is(oldValue, desc.value))) {
On 2015/08/05 17:46:53, kpreid_google wrote:
> This will give a false negative if the property's value is undefined.
> 
> This will throw an unhelpful error if the property does not exist (because
desc
> is not an object).
> 
> Parentheses are unnecessary.

Done.

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:1761: } catch (err) {}
On 2015/08/04 18:23:34, MarkM wrote:
> Always insert a comment in an empty catch clause.

Done.

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:1764: } catch (err) {}
On 2015/08/04 18:23:34, MarkM wrote:
> Always insert a comment in an empty catch clause.

Done.

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:1806: var newDesc = {
On 2015/08/05 17:46:53, kpreid_google wrote:
> Inline this literal into the defProp statement below, so that it can be read
in
> context of it being the descriptor for sharedImports.

Done.

[kpreid_google, 2015-08-06 19:39:33]

LGTM

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

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/start...
src/com/google/caja/ses/startSES.js:1597: * <tt>Object.defineProperty</tt>.
On 2015/08/05 21:28:49, MarkM wrote:
> On 2015/08/05 17:46:53, kpreid_google wrote:
> > use <code> not <tt> (here and below).
> 
> Done. But why? "<tt>" is much less noisy in the source.

<code>: This is a fragment of code.

<tt>: For some unspecified reason, we wish this to be presented in a monospace
font.