1065604 - Require JSPROP_SHARED when a property is defined with JSPROP_GETTER or JSPROP_SETTER

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

I think some accessor behavior is still controlled by JSPROP_SHARED, so I would expect bugs if we're failing to specify it when defining accessor properties.

Comment 1

[Jason Orendorff [:jorendorff]]

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

Comment 2

[Jason Orendorff [:jorendorff]]

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

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: Assert that JSPROP_SHARED is set on all properties defined with JSPROP_GETTER or JSPROP_SETTER. try: -b d -p macosx64,win32 -u all[10.8,6.2] -t none

Comment 4

[Jeff Walden [:Waldo]]

Comment on attachment 8487809 [details] [diff] [review]
Assert that JSPROP_SHARED is set on all properties defined with JSPROP_GETTER or JSPROP_SETTER.  try: -b d -p macosx64,win32 -u all[10.8,6.2] -t none

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

::: js/src/jsapi-tests/testDefinePropertyIgnoredAttributes.cpp
@@ +46,5 @@
>      JS::RootedValue defineValue(cx);
>  
>      // Try a getter. Allow it to fill in the defaults.
>      CHECK(JS_DefineProperty(cx, obj, "foo", defineValue,
> +                            IgnoreAll | JSPROP_NATIVE_ACCESSORS | JSPROP_SHARED,

Hum, I thought JSPROP_NATIVE_ACCESSORS was only for JSPropertySpec entries.  Maybe I'm wrong.

Comment 5

[Jason Orendorff [:jorendorff]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #4)
> Hum, I thought JSPROP_NATIVE_ACCESSORS was only for JSPropertySpec entries. 
> Maybe I'm wrong.

It is a "shallow" feature, dispensed with in jsapi.cpp:DefinePropertyById, but all the JS_Define* APIs go through there.

Comment 6

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a59d92e7cb7

Comment 7

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

https://hg.mozilla.org/mozilla-central/rev/3a59d92e7cb7

Comment 8

[Eric Faust [:efaust]]

Comment on attachment 8487809 [details] [diff] [review]
Assert that JSPROP_SHARED is set on all properties defined with JSPROP_GETTER or JSPROP_SETTER.  try: -b d -p macosx64,win32 -u all[10.8,6.2] -t none

Approval Request Comment
[Feature/regressing bug #]: Various
[User impact if declined]: Blocks uplift for bug 1042567
[Describe test coverage new/current, TBPL]: Been cooking on 
[Risks and why]: None. Sanifies a few existing calls for the addition of an assert.
[String/UUID change made/needed]: None

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr31/rev/6519e957e6de

Comment 10

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8487809 [details] [diff] [review]
Assert that JSPROP_SHARED is set on all properties defined with JSPROP_GETTER or JSPROP_SETTER.  try: -b d -p macosx64,win32 -u all[10.8,6.2] -t none

Beta+

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ecaedd858fd0

Comment 12

[bhavana bajaj [:bajaj]]

Comment on attachment 8487809 [details] [diff] [review]
Assert that JSPROP_SHARED is set on all properties defined with JSPROP_GETTER or JSPROP_SETTER.  try: -b d -p macosx64,win32 -u all[10.8,6.2] -t none

Approving given the blocking issue.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/6fab869fe612
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/9b229efe2402

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/ecaedd858fd0

Comment 15

[Kai-Zhen Li [:seinlin][:kli]]

http://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/6fab869fe612