Repair Object.create on Chrome; make SES compatible with no Object.create(null).

In Chrome 28.0.1480.0 canary (among other versions),
Object.freeze(Object.prototype) causes __proto__ to lose its magic,
which causes various internal code to fail; most critically,
Object.create(X) produces objects whose [[Prototype]] is
Object.prototype rather than X. Repair this by reimplementing
Object.create from scratch.

The repair cannot support Object.create(null); therefore, kludge
SES-only code into working despite its absence:
* StringMap.js is willing to use {} instead of Object.create(null); safe
  because the keys are suffixed.
* Replace string-map uses of Object.create(null) in repairES5 with an
  explicit (lightened) StringMap abstraction.
* sharedImports and scope objects will have Object.prototype's
  properties as bindings.
* Loosen testProtoMention test to be willing for __proto__ as a global
  variable to throw errors.

Also repair Error.prototype.toString whose native implementation is
also broken by lack of __proto__ magic.

Note that there is further fatal-to-Caja misbehavior in the same Chrome
versions, with apparently the same root cause, only if the "Enable
Experimental JavaScript" flag is on (symptom: {}.constructor === Symbol
(itself an experimental feature), rather than Object), and this repair
does not deal with that.

@r5359

[ihab.awad, 2013-04-16 21:50:24]

lgtm

[kpreid2, 2013-04-16 21:51:58]

Forgot to add MarkM as SES reviewer.

[MarkM, 2013-04-16 22:06:55]

(experimental) "symptom: {}.constructor === Symbol
(itself an experimental feature), rather than Object"

Has this been reported? Please include issue link.

[kpreid2, 2013-04-16 22:09:14]

On 2013/04/16 22:06:55, MarkM wrote:
> (experimental) "symptom: {}.constructor === Symbol
> (itself an experimental feature), rather than Object"
> 
> Has this been reported? Please include issue link.

I found it plausible that this has the same root cause (something expects
Object.create to work, and because it doesn't, accidentally overwrites
Object.prototype). Do you know of a post-V8-bug-fix version of Chrome to verify
the independent existence of this bug?

[MarkM, 2013-04-16 22:20:38]

LGTM pending the examination of mstarzinger's fix that I suggest below. I would
be surprised (pleasantly!) if this CL survives that examination.

https://codereview.appspot.com/8806045/diff/1/src/com/google/caja/ses/repairE...
File src/com/google/caja/ses/repairES5.js (right):

https://codereview.appspot.com/8806045/diff/1/src/com/google/caja/ses/repairE...
src/com/google/caja/ses/repairES5.js:2880: function repair_OBJECT_CREATE() {
This looks very much like my earlier attempt at repair that I gave up on. The
problem is that, in the presence of this bug, many things besides Object.create
were also broken. In short, look for all the places that mstarzinger's fix to
this bug touched. If any of these are not fixed by this repair, and are fatal to
us when unrepaired, that would be bad.

https://codereview.appspot.com/8806045/diff/1/src/com/google/caja/ses/repairE...
src/com/google/caja/ses/repairES5.js:2887: // "1. If Type(O) is not Object or
Null throw a TypeError exception."
This spec language does not correspond to testing (typeof O !== 'object').
Instead, (typeof O !== 'object' && typeof O !== 'function') would be valid. Or,
if you check for null first, this test could be (O !== Object(O)). The second is
arguably more robust (even if less efficient) because it does not depend on
knowing all the typeof possibilities exhaustively.

[kpreid2, 2013-04-16 23:07:12]

In Chrome 28.0.1480.0 canary (among other versions),
Object.freeze(Object.prototype) causes __proto__ to lose its magic and,
more critically, Object.create(X) to produce objects whose [[Prototype]]
is Object.prototype rather than X. Repair this by reimplementing
Object.create from scratch.

The repair cannot support Object.create(null), but that's OK because
ES5/3 does not support it either.

Note that there is further fatal-to-Caja misbehavior in the same Chrome
versions, with apparently the same root cause, if the "Enable
Experimental JavaScript" flag is on (symptom: {}.constructor === Symbol
(itself an experimental feature), rather than Object), and this repair
does not deal with that.

[kpreid2, 2013-04-16 23:09:48]

New snapshot.

https://codereview.appspot.com/8806045/diff/1/src/com/google/caja/ses/repairE...
File src/com/google/caja/ses/repairES5.js (right):

https://codereview.appspot.com/8806045/diff/1/src/com/google/caja/ses/repairE...
src/com/google/caja/ses/repairES5.js:2880: function repair_OBJECT_CREATE() {
On 2013/04/16 22:20:38, MarkM wrote:
> This looks very much like my earlier attempt at repair that I gave up on. The
> problem is that, in the presence of this bug, many things besides 
> Object.create were also broken. In short, look for all the places that
> mstarzinger's fix to this bug touched. If any of these are not fixed by this
> repair, and are fatal to us when unrepaired, that would be bad.

I have run the full Caja tests on Chrome Canary and everything passes (except
for the unrelated
<https://code.google.com/p/google-caja/issues/detail?id=1703>), which is
evidence things will be OK.

Reviewing the V8 changes, <https://code.google.com/p/v8/source/detail?r=13817>
is merely modifying Object.prototype.__proto__ itself, and
<https://code.google.com/p/v8/source/detail?r=13815> touches the following
files:

apinatives.js:
This appears to be initializing the .prototype.[[Prototype]] chains of
lazily-constructed builtin constructors. I suspect that startSES's traversal
causes all of these to be instantiated before freezing happens; and failures
here would be likely to break tests.

full-codegen-*.cc:
This appears to affect __proto__ in object literals, which we do not use nor
expect our guests to be able to use -- but means that uses of such inside of
Chrome will be broken. I have reviewed such uses and they appear to be OK,
except for one use in the implementation of getOwnPropertyNames pertaining to
"interceptor properties", whatever those are.

array.js:
This looks like it will cause .sort() of a sparse array to ignore inherited
numeric properties. I think ideally we would consider this a SAFE_SPEC_VIOLATION
and maybe even repair it (by de-holeing arrays before sorting) later, but it
won't break us or most guest code.

d8.js:
Appears to be debugger code.

math.js:
Presumably runs before SES, and if it is in fact lazy it will be triggered by
startSES's traversal.

messages.js:
The worst this will do is cause Error objects to be misprinted; I haven't
followed the full call chain but it looks like it's for debugging and/or
cross-frame stuff, so normal in-one-frame semantics are likely preserved.

proxy.js:
A proxy derived [[Construct]] trap will fail to operate correctly; we don't use
proxies as ctors and don't expose real Proxy to guests, and we already know we
fail with Experimental JavaScript (which includes proxies) turned on.

v8natives.js:
One of these is Object.create which we repair, and the other appears to be early
initialization code.

So, in conclusion, the only visible effect to guests (other than in debugging
them) that I can see will be on sparse arrays, which nobody uses.

https://codereview.appspot.com/8806045/diff/1/src/com/google/caja/ses/repairE...
src/com/google/caja/ses/repairES5.js:2887: // "1. If Type(O) is not Object or
Null throw a TypeError exception."
On 2013/04/16 22:20:38, MarkM wrote:
> This spec language does not correspond to testing (typeof O !== 'object').
> Instead, (typeof O !== 'object' && typeof O !== 'function') would be valid.
> Or, if you check for null first, this test could be (O !== Object(O)). The 
> second is arguably more robust (even if less efficient) because it does not
> depend on knowing all the typeof possibilities exhaustively.

Done.

[MarkM, 2013-04-16 23:23:12]

Awesome good news!
LGTM

[kpreid2, 2013-04-16 23:43:34]

Correction: I seem to have run the tests on the wrong browser. :(

I haven't found new unrepaired problems, but I have found that we have several
dependencies on Object.create(null) inside of SES (it's very handy to have...).
I am currently looking into removing these dependencies.

[kpreid2, 2013-04-17 17:33:27]

In Chrome 28.0.1480.0 canary (among other versions),
Object.freeze(Object.prototype) causes __proto__ to lose its magic,
which causes various internal code to fail; most critically,
Object.create(X) produces objects whose [[Prototype]] is
Object.prototype rather than X. Repair this by reimplementing
Object.create from scratch.

The repair cannot support Object.create(null); therefore, kludge
SES-only code into working despite its absence:
* StringMap.js is willing to use {} instead of Object.create(null); safe
  because the keys are suffixed.
* Replace string-map uses of Object.create(null) in repairES5 with an
  explicit (lightened) StringMap abstraction.
* sharedImports and scope objects will have Object.prototype's
  properties as bindings.
* Loosen testProtoMention test to be willing for __proto__ as a global
  variable to throw errors.

Also repair Error.prototype.toString whose native implementation is
also broken by lack of __proto__ magic.

Note that there is further fatal-to-Caja misbehavior in the same Chrome
versions, with apparently the same root cause, only if the "Enable
Experimental JavaScript" flag is on (symptom: {}.constructor === Symbol
(itself an experimental feature), rather than Object), and this repair
does not deal with that.

[kpreid2, 2013-04-17 17:34:33]

On 2013/04/16 23:43:34, kpreid2 wrote:
> Correction: I seem to have run the tests on the wrong browser. :(

New snapshot removes Object.create(null) dependencies and fixes other small
problems. Tested ES5 mode on Chrome Beta; I will run full tests but I wanted to
get the review started.

[MarkM, 2013-04-17 18:21:11]

LGTM
Full speed ahead!

https://codereview.appspot.com/8806045/diff/13001/src/com/google/caja/ses/rep...
File src/com/google/caja/ses/repairES5.js (right):

https://codereview.appspot.com/8806045/diff/13001/src/com/google/caja/ses/rep...
src/com/google/caja/ses/repairES5.js:2118: 'Repaired Object.create can not
support Object.create(null)') {
If you're going to compare with a long sentence like this, make it a symbol
constant and refer to it from everywhere it needs to be the same.

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

https://codereview.appspot.com/8806045/diff/13001/src/com/google/caja/ses/sta...
src/com/google/caja/ses/startSES.js:564: // (that is, if they occur in
freeNames).
In which case they will obtain the values of these properties from the imports
object, which would now inherit them from Object.prototype. This is fine, but
contradicts the safety comment there.

[kpreid2, 2013-04-17 18:28:02]

In Chrome 28.0.1480.0 canary (among other versions),
Object.freeze(Object.prototype) causes __proto__ to lose its magic,
which causes various internal code to fail; most critically,
Object.create(X) produces objects whose [[Prototype]] is
Object.prototype rather than X. Repair this by reimplementing
Object.create from scratch.

The repair cannot support Object.create(null); therefore, kludge
SES-only code into working despite its absence:
* StringMap.js is willing to use {} instead of Object.create(null); safe
  because the keys are suffixed.
* Replace string-map uses of Object.create(null) in repairES5 with an
  explicit (lightened) StringMap abstraction.
* sharedImports and scope objects will have Object.prototype's
  properties as bindings.
* Loosen testProtoMention test to be willing for __proto__ as a global
  variable to throw errors.

Also repair Error.prototype.toString whose native implementation is
also broken by lack of __proto__ magic.

Note that there is further fatal-to-Caja misbehavior in the same Chrome
versions, with apparently the same root cause, only if the "Enable
Experimental JavaScript" flag is on (symptom: {}.constructor === Symbol
(itself an experimental feature), rather than Object), and this repair
does not deal with that.

[kpreid2, 2013-04-17 18:28:55]

https://codereview.appspot.com/8806045/diff/13001/src/com/google/caja/ses/rep...
File src/com/google/caja/ses/repairES5.js (right):

https://codereview.appspot.com/8806045/diff/13001/src/com/google/caja/ses/rep...
src/com/google/caja/ses/repairES5.js:2118: 'Repaired Object.create can not
support Object.create(null)') {
On 2013/04/17 18:21:11, MarkM wrote:
> If you're going to compare with a long sentence like this, make it a symbol
> constant and refer to it from everywhere it needs to be the same.

Done.

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

https://codereview.appspot.com/8806045/diff/13001/src/com/google/caja/ses/sta...
src/com/google/caja/ses/startSES.js:564: // (that is, if they occur in
freeNames).
On 2013/04/17 18:21:11, MarkM wrote:
> In which case they will obtain the values of these properties from the imports
> object, which would now inherit them from Object.prototype. This is fine, but
> contradicts the safety comment there.

I don't follow. The comment here is saying that the correct behavior will be
preserved here (due to overriding). The comment on imports is saying that it is
visibly incorrect (contains the extra properties), but safe because those
properties are ambiently available.

[MarkM, 2013-04-17 18:30:12]

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

https://codereview.appspot.com/8806045/diff/13001/src/com/google/caja/ses/sta...
src/com/google/caja/ses/startSES.js:564: // (that is, if they occur in
freeNames).
Nevermind then. Still LGTM ;).

On 2013/04/17 18:28:55, kpreid2 wrote:
> On 2013/04/17 18:21:11, MarkM wrote:
> > In which case they will obtain the values of these properties from the
imports
> > object, which would now inherit them from Object.prototype. This is fine,
but
> > contradicts the safety comment there.
> 
> I don't follow. The comment here is saying that the correct behavior will be
> preserved here (due to overriding). The comment on imports is saying that it
is
> visibly incorrect (contains the extra properties), but safe because those
> properties are ambiently available.

[ihab.awad, 2013-04-17 18:40:07]

lgtm!