The defineProperty method applied to the window object (or any other direct proxy or DOM proxy) sets all property descriptor values to their defaults.

NEW
Unassigned

Status

()

Core
JavaScript Engine
5 years ago
4 years ago

People

(Reporter: martin.slota, Unassigned)

Tracking

({dev-doc-needed})

23 Branch
x86_64
Linux
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

5 years ago
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:23.0) Gecko/20100101 Firefox/23.0 (Beta/Release)
Build ID: 20130803215302

Steps to reproduce:

I tried the following global code in Firefox:

    this.n = 42;
    console.log(Object.getOwnPropertyDescriptor(this, "n"));
    Object.defineProperty(this, "n", {});
    console.log(Object.getOwnPropertyDescriptor(this, "n"));


Actual results:

The first log entry has the value attribute set to 42 and all flags (writable, enumerable, configurable) set to true. The subsequent log has all the attributes set to their defaults, i.e. value is undefined and writable, enumerable, configurable are false.


Expected results:

I believe that the second log should be identical to the first log, just as it would be if a regular object was used instead of window (unless there is a good reason for this behaviour, of course).
So what happens here is that js::DefineProperty calls Proxy::defineProperty with the empty property descriptor.  That lands us in nsOuterWindowProxy::defineProperty, which forwards to js::Wrapper::defineProperty, which is actually js::DirectProxyHandler::defineProperty.

And DirectProxyHandler does:

  return CheckDefineProperty(cx, target, id, v, desc.getter(), desc.setter(),
                             desc.attributes()) &&
         JS_DefinePropertyById(cx, target, id, v, desc.getter(), desc.setter(),
                               desc.attributes());

and now we've clobbered the property that used to be there.

Jeff, is this an issue in DirectProxyHandler (in that it should be doing the "[[GetOwnProperty]] and merge that with the incoming desc" dance), or in js::DefineProperty, or in obj_defineProperty, or in nsOuterWindowProxy::defineProperty?  For normal objects, js::DefineProperty calls DefinePropertyOnObject which does all the 8.12.9 bookkeeping, but it's not clear to me whether the proxy is supposed to be trapping [[DefineOwnProperty]] or some other part of the MOP, and what exactly is supposed to be the right way to get "normal" behavior if [[DefineOwnProperty]] is what's trapped...
Assignee: nobody → general
Status: UNCONFIRMED → NEW
Component: DOM: Core & HTML → JavaScript Engine
Ever confirmed: true
Flags: needinfo?(jwalden+bmo)
You get the same issue with "this" replaced by document.getElementsByTagName("*"), for similar reasons (though there DOMProxyHandler::defineProperty is explicitly calling js_DefineOwnProperty on the expando object).

For the direct proxy handler case, you even get it with JS Proxy objects.  A shell testcase:

  var obj = {};
  var proxy = new Proxy(obj, {});
  proxy.n = 42;
  print(JSON.stringify(Object.getOwnPropertyDescriptor(proxy, "n")));
  Object.defineProperty(proxy, "n", {});
  print(JSON.stringify(Object.getOwnPropertyDescriptor(proxy, "n")));

So _if_ the proxy trap is supposed to represent [[DefineOwnProperty]], what I think we need here is a way of invoking the default [[DefineOwnProperty]], to be used by both DirectProxyHandler and DOMProxyHandler....
Summary: The defineProperty method applied to the window object sets all property descriptor values to their defaults. → The defineProperty method applied to the window object (or any other direct proxy or DOM proxy) sets all property descriptor values to their defaults.

Updated

5 years ago
Keywords: dev-doc-needed

Comment 3

5 years ago
It's sort of an issue with nsOuterWindowProxy::defineProperty.  But it's not an issue that could be fixed by changing that method, right now, because the ObjectOps::defineProperty signature is insufficiently powerful to implement descriptor-merging.  This comes back to ObjectOps's MOP being not the one we need, as usual.  Fixing this (I'm pretty sure there's a bug on file for it already) is roughly on my list as the next thing after bug 826587.
Flags: needinfo?(jwalden+bmo)
(Assignee)

Updated

4 years ago
Assignee: general → nobody
You need to log in before you can comment on or make changes to this bug.