Repair Object.create() & numeric properties bug in IE.

The bug in Internet Explorer described at
http://webreflection.blogspot.co.uk/2014/04/all-ie-objects-are-broken.html
is now known as NUMERIC_PROPERTIES_INVISIBLE and repaired fully by SES.

This issue does not affect security of previous versions of Caja as we
do not depend on correct answers from reflection on existing objects;
we are repairing it solely to provide a more correct SES environment.

@r5681

[MarkM, 2014-05-05 21:28:41]

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

https://codereview.appspot.com/91140043/diff/1/src/com/google/caja/ses/repair...
src/com/google/caja/ses/repairES5.js:4387: canRepair: false,
Shouldn't this be
canRepair: true,
?

[kpreid_google, 2014-05-05 21:33:16]

The bug in Internet Explorer described at
http://webreflection.blogspot.co.uk/2014/04/all-ie-objects-are-broken.html
is now known as NUMERIC_PROPERTIES_INVISIBLE and repaired fully by SES.

This issue does not affect security of previous versions of Caja as we
do not depend on correct answers from reflection on existing objects;
we are repairing it solely to provide a more correct SES environment.

[kpreid_google, 2014-05-05 21:34:18]

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

https://codereview.appspot.com/91140043/diff/1/src/com/google/caja/ses/repair...
src/com/google/caja/ses/repairES5.js:4387: canRepair: false,
On 2014/05/05 21:28:40, MarkM wrote:
> Shouldn't this be
> canRepair: true,
> ?

Done. Forgot to update it while I was entering this problem record based on
another one.

[MarkM, 2014-05-05 21:39:25]

LGTM

[andrea.giammarchi, 2014-05-06 00:13:46]

On 2014/05/05 21:39:25, MarkM wrote:
> LGTM

I don't think it's wise to use `x` as property to add and delete ... any generic
`Point` class will have most likely an `x` property maybe with a setter/getter
or even worst, a typed value.

The suggested way to fix this problem is to verify that if the object has not an
own numeric property, then it's possible to assign and delete one in order to
fix it.

This will most likely have no side effects as a named property would have, since
I don't know code around that uses getter and setters within numbers ...

Accordingly, I'd rather patch it like the following:

+  function repair_NUMERIC_PROPERTIES_INVISIBLE() {
+    var create = Object.create;
+    var configurable = {configurable: true};
+    Object.defineProperty(Object, 'create', {
+      configurable: true,
+      writable: true,  // allow other repairs to stack on
+      value: function repairedCreate(prototype, props) {
+        var o = create(prototype);
+        // By deferring the defineProperties operation, we avoid possibly
+        // conflicting with the caller-specified property names, without
+        // needing to examine props twice.
+        if (!configurable.hasOwnProperty.call(o, 0)) {
+          delete Object.defineProperty(o, 0, configurable)[0];
+        }
+        if (props !== undefined) {
+          Object.defineProperties(o, props);
+        }
+        return o;
+      }
+    });
+  }
+

[andrea.giammarchi, 2014-05-06 00:15:32]

On 2014/05/06 00:13:46, andrea.giammarchi wrote:
> On 2014/05/05 21:39:25, MarkM wrote:
> > LGTM
> 
> I don't think it's wise to use `x` as property to add and delete ... any
generic
> `Point` class will have most likely an `x` property maybe with a setter/getter
> or even worst, a typed value.
> 
> The suggested way to fix this problem is to verify that if the object has not
an
> own numeric property, then it's possible to assign and delete one in order to
> fix it.
> 
> This will most likely have no side effects as a named property would have,
since
> I don't know code around that uses getter and setters within numbers ...
> 
> Accordingly, I'd rather patch it like the following:
> 
> +  function repair_NUMERIC_PROPERTIES_INVISIBLE() {
> +    var create = Object.create;
> +    var configurable = {configurable: true};
> +    Object.defineProperty(Object, 'create', {
> +      configurable: true,
> +      writable: true,  // allow other repairs to stack on
> +      value: function repairedCreate(prototype, props) {
> +        var o = create(prototype);
> +        // By deferring the defineProperties operation, we avoid possibly
> +        // conflicting with the caller-specified property names, without
> +        // needing to examine props twice.
> +        if (!configurable.hasOwnProperty.call(o, 0)) {
> +          delete Object.defineProperty(o, 0, configurable)[0];
> +        }
> +        if (props !== undefined) {
> +          Object.defineProperties(o, props);
> +        }
> +        return o;
> +      }
> +    });
> +  }
> +

actually, delete Object.defineProperty(o, 0, configurable)[0]; is all you need
without the if around

[kpreid_google, 2014-05-06 00:17:38]

On Mon, May 5, 2014 at 5:13 PM, <andrea.giammarchi@gmail.com> wrote:

> I don't think it's wise to use `x` as property to add and delete ... any
> generic `Point` class will have most likely an `x` property maybe with a
> setter/getter or even worst, a typed value.
>

...oops.

You're right. When I wrote the fix I was thinking that o was an “empty”
object but I overlooked that the prototype could contribute accessor
properties.

By using defineProperty we can avoid interacting with the prototype.

Thanks for catching this!

[andrea.giammarchi, 2014-05-06 00:23:15]

no problem

last reminder is that apparently that operation flags forever the "object
shape" resulting in slower objects during their lifecycle ... although I
don't have many more details on this, it was rather a hint from @jdalton

Best Regards


On Mon, May 5, 2014 at 5:17 PM, Kevin Reid <kpreid@google.com> wrote:

> On Mon, May 5, 2014 at 5:13 PM, <andrea.giammarchi@gmail.com> wrote:
>
>> I don't think it's wise to use `x` as property to add and delete ... any
>> generic `Point` class will have most likely an `x` property maybe with a
>> setter/getter or even worst, a typed value.
>>
>
> ...oops.
>
> You're right. When I wrote the fix I was thinking that o was an “empty”
> object but I overlooked that the prototype could contribute accessor
> properties.
>
> By using defineProperty we can avoid interacting with the prototype.
>
> Thanks for catching this!
>

[kpreid_google, 2014-05-06 00:35:28]

On Mon, May 5, 2014 at 5:23 PM, Andrea Giammarchi <
andrea.giammarchi@gmail.com> wrote:

> last reminder is that apparently that operation flags forever the "object
> shape" resulting in slower objects during their lifecycle ... although I
> don't have many more details on this, it was rather a hint from @jdalton
>

Yep, that's not-a-surprise. We consider the correctness more important in
this case; though it would be nice to have a refinement that avoided
slowing down the typical case.

The main problem with the obvious tricks is that we can't just, for
example, look at 'props' to see if it defines any non-numeric properties,
because it might be some more or less magical object that gives different
answers at different times. Though come to think of it we could do our own
iteration and use defineProperty instead of defineProperties — but that's
getting hairy.

[andrea.giammarchi, 2014-05-06 00:50:26]

I would personally just `delete Object.defineProperty(o, '0',
confgureZero)[0]` as suggested already and let IE solve this (hopefully)
ASAP.

Take care


On Mon, May 5, 2014 at 5:35 PM, Kevin Reid <kpreid@google.com> wrote:

> On Mon, May 5, 2014 at 5:23 PM, Andrea Giammarchi <
> andrea.giammarchi@gmail.com> wrote:
>
>> last reminder is that apparently that operation flags forever the "object
>> shape" resulting in slower objects during their lifecycle ... although I
>> don't have many more details on this, it was rather a hint from @jdalton
>>
>
> Yep, that's not-a-surprise. We consider the correctness more important in
> this case; though it would be nice to have a refinement that avoided
> slowing down the typical case.
>
> The main problem with the obvious tricks is that we can't just, for
> example, look at 'props' to see if it defines any non-numeric properties,
> because it might be some more or less magical object that gives different
> answers at different times. Though come to think of it we could do our own
> iteration and use defineProperty instead of defineProperties — but that's
> getting hairy.
>