1015790 - Problem with JS_CopyPropertyFrom when target is proxy

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I'm running into a problem that I'm not sure how to solve. I'm calling JS_CopyPropertyFrom and the target is an OuterWindowProxy. That calls js::DefineOwnProperty [1], passing in a PropertyDescriptor that came from GetOwnPropertyDescriptor. js::DefineOwnProperty creates a PropDesc and initialized it with initFromPropertyDescriptor. That PropDesc is then passed to js::DefineProperty [2], which takes the is<ProxyObject> path. It gets desc.pd(), which is undefined, and then Proxy::defineProperty returns an error when it tries to convert that undefined value back to a PropertyDescriptor.

I don't really understand the invariants here. When is the pd() field of PropDesc allowed to be undefined? It seems like it only makes sense for scripted proxies. Should we sythesize a value for pd() when it's not available (maybe using js::NewPropertyDescriptorObject)? Or should we just code around this path and avoid using desc.pd() in this case?

Also, there's a comment that says we should be able to remove PropDesc::pd() in the future [3]. Are we in the future yet? Does the removal depend on removing indirect scripted proxies? What is the purpose of the pd() field?

Jeff, can you answer these questions?

[1] http://mxr.mozilla.org/mozilla-central/source/js/src/jsobj.cpp#954
[2] http://mxr.mozilla.org/mozilla-central/source/js/src/jsobj.cpp#913
[3] http://mxr.mozilla.org/mozilla-central/source/js/src/vm/Shape.h#141

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Jason, maybe you can help here?

Comment 2

[Jason Orendorff [:jorendorff]]

I bet the patch in bug 1008441 is part of the answer here.

Comment 3

[Jason Orendorff [:jorendorff]]

It's super goofy that we have both PropDesc and PropertyDescriptor. We can definitely remove pd_ now.

Comment 4

[Jeff Walden [:Waldo]]

Yeah, this is more efaust's territory now, he's far closer to it than I am now, and is actively modifying it.

Comment 5

[Eric Faust [:efaust]]

This is ony my radar. Trying to get bug 1008441's blockers landed before tackling this.

Comment 6

[Eric Faust [:efaust]]

Attachment: Remove PropDesc::descObj_

Comment 7

[Jason Orendorff [:jorendorff]]

Comment on attachment 8442520 [details] [diff] [review]
Remove PropDesc::descObj_

Review of attachment 8442520 [details] [diff] [review]:
-----------------------------------------------------------------

Yup.

::: js/src/jsobj.cpp
@@ +271,2 @@
>          return false;
> +    vp.set(ObjectValue(*descObj));

setObject?

::: js/src/vm/ObjectImpl.cpp
@@ +22,5 @@
>      setUndefined();
>  }
>  
>  void
>  PropDesc::setUndefined()

This method's unused except by the constructor. You could delete it if you wanted to. No opinion here.

Comment 8

[Eric Faust [:efaust]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e99e0e06c51b

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e99e0e06c51b