"cannot report inconsistent value for non-writable, non-configurable property"

[johnjbarton]

I hope you won't mind a user-level question here.

I am using Chrome with this library to wrap 'window' in a proxy so that I might record and replay the DOM operations. On the operation 'window.document', I fail. I need to return the proxy of window.document so subsequent operations on the return will also enter the proxy code. However, window.document is own frozen data of window and the test in reflect.js line 1063 throws a TypeError. As far as I can see from the code, I could not succeed in wrapping returns from window.document, and yet my case seems quite similar to the ones discussed as use cases for Proxy. Any hints?

[tvcutsem]

Hi John,

For background on why that particular error occurs, see http://soft.vub.ac.be/~tvcutsem/invokedynamic/frozen-proxies

There's a dirty fix and a proper fix. The dirty fix is to change the target of your proxy from window to an empty object that inherits from window. I say "dirty" because now the proxy object will not describe window's properties as own properties (they are instead inherited). The upside is that proxies may change the value of inherited properties, even if the inherited property is non-configurable, non-writable, such as window.document.

The proper fix requires setting the proxy's target to a dummy, "shadow" target that will store wrapped properties of the "real" target. The basic idea is that, when a frozen property such as document is accessed, the proxy handler adds a wrapped version of the property to its shadow and returns the wrapped property. The proxy will then find the same wrapped frozen property on the shadow target, so it won't trip up on the assertion when it checks whether the properties are the same.

In code, rather than doing:

var realWindow = window;
var proxyWindow = new Proxy(realWindow, {
  get: function(target, name, rcvr) {
    return wrap(target[name]); // assume "wrap" does whatever wrapping you need to do on properties
  }
});

You do something like:

var realWindow = window;
var shadowWindow = {};
var proxyWindow = new Proxy(shadowWindow, {
  get: function(target, name, rcvr) {
    // target is now bound to the shadowWindow
    var pd = Object.getOwnPropertyDescriptor(realWindow, name);
    if (pd !== undefined && !pd.configurable && !pd.writable) {
      // the property is non-configurable, non-writable
      Object.defineProperty(target, name, {
         value: wrap(realWindow[name]),
         writable: false,
         configurable: false,
         enumerable: pd.enumerable
      });
      return target[name];
    }
    // if the property isn't non-configurable, no problem to just do as before
    return wrap(realWindow[name]);
  }
});

For a little more background on this, see http://soft.vub.ac.be/~tvcutsem/invokedynamic/notification-proxies (scroll down to "Implementing Membranes with Direct Proxies").

[johnjbarton]

Thanks, that much worked great.

In Chrome (at least) one cannot cause a program to run against the proxyWindow in a trivial way, eg window = proxyWindow. So I guess have to transcode the JS to replace identifiers by something like (id in window ? windowProxy.id : id).

[johnjbarton]

(I'm unsure what is going on with my window.HTMLElement.prototype property proxy)

[johnjbarton]

After

Object.defineProperty(target, name, descriptor);
var result = target[name];

For name==='prototype' and original object window.HTMLElement, result !== descriptor.value, so the subject error message returns.

[johnjbarton]

I replaced

return target[name];

with

return descriptor.value;

where descriptor is pd with a wrapped value property. That seems to work.

[johnjbarton]

I came upon another form of this issue. The defineProperty() is really updateProperty(). If we start with:

Object.getOwnPropertyDescriptor(window.Element.prototype, 'createShadowRoot')
Object {value: function, writable: true, enumerable: true, configurable: true}

then we apply

Object.defineProperty(window.Element.prototype, 'createShadowRoot', {value: function() {return this.webkitCreateShadowRoot();}});

then the resulting property will be

Object.getOwnPropertyDescriptor(window.Element.prototype, 'createShadowRoot')
Object {value: function, writable: true, enumerable: true, configurable: true}

But if we set this updated descriptor on to the target, then the trap will fail because the incoming desc argument will have configurable: false, giving:

 Uncaught TypeError: cannot successfully define a non-configurable descriptor for 
 configurable or non-existent property 'createShadowRoot'

[tvcutsem]

I don't think this is an issue related to proxies or this shim, but is inherent to the ECMAScript spec.

defineProperty either creates a property if it does not yet exist (and in this case, it uses false as the default for any missing attribute, such as writable,configurable,enumerable), OR if the property does already exist, then defineProperty updates it, not updating any attributes left out of the descriptor (so if configurable was true, it stays true)

What I tend to do to avoid issues like this is to never leave out any attributes in my arguments to defineProperty (i.e. always specify an explicit value for the writable,configurable and enumerable attributes).

[johnjbarton]

The way this issue relates to the shim is via https://github.com/tvcutsem/harmony-reflect/blob/master/reflect.js#L547,

    if (!desc.configurable && !isSealedDesc(targetDesc)) {

The desc is given by the program source being processed by the proxy-based tool: we don't have the option to change this code to fix problems from the ECMAScript spec. And the shim, when we use the pattern you outline above, does not have access to the underlying original object to know that it's descriptor is configurable, even though the target shadow object's descriptor is not.

The only fix I see is to break the shim into two layers, a base and extension layer, then extending the base once for normal proxies and once for the pattern above. That way the problematic test can be placed only in the 'normal' branch and the other branch can have a test that includes the shadow.

As a practical matter I simply disabled the test in my copy of reflect.js.

[tvcutsem]

Thanks for the feedback.

Two other possibilities:

• Stash a reference to the real target in the shadow, e.g. __realTarget, allowing you to check if the updated/defined property already exists on the real target.
• Rather than initializing the shadow to an empty object, you could initialize it so that it is born with a copy of the properties of the real target (with the values wrapped). This way, every time defineProperty updates an existing property on the real target, it will also update an existing property on the shadow.

I don't have enough insight into your use case whether these solutions would be feasible or not (eager wrapping has a bigger perf impact than lazy wrapping).

I'll need to think more about how to implement a proper fix. One option could be to allow users of the shim to disable invariant checking on proxies altogether (essentially removing the if-test you highlighted, and many other similar ones). It's kind of a hack in the sense that invariants can now be broken, so such an option would have to come with a "use only if you know what you're doing" disclaimer.

[johnjbarton]

Just another note about the 'shadow' technique for future reference: the fallback or default implementation of Proxy fails, sometimes in mysterious ways. The reason is simple once I realized it: the fallback goes to the shadow, not to the object closed-over.

The latest way I got burnt was has(), which I did not know existed. But this manifest when forEach() from

var forEach = Array.prototype.forEach.call.bind(Array.prototype.forEach);

failed for results from document.querySelectorAll(). Evidently the built-in forEach calls the in operator and when the proxy responds with false for every element, then forEach fails to call the callback.

[johnjbarton]

As far as I can figure out, I don't think your suggestions work out for the 'update' problem. We don't need __realTarget because we have the real target in the closure. We can read the real target descriptor but then what? The validation that fails uses the argument desc.configurable and compares it to the proxy-target final-descriptor. The correct value for desc.configurable is false: this is given by the caller. The correct value for the real target final descriptor is configurable: true (since we are updating). If we use __realTarget we can only read a value which will fail the validation.

If my claim is true, then this issue is not directly related to the shadow technique, but only to the update issue. Indeed if I put this test into testProxies.js it fails:

  function testUpdatePropertyDescriptor() {
    var obj = {};
    Object.defineProperty(obj, 'prop', {value: true, configurable: true});
    var proxy = Proxy(obj, {
      defineProperty: function(target, name, desc) {
        return Object.defineProperty(obj, name, desc);
      }
    });
    Object.defineProperty(proxy, 'prop', {value: function() { return false; }});
    var descriptor = Object.getOwnPropertyDescriptor(obj, 'prop');
    assert(typeof descriptor.value === 'function');
  }

[tvcutsem]

Another option: in the defineProperty trap of your proxy, first update the shadow target's property, and only then update the real target's property. This should bring the shadow and real target's property attributes in sync by the time the proxy performs the assertion. Something like:

var p = Proxy(shadow, {
  defineProperty(shadow, name, desc) {
    Object.defineProperty(shadow, name, desc);
    return Object.defineProperty(realTarget, name, desc);
  }
});

[johnjbarton]

This option should also fail if the shadow has configurable: true. The incoming desc does not specify configurable so the update

Object.defineProperty(shadow, name, desc);

does not change the property value, so again the test will fail.

The validator could verify that the trap did not update to configurable: true when the incoming desc is not true. It would have to read the descriptor on the target before the trap and look at the diff. In my example then the 'before-trap' value of configurable would be true and the desc value is undefined so the update value is true and the validation test would pass (we would not need to check sealed).

Since the function and the trap are really updaters I think that's the only way to verify correctly.

[tvcutsem]

Finally I realized what was going on: the assertion wrongly assumes that a missing configurable attribute implies that configurable was set to false.

I just submitted a patch that replaces the test !desc.configurable with an explicit desc.configurable === false. The little test case you submitted (which I also included in testProxies.js) now works as expected.