Issue 6388 - Wrap inherited function properties as well

Issue 6388 - Wrap inherited function properties as well

[kzar, 2018-02-14 14:47:14]

Patch Set 1

[Manish Jethani, 2018-02-14 17:17:23]

On 2018/02/14 14:47:14, kzar wrote:
> Patch Set 1

I'm not seeing the connection between this and #6338 and I'm not sure why this
would be necessary.

[kzar, 2018-02-14 18:12:04]

On 2018/02/14 17:17:23, Manish Jethani wrote:

> I'm not seeing the connection between this and #6338...

Whoops I typed the issue number wrong, it's actually #6388.

[a.giammarchi, 2018-02-14 18:57:21]

Since I'm in CC, and if I might, this patch solves the ticket issue so, in that
specific regard, it's a LGTM.

However, I don't fully understand the current whole dance around the descriptor.

If it's an accessor, `get` and `set` are deleted but `func` is trapped upfront
with the assumption it's a function (no check, no typeof, no guard).

A getter that returns a function usually comes only from weird/proxied
accessors/cases, but fair enough, I guess there is some edge case for that I'm
not aware of.

In any case, if it's not an accessor, the only slots in a descriptor are
`value`, which is overwritten, and `writable`, which is irrelevant when you use
`defineProperty` and ignored in this case anyway.

The only common property between accessors and regular descriptors is
`configurable`, which is not checked, and ignored when used with
`defineProperty`, hence capable of silently failing which is usually always
undesired.

Accordingly, if the whole `while(__proto__)` loop was used to understand if the
accessor or property was `configurable`, this piece of code would make a lot of
sense but, as it is, and for my understanding of that code, this is like
promisifying a method without guarantees it happened and through unnecessary
overhead because a direct `Object.defineProperty(object, name, {value(){...that
code with the func reference...}})` would've obtained exact same result.

If there was a specific reason for all of this then I'd love to learn it, thank
you.

[Manish Jethani, 2018-02-15 09:59:24]

https://codereview.adblockplus.org/29697596/diff/29697597/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29697596/diff/29697597/polyfill.js#newcode87
polyfill.js:87: while (!descriptor && (objectProto =
Object.getPrototypeOf(objectProto)));
I think we should have a separate function called getPropertyDescriptor right
above wrapAsyncAPIs that looks like this:

  function getPropertyDescriptor(object, name)
  {
    return Object.getOwnPropertyDescriptor(object, name) ||
           getPropertyDescriptor(Object.getPrototypeOf(object), name);
  }

This is elegant and will not be a performance issue like recursion normally is
since the prototype chain is not very long.

Note that Object.getPrototypeOf here is guaranteed to return non-null (since we
already checked that the object contains the property), we can avoid that check.

[Manish Jethani, 2018-02-15 10:09:40]

On 2018/02/14 18:57:21, a.giammarchi wrote:

> If it's an accessor, `get` and `set` are deleted but `func` is trapped upfront
> with the assumption it's a function (no check, no typeof, no guard).

It is guaranteed to be a function, because we are looking for only those APIs
that are functions. See the asyncAPIs array.

> A getter that returns a function usually comes only from weird/proxied
> accessors/cases, but fair enough, I guess there is some edge case for that I'm
> not aware of.

It is an "Edge" case alright, see
https://hg.adblockplus.org/adblockpluschrome/rev/fe75016a7373

If it's a read-only property then it has a getter but no setter.

[a.giammarchi, 2018-02-15 10:21:02]

On 2018/02/15 10:09:40, Manish Jethani wrote:
> On 2018/02/14 18:57:21, a.giammarchi wrote:
>
> It is an "Edge" case alright, see
> https://hg.adblockplus.org/adblockpluschrome/rev/fe75016a7373
>

Thanks. That explains the `func` trap, but it doesn't fully explain why we need
to find the descriptor and change it instead of simply defining it right away.

Is it because we want to preserve enumerable ? shouldn't this code check also
for configurable in case it's own property descriptor so that errors won't be
thrown?

Or maybe that's never a concern? Thanks for extra details.

[Manish Jethani, 2018-02-15 11:19:43]

On 2018/02/15 10:21:02, a.giammarchi wrote:
> On 2018/02/15 10:09:40, Manish Jethani wrote:
> > On 2018/02/14 18:57:21, a.giammarchi wrote:
> >
> > It is an "Edge" case alright, see
> > https://hg.adblockplus.org/adblockpluschrome/rev/fe75016a7373
> >
> 
> Thanks. That explains the `func` trap, but it doesn't fully explain why we
need
> to find the descriptor and change it instead of simply defining it right away.

So that's a bit out of idealism. We want to preserve the original properties of
the descriptors, just in case it matters. I suppose it doesn't make a difference
either way, but it's neat to do it this way.

> Is it because we want to preserve enumerable ? shouldn't this code check also
> for configurable in case it's own property descriptor so that errors won't be
> thrown?

This is a basic assumption we have made about the APIs, i.e. they will be
configurable. If they are not configurable, there is nothing we can do anyway.
We cannot "fail gracefully" because the extension won't work without the APIs
being the way we need them to be.

Right now we only run this code on Chromium and Edge, and it is not a problem so
far. I would be surprised if they change the configurability later - it would
break at least some extensions (and then we would complain and the issue would
get fixed at the browser's end).

[a.giammarchi, 2018-02-15 11:33:57]

On 2018/02/15 11:19:43, Manish Jethani wrote:
> 
> So that's a bit out of idealism. We want to preserve the original properties
of
> the descriptors, just in case it matters. I suppose it doesn't make a
difference
> either way, but it's neat to do it this way.

It works, but accessors are trashed without checks so that `writable` gets
ignored, and so does configurable because of the assumption.

I mean, that's it, the only thing that is preserved here is enumerable, there's
nothing left in a descriptor.

Accessor: get, set, enumerable, configurable
Regular: enumerable, configurable, writable, value

We take possible accessors and make them regular, non writable, descriptors,
and we overwrite the value (trapped upfront so, who cares) and we ignore
configurable.

So this still LGTM because it fixes the issue (and your suggested function is
surely cleaner than the loop) but it feels slightly overhead dance to keep only
enumerable around, also considering that previous descriptor will be preserved.

```js
var obj = {value: 123};
Object.defineProperty(obj, 'value', {value: 456});
obj.value; // 456
obj.propertyIsEnumerable('value'); // still true
obj.value = 9;
obj.value; // 9 still writable
```

TL;DR if configurable, which is our default assumption, omitting properties for
own descriptors keep previous values (i.e. enumerable or writable) while
inherited descriptor (from class or prototype) usually aren't expected to be
enumerable and accessors would be shadowed.

This means that at the end of the day doing directly
`Object.defineProperty(object, name, {value() {}})` will result in *exact same
result* of retrieving the descriptor object, trashing accessors, and setting it
as own descriptor.

However, I'm done bothering you with this 'cause it's longer to write and read
than to accept this PR :-)

Thanks for details and clarifications.

[Manish Jethani, 2018-02-15 13:05:13]

On 2018/02/15 11:33:57, a.giammarchi wrote:

> [snip]
>
> TL;DR if configurable, which is our default assumption, omitting properties
for
> own descriptors keep previous values (i.e. enumerable or writable) while
> inherited descriptor (from class or prototype) usually aren't expected to be
> enumerable and accessors would be shadowed.
> 
> This means that at the end of the day doing directly
> `Object.defineProperty(object, name, {value() {}})` will result in *exact same
> result* of retrieving the descriptor object, trashing accessors, and setting
it
> as own descriptor.

You're right, I didn't realize that Object.defineProperty preserved the original
values. Nevertheless if we're overriding a property up in the prototype chain by
defining the same property on the object directly, in that case we could still
try to preserve the original values. So it makes sense to still copy the
descriptor and make adjustments for "writable".

e.g.

  if ("set" in descriptor)
    descriptor.writable = true;

  delete descriptor["get"];
  delete descriptor["set"];
  ...

But I would do this as part of another patch.

[Manish Jethani, 2018-02-15 13:17:51]

https://codereview.adblockplus.org/29697596/diff/29697597/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29697596/diff/29697597/polyfill.js#newcode83
polyfill.js:83: do
By the way I find this nicer too:

    let descriptor;
    for (let proto = object;
         !(descriptor = Object.getOwnPropertyDescriptor(proto, name));
         proto = Object.getPrototypeOf(proto));

So the extra variable stays local to the loop, and also we don't check if proto
is non-null, only descriptor.

[a.giammarchi, 2018-02-15 13:25:08]

On 2018/02/15 13:05:13, Manish Jethani wrote:
> On 2018/02/15 11:33:57, a.giammarchi wrote:
> 
> in that case we could still try to preserve the original values.

configurable is the assumption, but non writable is IMO desirable.

nullified setters also have usually side effects + a setter doesn't necessary
mean `writable`, it might be something that throws a better error once some code
tries to redefine the method.

Accordingly, once we have `func` trapped, and it's not `null`ish, what I think
is all we need is the following descriptor for that property name:

{
  configurable: true,
  value() { ... promisified ... }
}

That's it. If it was an accessor, we don't care, we shadow it, and keep it
configurable as we do already now.
If it was inherited, we don't want it to be enumerable (my preference, at
least).
If it was own, we leave enumerability and writability as it was.

I might argue I'd prefer to have patched methods explicitly non writable (maybe
also non configurable if I want to trust my patches), but I don't have enough
background to be sure that's really desired here.

Regards

[Manish Jethani, 2018-02-15 14:31:11]

On 2018/02/15 13:25:08, a.giammarchi wrote:

> [snip]
>
> Accordingly, once we have `func` trapped, and it's not `null`ish, what I think
> is all we need is the following descriptor for that property name:
> 
> {
>   configurable: true,
>   value() { ... promisified ... }
> }
> 
> That's it. If it was an accessor, we don't care, we shadow it, and keep it
> configurable as we do already now.
> If it was inherited, we don't want it to be enumerable (my preference, at
> least).
> If it was own, we leave enumerability and writability as it was.

OK, you've convinced me. Given that Object.defineProperty automatically
preserves the original values, and in some cases we are in fact overriding a
property rather than replacing it (it still exists in the prototype chain), it
seems logical to set just "value" and "configurable" here. So we don't need to
find the descriptor after all, we should just set the descriptor using
Object.defineProperty.

Thanks for this.

The only concern I have is that it should be tested on Edge as well, just in
case there are any issues.

Dave - we should include Ollie here since the original change was for Edge, done
by him.

[a.giammarchi, 2018-02-15 14:57:04]

On 2018/02/15 14:31:11, Manish Jethani wrote:
> On 2018/02/15 13:25:08, a.giammarchi wrote:
> 
> The only concern I have is that it should be tested on Edge as well, just in
> case there are any issues.

It's always worth testing Edge

> Dave - we should include Ollie here since the original change was for Edge,
done
> by him.

All I know about IE, but hopefully not Edge, is that accessors need to be
explicitly deleted (since configurable anyway) before you can redefine same
property as non-accessor.

If this is the case, all you need to do is:

```js
// before setting the new descriptor
delete object[name];
// set the new descriptor
Object.defineProperty(object, name, descriptor);
```

Deleting also drops the previous descriptor so that the patch would look like my
initial comment in the ticket:

```js
const descriptor = Object.getOwnPropertyDescriptor(object, name) ||
                   {configurable: true};

delete object[name];
delete descriptor.get;
delete descriptor.set;

descriptor.value = function () {};

return Object.defineProperty(object, name, descriptor);
```

Above code will do everything that worked already and fallback to regular
descriptor.

In every case all we've agreed until now remain the same (descriptor properties
preserved or shadowing inherited descriptor) and no browser should get hurt.

Yet I believe IE11 already fixed the infamous accessor bug so hopefully none of
this is needed.

Always worth double checking ;-)

[kzar, 2018-02-16 13:36:46]

Patch Set 2: Simplify our approach

I also did not know Object.defineProperty preserved the attributes for existing
properties which weren't specified! Very cool, thanks for the tip.

I've tested that on Edge 41.16299.15.0, Chrome 64.0.3282.167 and Firefox 58.0.1.
It's quite nice, the accessor properties are cleared for us since we specified
the "value" property. FWIW I used this code to test:

(function ()
{
  "use strict";
  let accessorObj = Object.create({}, {foo: {get: Math.random, configurable:
true, enumerable: true}});
  let notwritableObj = Object.create({}, {foo: {writable: false, value: 42,
configurable: true}});
  
  console.log("before", accessorObj.foo, notwritableObj.foo);
  console.log("before desc",
              Object.getOwnPropertyDescriptor(accessorObj, "foo"),
              Object.getOwnPropertyDescriptor(notwritableObj, "foo"));
  
  Object.defineProperty(accessorObj, "foo", {value: 1});
  Object.defineProperty(notwritableObj, "foo", {value: 1});
  
  console.log("after", accessorObj.foo, notwritableObj.foo);
  console.log("after desc",
              Object.getOwnPropertyDescriptor(accessorObj, "foo"),
              Object.getOwnPropertyDescriptor(notwritableObj, "foo"));
}());

I've tested my changes on those browsers as well. One caveat however is that my
Windows 10 VM was having trouble opening the console for the extension's
background page, but I tested blocking worked (once I had applied my patch for
#6385) and that I didn't regress #5954 - the reason why Ollie made this change
in the first place.

https://codereview.adblockplus.org/29697596/diff/29699555/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29697596/diff/29699555/polyfill.js#newcode87
polyfill.js:87: {
Sorry for changing the indentation here, it makes the diff a bit messy. I
haven't changed anything else inside the function though... honest.

[a.giammarchi, 2018-02-16 13:45:21]

LGTM ( the "last but best" one 😂 )

[Manish Jethani, 2018-02-16 14:03:53]

https://codereview.adblockplus.org/29697596/diff/29699555/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29697596/diff/29699555/polyfill.js#newcode83
polyfill.js:83: // inherited its other attributes (e.g. enumerable) are
presereved,
Nit: "preserved" (spelling)

https://codereview.adblockplus.org/29697596/diff/29699555/polyfill.js#newcode84
polyfill.js:84: // except accessor properties since we're specifying a value.
Nit: "except accessor attributes" (?)

https://codereview.adblockplus.org/29697596/diff/29699555/polyfill.js#newcode86
polyfill.js:86: value: function(...args)
Since we're using an object literal here now we can use the shorthand:

  Object.defineProperty(object, name, {
    value(...args)
    {
      ...
    }
  });

i.e. no function keyword

[Manish Jethani, 2018-02-16 16:28:17]

On 2018/02/16 14:03:53, Manish Jethani wrote:
> https://codereview.adblockplus.org/29697596/diff/29699555/polyfill.js
> File polyfill.js (right):
> 
>
https://codereview.adblockplus.org/29697596/diff/29699555/polyfill.js#newcode83
> polyfill.js:83: // inherited its other attributes (e.g. enumerable) are
> presereved,
> Nit: "preserved" (spelling)
> 
>
https://codereview.adblockplus.org/29697596/diff/29699555/polyfill.js#newcode84
> polyfill.js:84: // except accessor properties since we're specifying a value.
> Nit: "except accessor attributes" (?)
> 
>
https://codereview.adblockplus.org/29697596/diff/29699555/polyfill.js#newcode86
> polyfill.js:86: value: function(...args)
> Since we're using an object literal here now we can use the shorthand:
> 
>   Object.defineProperty(object, name, {
>     value(...args)
>     {
>       ...
>     }
>   });
> 
> i.e. no function keyword

Oh, and LGTM regardless.

[kzar, 2018-02-16 16:41:08]

Patch Set 3 : Use method shorthand
Patch Set 4 : Improved comment

https://codereview.adblockplus.org/29697596/diff/29699555/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29697596/diff/29699555/polyfill.js#newcode83
polyfill.js:83: // inherited its other attributes (e.g. enumerable) are
presereved,
On 2018/02/16 14:03:53, Manish Jethani wrote:
> Nit: "preserved" (spelling)

Done.

https://codereview.adblockplus.org/29697596/diff/29699555/polyfill.js#newcode84
polyfill.js:84: // except accessor properties since we're specifying a value.
On 2018/02/16 14:03:53, Manish Jethani wrote:
> Nit: "except accessor attributes" (?)

How's this?

https://codereview.adblockplus.org/29697596/diff/29699555/polyfill.js#newcode86
polyfill.js:86: value: function(...args)
On 2018/02/16 14:03:53, Manish Jethani wrote:
> Since we're using an object literal here now we can use the shorthand:
> 
>   Object.defineProperty(object, name, {
>     value(...args)
>     {
>       ...
>     }
>   });
> 
> i.e. no function keyword

Good point, eslint picked that up I just forgot to run it. Done.

[Manish Jethani, 2018-02-16 16:47:38]

https://codereview.adblockplus.org/29697596/diff/29699555/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29697596/diff/29699555/polyfill.js#newcode84
polyfill.js:84: // except accessor properties since we're specifying a value.
On 2018/02/16 16:41:08, kzar wrote:
> On 2018/02/16 14:03:53, Manish Jethani wrote:
> > Nit: "except accessor attributes" (?)
> 
> How's this?

Since "enumerable" is referred to as an attribute in the previous part of the
sentence, I thought the accessors ("get" and "set") should also be referred to
as attributes, but I may be wrong. Anyway this is very minor.

[Manish Jethani, 2018-02-16 16:49:51]

https://codereview.adblockplus.org/29697596/diff/29699564/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29697596/diff/29699564/polyfill.js#newcode84
polyfill.js:84: // except accessor property attributes (e.g. get and set) since
Yeah this reads much better.

LGTM

[kzar, 2018-02-16 17:08:37]

Thanks to you both, I'm happy with this now.