Closed Bug 1065604 Opened 5 years ago Closed 5 years ago

Require JSPROP_SHARED when a property is defined with JSPROP_GETTER or JSPROP_SETTER

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
firefox34 --- fixed
firefox35 --- fixed
firefox-esr31 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: jorendorff, Assigned: jorendorff)

References

Details

Attachments

(1 file)

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.
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
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.
Attachment #8487809 - Flags: review?(jwalden+bmo) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/3a59d92e7cb7
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
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
Attachment #8487809 - Flags: approval-mozilla-esr31?
Attachment #8487809 - Flags: approval-mozilla-beta?
Attachment #8487809 - Flags: approval-mozilla-b2g32?
Attachment #8487809 - Flags: approval-mozilla-b2g30?
Blocks: 1042567
Attachment #8487809 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
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+
Attachment #8487809 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Attachment #8487809 - Flags: approval-mozilla-b2g32?
Attachment #8487809 - Flags: approval-mozilla-b2g32+
Attachment #8487809 - Flags: approval-mozilla-b2g30?
Attachment #8487809 - Flags: approval-mozilla-b2g30+
You need to log in before you can comment on or make changes to this bug.