1008441 - Object.defineProperty(proxy, desc) parses desc twice

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

The first time when js::DefineOwnProperty calls PropDesc::initialize.

The second time when js::DefineProperty calls Proxy::defineProperty(JSContext *cx, HandleObject proxy, HandleId id, HandleValue v)

This is observable using standard ES6 library features, since the descriptor may also be a proxy.

(More to the point, this means you can't call js::DefineProperty passing a PropDesc that doesn't have an initialized pd_ field for Proxy::defineProperty to redundantly parse. Since I want to do that for Array.from, I have to fix this bug.)

Comment 1

[Jason Orendorff [:jorendorff]]

Attachment: bug-1008441-PropDesc-twice-v1.patch

That Proxy::defineProperty signature turns out to be used only in these two places, so delete it.

Comment 2

[André Bargull [:anba]]

Comment on attachment 8420400 [details] [diff] [review]
bug-1008441-PropDesc-twice-v1.patch

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

::: js/src/jsobj.cpp
@@ +223,5 @@
> +    if (!hasConfigurable() || !configurable())
> +        attrs |= JSPROP_PERMANENT;
> +    if (hasEnumerable() && enumerable())
> +        attrs |= JSPROP_ENUMERATE;
> +    if (!hasWritable() || !hasWritable())

Change to `!hasWritable() || !writable()` ?

Comment 3

[Jason Orendorff [:jorendorff]]

Uh. Wow. How did I manage to write that? Good catch.

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: bug-1008441-PropDesc-twice-v2.patch

Actually, even with hasWritable(), that line was busted. We must not set JSPROP_READONLY if the descriptor is an accessor descriptor.

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: bug-1008441-PropDesc-twice-v3.patch

Rebased on top of Eric's patches in bug 978238, which contain the populatePropertyDescriptor method I'm using... bumping review to Eric too.

Comment 6

[Jason Orendorff [:jorendorff]]

https://tbpl.mozilla.org/?tree=Try&rev=c64a8769add5

Comment 7

[Eric Faust [:efaust]]

Comment on attachment 8430383 [details] [diff] [review]
bug-1008441-PropDesc-twice-v3.patch

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

Looks good. Thanks for doing this. r=me

Comment 8

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3825abd9a302

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3825abd9a302