The default bug view has changed. See this FAQ.

Drop native object checks from JS_(Get|Set)ReservedSlot

RESOLVED FIXED in mozilla13

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

({dev-doc-needed})

unspecified
mozilla13
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

v2
5.14 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
+++ This bug was initially created as a clone of Bug #724310 comment 3 +++

I suppose for historical reasons we allow JS_SetReservedSlot and JS_GetReservedSlot to take a non-native object and do nothing in that case. We should replace those checks with asserts. Also JS_SetReservedSlot calls GCPoke presumably to help the shutdown GC to collect more garbage. I suspect that with cycle collector that is not necessary for quite some time and we can drop that.
(Assignee)

Comment 1

5 years ago
We do not need those native/non-native asserts. The checks in JSObject::(get|set)ReservedSlot should be sufficient to call the API.
Summary: Require that JS_(Get|Set)ReservedSlot is only used with native objects → Drop native object checks from JS_(Get|Set)ReservedSlot
(Assignee)

Comment 2

5 years ago
Created attachment 596507 [details] [diff] [review]
v1

The patch makes JS_(Get|Set)ReservedSlot a public API around JSObject::(get|set)ReservedSlot. That allowed to remove inlined SetReservedSlot from jsfriendsapi.h. However I have to keep GetReservedSlot as its use is widespread and its removal may have bad performance implications.
Assignee: general → igor
Attachment #596507 - Flags: review?(jwalden+bmo)
Actually, we were using js::SetReservedSlot in the new-binding code.  It can get pretty hot during prototype object creation and worse yet object wrapping...

Is there actually a reason to remove it?
(Assignee)

Comment 4

5 years ago
(In reply to Boris Zbarsky (:bz) from comment #3)
> Actually, we were using js::SetReservedSlot in the new-binding code.  It can
> get pretty hot during prototype object creation and worse yet object
> wrapping...
> 
> Is there actually a reason to remove it?

The reason is to minimize exposure of JS internals. But if this a hot issue, then surely we can keep it.
(Assignee)

Comment 5

5 years ago
Created attachment 596944 [details] [diff] [review]
v2

The new patch keeps js::SetReservedSlot and its callers outside js engine as is.
Attachment #596507 - Attachment is obsolete: true
Attachment #596507 - Flags: review?(jwalden+bmo)
Attachment #596944 - Flags: review?(jwalden+bmo)
Attachment #596944 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 6

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1b718602a5a
https://hg.mozilla.org/mozilla-central/rev/c1b718602a5a
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.