Ban stub getters and setters in most places throughout the engine

RESOLVED FIXED in mozilla37

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

unspecified
mozilla37
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

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?
(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: nobody → jorendorff
Status: NEW → ASSIGNED
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+
(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.
Depends on: 1123875
You need to log in before you can comment on or make changes to this bug.