Ban stub getters and setters in most places throughout the engine

RESOLVED FIXED in mozilla37

Status

()

Core
JavaScript Engine
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

unspecified
mozilla37
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

(Assignee)

Description

4 years ago
Shapes are already a "clean spot": a Shape never has a getter value of JS_PropertyStub or a setter value of JS_StrictPropertyStub.

In this bug, I want to grow the clean spot to cover most of the engine.

In the end, the stubs have to go.
The only reason to really have the stubs right now is to differentiate "nothing special" and "class-default", right?

Could we perhaps flip the API boundary indication there, so null means "nothing special" and you have to go out of your way to ask for the class default thing?
(Assignee)

Comment 3

4 years ago
(In reply to [:bz] from comment #1)
> The only reason to really have the stubs right now is to differentiate
> "nothing special" and "class-default", right?

And only in the JS_Define* APIs, right.

> Could we perhaps flip the API boundary indication there, so null means
> "nothing special" and you have to go out of your way to ask for the class
> default thing?

Yep, that's my intent. Not in this bug; it'll be the last thing to change before deleting the stubs entirely. It'll involve paying a short visit to all 500 JS_Define* call sites. Big fun.
(Assignee)

Comment 4

4 years ago
Created attachment 8527846 [details] [diff] [review]
part 1 - Forbid stub getter/setter arguments to NativeObject::{add,change,put}Property
Attachment #8527846 - Flags: review?(bhackett1024)
(Assignee)

Updated

4 years ago
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
(Assignee)

Comment 5

4 years ago
Created attachment 8527847 [details] [diff] [review]
part 2 - Ban stub getter/setter arguments to js::baseops::Define{Property,Generic,Element}, DefineNativeProperty, and DefinePropertyOrElement
Attachment #8527847 - Flags: review?(bhackett1024)
(Assignee)

Comment 6

4 years ago
Created attachment 8527848 [details] [diff] [review]
part 3 - Ban stub getter/setter arguments to JSObject::define{Generic,Property,Element} and js::SetPropertyIgnoringNamedGetter
Attachment #8527848 - Flags: review?(bhackett1024)
(Assignee)

Comment 7

4 years ago
Created attachment 8527849 [details] [diff] [review]
part 4 - Ban stub getter/setter arguments to js::CheckDefineProperty
Attachment #8527849 - Flags: review?(bhackett1024)
(Assignee)

Comment 8

4 years ago
Created attachment 8527851 [details] [diff] [review]
part 5 - Make Class::getProperty and setProperty nullable instead of needing stub functions. Never store stub functions in JSPropertyDescriptors
Attachment #8527851 - Flags: review?(bhackett1024)
Attachment #8527846 - Flags: review?(bhackett1024) → review+
Comment on attachment 8527847 [details] [diff] [review]
part 2 - Ban stub getter/setter arguments to js::baseops::Define{Property,Generic,Element}, DefineNativeProperty, and DefinePropertyOrElement

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

::: js/src/jsobj.cpp
@@ -2247,5 @@
>              if (!xdr->codeConstValue(&tmpValue))
>                  return false;
>  
>              if (mode == XDR_DECODE) {
> -                if (!DefineNativeProperty(cx, obj, id, tmpValue, NULL, NULL, JSPROP_ENUMERATE))

Huh, do we have a way to keep uses of NULL from leaking back into the code base?
Attachment #8527847 - Flags: review?(bhackett1024) → review+
Attachment #8527848 - Flags: review?(bhackett1024) → review+
Attachment #8527849 - Flags: review?(bhackett1024) → review+
Attachment #8527851 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 10

4 years ago
(In reply to Brian Hackett (:bhackett) from comment #9)
> > -                if (!DefineNativeProperty(cx, obj, id, tmpValue, NULL, NULL, JSPROP_ENUMERATE))
> 
> Huh, do we have a way to keep uses of NULL from leaking back into the code
> base?

Nope. A quick grep shows dozens of hits for NULL, plus another 200 or so in vm/Opcodes.h.
(In reply to Brian Hackett (:bhackett) from comment #9)
> Huh, do we have a way to keep uses of NULL from leaking back into the code
> base?

You could add a template overload that's MOZ_DELETEd:

template<typename N1, typename N2>
bool
js::DefineNativeProperty(ExclusiveContext *cx, HandleNativeObject obj, HandleId id,
                         HandleValue value, N getter, N2 setter, unsigned attrs,
                         typename mozilla::EnableIf<mozilla::IsNullPointer<N1>::value ||
                                                    mozilla::IsNullPointer<N1>::value,
                                                    int>::Type dummy = 0) MOZ_DELETE;

Only works for functions where no user should ever call providing literal nullptr.  Not useful for JSAPI entry points where nullptr is supposed to mean the default behavior.  Also, a bit of a mess to get right, but maybe worth it as one-time pain (and then only pain for people who get it wrong afterward).
...which of course assumes people are using nullptr only.  We could more or less guarantee that in our codebase, and indeed I don't know how it is that we still have any NULL uses -- I thought they'd all been converted to nullptr.

Updated

4 years ago
Depends on: 1123875
You need to log in before you can comment on or make changes to this bug.