Figuring out the behavior of WindowProxy in the face of non-configurable properties

classic Classic list List threaded Threaded
51 messages Options
123
Reply | Threaded
Open this post in threaded view
|

Re: Figuring out the behavior of WindowProxy in the face of non-configurable properties

Mark S. Miller-2
Yehuda and I just talked about this code and realized that we can allow this code to proceed on the success path without violating the invariants. However, this analysis reveals that the intent stated in the comment is unwarranted, but even that intent can be adequately honored in the scenario of interest.

The reason why the intent is unwarranted is that the descriptor omits "configurable:" rather than explicitly saying "configurable: true". If the owner object already has a configurable own property of the same name, then a defineProperty where the "configurable:" is omitted defines an own property preserving the configurability of the original own property.

Even if owner could not have already had an own property of this name, owner might be an ES6 proxy whose target is, say, an empty object. The handler's defineProperty trap could still first define a configurable own property of this name, and then proceed with the normal logic.

Since the WindowProxy is not a Proxy, or more relevantly, even if it were a Proxy, the underlying Window is not its target, we can even do the following:

When the WindowProxy sees the defineProperty with the omitted "configurable:" and determines that the underlying Window does not already have this property. WindowProxy can even preserve the unwarranted intent expressed in the comment by actually defining a *non-configurable* own property on the Window itself. However, the behavior of the WindowProxy is not observably different than our Proxy example: It acts as if it created a configurable own property on the WindowProxy of this same name, and then proceeds with the normal defineProperty behavior, which preserves that alleged configurability.






On Tue, Jan 27, 2015 at 11:48 AM, Boris Zbarsky <[hidden email]> wrote:
On 12/4/14 11:49 AM, Mark S. Miller wrote:
On Thu, Dec 4, 2014 at 2:58 AM, Boris Zbarsky <[hidden email]> wrote:
OK.  What do we do if we discover that throwing from the defineProperty call
with a non-configurable property descriptor is not web-compatible?

What we always do

So just for the record, jQuery (at least all the 2.* versions I've looked at) contains that following bits:

  Data.prototype = {
        key: function( owner ) {
...
                var descriptor = {},
...
                        // Secure it in a non-enumerable, non-writable property
                        try {
                                descriptor[ this.expando ] = { value: unlock };
                                Object.defineProperties( owner, descriptor );

                        // Support: Android < 4
                        // Fallback to a less secure definition
                        } catch ( e ) {
                                descriptor[ this.expando ] = unlock;
                                jQuery.extend( owner, descriptor );
                        }

This function is called from Data.prototype.get, which is called from jQuery.event.add.  So the upshot is that trying to add an event listener to the window via the jQuery API will hit this codepath.

Now the good news is that the try/catch _is_ present there, so this doesn't immediately break sites.  But it's something to watch out for, and we _will_ be changing the behavior of jQuery here in a way that the jQuery developers clearly think is undesirable.

-Boris



--
    Cheers,
    --MarkM

_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Figuring out the behavior of WindowProxy in the face of non-configurable properties

Boris Zbarsky
On 1/27/15 4:44 PM, Mark S. Miller wrote:

> Since the WindowProxy is not a Proxy, or more relevantly, even if it
> were a Proxy, the underlying Window is not its target, we can even do
> the following:
>
> When the WindowProxy sees the defineProperty with the omitted
> "configurable:" and determines that the underlying Window does not
> already have this property. WindowProxy can even preserve the
> unwarranted intent expressed in the comment by actually defining a
> *non-configurable* own property on the Window itself. However, the
> behavior of the WindowProxy is not observably different than our Proxy
> example: It acts as if it created a configurable own property on the
> WindowProxy of this same name, and then proceeds with the normal
> defineProperty behavior, which preserves that alleged configurability.

I'd like to understand better the suggestion here, because I'm not sure
I'm entirely following it.  Specifically, I'd like to understand it in
terms of the internal methods defined by
<https://github.com/domenic/window-proxy-spec>.

Presumably you're proposing that we keep all of that as-is except for
[[DefineOwnProperty]], right?

For [[DefineOwnProperty]], are we basically talking about changing step
1 to:

1)  If the [[Configurable]] field of Desc is present and
Desc.[[Configurable]] is false, then throw a TypeError exception.

while keeping everything else as-is, as opposed to the behavior I'd
understood we were aiming for, which was:

1)  If the [[Configurable]] field of Desc is not present or
Desc.[[Configurable]] is false, then throw a TypeError exception.

?  If so, that's certainly a change that is much more likely to be
web-compatible...

-Boris
_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Figuring out the behavior of WindowProxy in the face of non-configurable properties

Brendan Eich-2
In reply to this post by Mark S. Miller-2
Mark S. Miller wrote:
> The reason why the intent is unwarranted is that the descriptor omits
> "configurable:" rather than explicitly saying "configurable: true". If
> the owner object already has a configurable own property of the same
> name, then a defineProperty where the "configurable:" is omitted
> defines an own property preserving the configurability of the original
> own property.

Wild, and genius. How many more narrow escapes can we make and keep both
web compat and integrity? :-P

Is there any downside? What is the bad case that observably changes
behavior, if any (not involving proxies)? I'm too tired to search the
state space right now, throwing this out as a challenge.

/be
_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Figuring out the behavior of WindowProxy in the face of non-configurable properties

Mark S. Miller-2
In reply to this post by Boris Zbarsky


On Tue, Jan 27, 2015 at 5:53 PM, Boris Zbarsky <[hidden email]> wrote:
On 1/27/15 4:44 PM, Mark S. Miller wrote:
Since the WindowProxy is not a Proxy, or more relevantly, even if it
were a Proxy, the underlying Window is not its target, we can even do
the following:

When the WindowProxy sees the defineProperty with the omitted
"configurable:" and determines that the underlying Window does not
already have this property. WindowProxy can even preserve the
unwarranted intent expressed in the comment by actually defining a
*non-configurable* own property on the Window itself. However, the
behavior of the WindowProxy is not observably different than our Proxy
example: It acts as if it created a configurable own property on the
WindowProxy of this same name, and then proceeds with the normal
defineProperty behavior, which preserves that alleged configurability.

I'd like to understand better the suggestion here, because I'm not sure I'm entirely following it.  Specifically, I'd like to understand it in terms of the internal methods defined by <https://github.com/domenic/window-proxy-spec>.

Presumably you're proposing that we keep all of that as-is except for [[DefineOwnProperty]], right?

For [[DefineOwnProperty]], are we basically talking about changing step 1 to:

1)  If the [[Configurable]] field of Desc is present and Desc.[[Configurable]] is false, then throw a TypeError exception.

while keeping everything else as-is,

Exactly correct. I didn't realize until reading your reply is that this is all that's necessary -- that it successfully covers all the cases I was thinking about without any further case division.

 
as opposed to the behavior I'd understood we were aiming for, which was:

1)  If the [[Configurable]] field of Desc is not present or Desc.[[Configurable]] is false, then throw a TypeError exception. 

?  If so, that's certainly a change that is much more likely to be web-compatible...

Good! It certainly takes care of the one concrete breakage we know about so far.



--
    Cheers,
    --MarkM

_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Figuring out the behavior of WindowProxy in the face of non-configurable properties

Mark S. Miller-2
In reply to this post by Brendan Eich-2
On Tue, Jan 27, 2015 at 7:22 PM, Brendan Eich <[hidden email]> wrote:
Mark S. Miller wrote:
The reason why the intent is unwarranted is that the descriptor omits "configurable:" rather than explicitly saying "configurable: true". If the owner object already has a configurable own property of the same name, then a defineProperty where the "configurable:" is omitted defines an own property preserving the configurability of the original own property.

Wild, and genius.

(blush)

 
How many more narrow escapes can we make and keep both web compat and integrity? :-P

How many will we need? ;)

 

Is there any downside? What is the bad case that observably changes behavior, if any (not involving proxies)?

You get the following non-intuitive but allowed behavior.

if (!hasOwnProperty(W, P)) {
  defineProperty(W, P, { value: V })
  console.log(getOwnPropertyDescriptor(W, P).configurable); // true
}

However, you could also get this behavior if W is a proxy, so it doesn't introduce any new cases beyond what's already possible. It is only surprising.

That's not much of a downside, and I can't think of any other downside. 


I'm too tired to search the state space right now, throwing this out as a challenge.

 
--
    Cheers,
    --MarkM

_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Figuring out the behavior of WindowProxy in the face of non-configurable properties

Mark S. Miller-2
In reply to this post by Mark S. Miller-2


On Tue, Jan 27, 2015 at 9:45 PM, Mark S. Miller <[hidden email]> wrote:


On Tue, Jan 27, 2015 at 5:53 PM, Boris Zbarsky <[hidden email]> wrote:
On 1/27/15 4:44 PM, Mark S. Miller wrote:
Since the WindowProxy is not a Proxy, or more relevantly, even if it
were a Proxy, the underlying Window is not its target, we can even do
the following:

When the WindowProxy sees the defineProperty with the omitted
"configurable:" and determines that the underlying Window does not
already have this property. WindowProxy can even preserve the
unwarranted intent expressed in the comment by actually defining a
*non-configurable* own property on the Window itself. However, the
behavior of the WindowProxy is not observably different than our Proxy
example: It acts as if it created a configurable own property on the
WindowProxy of this same name, and then proceeds with the normal
defineProperty behavior, which preserves that alleged configurability.

I'd like to understand better the suggestion here, because I'm not sure I'm entirely following it.  Specifically, I'd like to understand it in terms of the internal methods defined by <https://github.com/domenic/window-proxy-spec>.

Presumably you're proposing that we keep all of that as-is except for [[DefineOwnProperty]], right?

For [[DefineOwnProperty]], are we basically talking about changing step 1 to:

1)  If the [[Configurable]] field of Desc is present and Desc.[[Configurable]] is false, then throw a TypeError exception.

while keeping everything else as-is,

Exactly correct. I didn't realize until reading your reply is that this is all that's necessary -- that it successfully covers all the cases I was thinking about without any further case division.

Here's another option, not clearly better or worse:

[[DefineOwnProperty]] (P, Desc)

  1. let R be the result of calling the [[DefineOwnProperty]] internal method of W with arguments P and Desc.
  2. If desc.[[Configurable]] is present and false, then throw a TypeError exception.
  3. return R.
This is exactly like your solution, but with the order of the two steps switched. Perhaps the next breakage we see will tell us which of these to choose. If both are web compatible, then we need only pick which one we like better.

 

 
as opposed to the behavior I'd understood we were aiming for, which was:

1)  If the [[Configurable]] field of Desc is not present or Desc.[[Configurable]] is false, then throw a TypeError exception. 

?  If so, that's certainly a change that is much more likely to be web-compatible...

Good! It certainly takes care of the one concrete breakage we know about so far.



--
    Cheers,
    --MarkM



--
    Cheers,
    --MarkM

_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

RE: Figuring out the behavior of WindowProxy in the face of non-configurable properties

Domenic Denicola
In reply to this post by Mark S. Miller-2
From: Mark S. Miller [mailto:[hidden email]]

> On Tue, Jan 27, 2015 at 5:53 PM, Boris Zbarsky <[hidden email]> wrote:
>> I'd like to understand better the suggestion here, because I'm not sure I'm entirely following it.  Specifically, I'd like to understand it in terms of the internal methods defined by <https://github.com/domenic/window-proxy-spec>.
>>
>> Presumably you're proposing that we keep all of that as-is except for [[DefineOwnProperty]], right?
>>
>> For [[DefineOwnProperty]], are we basically talking about changing step 1 to:
>>
>> 1)  If the [[Configurable]] field of Desc is present and Desc.[[Configurable]] is false, then throw a TypeError exception.
>>
>> while keeping everything else as-is,
>
> Exactly correct. I didn't realize until reading your reply is that this is all that's necessary -- that it successfully covers all the cases I was thinking about without any further case division.

I'm having a bit of trouble understanding how this maps to the solution described in your previous message, Mark. Your "I didn't realize until reading your reply is that this is all that's necessary" indicates I'm probably just missing something, so help appreciated.

My question is, what happens if Desc.[[Configurable]] is not present, and P does not already exist on W? By my reading, we then fall through to calling the [[DefineOwnProperty]] internal method of W with arguments P and Desc.

Assuming W's [[DefineOwnProperty]] is that of an ordinary object, I believe that takes us through OrdinaryDefineOwnProperty(W, P, Desc). Since P does not exist on W, and W is extensible, that takes us to ValidateAndApplyPropertyDescriptor(O, P, true, Desc, undefined). Then according to step 2.c, " If the value of an attribute field of Desc is absent, the attribute of the newly created property is set to its default value." The default value is false, right? So won't this try to define a non-configurable property on W?

I would have thought the modification needed to be more like:

[[DefineOwnProperty]] (P, Desc)

1. If desc.[[Configurable]] is not present, set desc.[[Configurable]] to true.
2. If desc.[[Configurable]] is false, then throw a TypeError exception.
3. Return the result of calling the [[DefineOwnProperty]] internal method of W with arguments P and Desc.

(here I have inserted step 1, but step 2 and 3 are unchanged from the previous incarnation).
_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Figuring out the behavior of WindowProxy in the face of non-configurable properties

Mark S. Miller-2
On Wed, Jan 28, 2015 at 8:51 AM, Domenic Denicola <[hidden email]> wrote:
From: Mark S. Miller [mailto:[hidden email]]

> On Tue, Jan 27, 2015 at 5:53 PM, Boris Zbarsky <[hidden email]> wrote:
>> I'd like to understand better the suggestion here, because I'm not sure I'm entirely following it.  Specifically, I'd like to understand it in terms of the internal methods defined by <https://github.com/domenic/window-proxy-spec>.
>>
>> Presumably you're proposing that we keep all of that as-is except for [[DefineOwnProperty]], right?
>>
>> For [[DefineOwnProperty]], are we basically talking about changing step 1 to:
>>
>> 1)  If the [[Configurable]] field of Desc is present and Desc.[[Configurable]] is false, then throw a TypeError exception.
>>
>> while keeping everything else as-is,
>
> Exactly correct. I didn't realize until reading your reply is that this is all that's necessary -- that it successfully covers all the cases I was thinking about without any further case division.

I'm having a bit of trouble understanding how this maps to the solution described in your previous message, Mark. Your "I didn't realize until reading your reply is that this is all that's necessary" indicates I'm probably just missing something, so help appreciated.

My question is, what happens if Desc.[[Configurable]] is not present, and P does not already exist on W? By my reading, we then fall through to calling the [[DefineOwnProperty]] internal method of W with arguments P and Desc.

Assuming W's [[DefineOwnProperty]] is that of an ordinary object, I believe that takes us through OrdinaryDefineOwnProperty(W, P, Desc). Since P does not exist on W, and W is extensible, that takes us to ValidateAndApplyPropertyDescriptor(O, P, true, Desc, undefined). Then according to step 2.c, " If the value of an attribute field of Desc is absent, the attribute of the newly created property is set to its default value." The default value is false, right? So won't this try to define a non-configurable property on W?

In this situation, it will try and succeed. This more closely obeys the intent in the original code (e.g., the comment in the jQuery code), since it creates a non-configurable property on the *Window* W. It does not violate any invariant, since all that's observable on the *WindowProxy* (given the rest of your draft spec, which remain unchanged) is a configurable property of the same name.


 

I would have thought the modification needed to be more like:

[[DefineOwnProperty]] (P, Desc)

1. If desc.[[Configurable]] is not present, set desc.[[Configurable]] to true.
2. If desc.[[Configurable]] is false, then throw a TypeError exception.
3. Return the result of calling the [[DefineOwnProperty]] internal method of W with arguments P and Desc.

(here I have inserted step 1, but step 2 and 3 are unchanged from the previous incarnation).



--
    Cheers,
    --MarkM

_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

RE: Figuring out the behavior of WindowProxy in the face of non-configurable properties

Domenic Denicola
From: Mark S. Miller [mailto:[hidden email]]

> In this situation, it will try and succeed. This more closely obeys the intent in the original code (e.g., the comment in the jQuery code), since it creates a non-configurable property on the *Window* W. It does not violate any invariant, since all that's observable on the *WindowProxy* (given the rest of your draft spec, which remain unchanged) is a configurable property of the same name.

Ah, I see! So then another non-intuitive (but invariant-preserving) consequence would be:

```js
Object.defineProperty(window, "prop", { value: "foo" });

var propDesc = Object.getOwnPropertyDescriptor(window, "prop");

if (propDesc.configurable) {
  Object.defineProperty(window, "prop", { value: "bar" });

  // this will fail, even though the property is supposedly configurable,
  // since when it forwards from the WindowProxy `window` to the underlying
  // Window object, it the Window's [[DefineOwnProperty]] fails.
}
```

Am I getting this right?
_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Figuring out the behavior of WindowProxy in the face of non-configurable properties

Mark S. Miller-2


On Wed, Jan 28, 2015 at 11:08 AM, Domenic Denicola <[hidden email]> wrote:
From: Mark S. Miller [mailto:[hidden email]]

> In this situation, it will try and succeed. This more closely obeys the intent in the original code (e.g., the comment in the jQuery code), since it creates a non-configurable property on the *Window* W. It does not violate any invariant, since all that's observable on the *WindowProxy* (given the rest of your draft spec, which remain unchanged) is a configurable property of the same name.

Ah, I see! So then another non-intuitive (but invariant-preserving) consequence would be:

```js
Object.defineProperty(window, "prop", { value: "foo" });

var propDesc = Object.getOwnPropertyDescriptor(window, "prop");

if (propDesc.configurable) {
  Object.defineProperty(window, "prop", { value: "bar" });

  // this will fail, even though the property is supposedly configurable,
  // since when it forwards from the WindowProxy `window` to the underlying
  // Window object, it the Window's [[DefineOwnProperty]] fails.
}
```

Am I getting this right?

Exactly, yes. And again, if window is an ES6 proxy rather that a WindowProxy, it could also cause this behavior, so it doesn't create any situation which is not otherwise possible.

The key points are:

1) The throw does (arguably) better obey the code's intent, since the property mostly acts like a non-configurable property until the window is navigated.

2) If a window navigation happens between your first step and your second, the second step may well succeed, which is what we (arguably) want, but which would have been prohibited if propDesc.configurable evaluated to true.


--
    Cheers,
    --MarkM

_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
Reply | Threaded
Open this post in threaded view
|

Re: Figuring out the behavior of WindowProxy in the face of non-configurable properties

Brendan Eich-2
In reply to this post by Mark S. Miller-2
Mark S. Miller wrote:

>
>     Exactly correct. I didn't realize until reading your reply is that
>     this is all that's necessary -- that it successfully covers all
>     the cases I was thinking about without any further case division.
>
>
> Here's another option, not clearly better or worse:
>
>
>       [[DefineOwnProperty]] (P, Desc)
>
>  1. let R be the result of calling the [[DefineOwnProperty]] internal
>     method of/W/with arguments/P/and /Desc/.
>  2. If/desc/.[[Configurable]] is present and*false*, then throw
>     a*TypeError*exception.
>  3. return R.
>
> This is exactly like your solution, but with the order of the two
> steps switched. Perhaps the next breakage we see will tell us which of
> these to choose. If both are web compatible, then we need only pick
> which one we like better.

I like the shorter one (filling in from cited text below, here it is in
full:


      [[DefineOwnProperty]] (P, Desc)

 1. If /desc/.[[Configurable]] is present and/desc/.[[Configurable]] is
    *false*, then throw a *TypeError* exception.
 2. Return the result of calling the [[DefineOwnProperty]] internal
    method of /W/ with arguments /P/ and /Desc/.


Besides being shorter, this doesn't call through to [[DOP]], which could
have effects, and only then maybe-throw.

/be

>
>         as opposed to the behavior I'd understood we were aiming for,
>         which was:
>
>         1)  If the [[Configurable]] field of Desc is not present or
>         Desc.[[Configurable]] is false, then throw a TypeError exception.
>
>
>         ?  If so, that's certainly a change that is much more likely
>         to be web-compatible...
>
>
>     Good! It certainly takes care of the one concrete breakage we know
>     about so far.
>
_______________________________________________
es-discuss mailing list
[hidden email]
https://mail.mozilla.org/listinfo/es-discuss
123