Freeze globals successfully even on standards-conformant browsers. (issue 258110043 by erights@gmail.com)

[re...@codereview-hr.appspotmail.com]

Reviewers: kpreid_google,

Description:
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

Please review this at https://codereview.appspot.com/258110043/

Affected files (+284, -6 lines):
M src/com/google/caja/ses/startSES.js

[eri...@gmail.com]

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

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/startSES.js#newcode1595
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/startSES.js#newcode1620
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/startSES.js#newcode1658
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/startSES.js#newcode1662
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/startSES.js#newcode1717
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/startSES.js#newcode1729
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/startSES.js#newcode1761
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/startSES.js#newcode1764
src/com/google/caja/ses/startSES.js:1764: } catch (err) {}
Always insert a comment in an empty catch clause.

https://codereview.appspot.com/258110043/

[kpr...@google.com]

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

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/startSES.js#newcode263
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/startSES.js#newcode1523
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/startSES.js#newcode1533
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/startSES.js#newcode1535
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/startSES.js#newcode1597
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/startSES.js#newcode1658
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/startSES.js#newcode1667
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/startSES.js#newcode1729
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/startSES.js#newcode1736
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/startSES.js#newcode1806
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.

https://codereview.appspot.com/258110043/

[eri...@gmail.com]

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

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/startSES.js#newcode263
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/startSES.js#newcode1523
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/startSES.js#newcode1533
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/startSES.js#newcode1535
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/startSES.js#newcode1595
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/startSES.js#newcode1597
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/startSES.js#newcode1620
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/startSES.js#newcode1658
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/startSES.js#newcode1658
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/startSES.js#newcode1662
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/startSES.js#newcode1667
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/startSES.js#newcode1717
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/startSES.js#newcode1729
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/startSES.js#newcode1729
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/startSES.js#newcode1736
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/startSES.js#newcode1761
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/startSES.js#newcode1764
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/startSES.js#newcode1806
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.

https://codereview.appspot.com/258110043/

[kpr...@google.com]

LGTM

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

https://codereview.appspot.com/258110043/diff/1/src/com/google/caja/ses/startSES.js#newcode1597
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.

https://codereview.appspot.com/258110043/