1103368 - Ban stub getters and setters in most places throughout the engine

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

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?

Comment 2

[Jason Orendorff [:jorendorff]]

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

Comment 3

[Jason Orendorff [:jorendorff]]

(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.

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: part 1 - Forbid stub getter/setter arguments to NativeObject::{add,change,put}Property

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: part 2 - Ban stub getter/setter arguments to js::baseops::Define{Property,Generic,Element}, DefineNativeProperty, and DefinePropertyOrElement

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: part 3 - Ban stub getter/setter arguments to JSObject::define{Generic,Property,Element} and js::SetPropertyIgnoringNamedGetter

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: part 4 - Ban stub getter/setter arguments to js::CheckDefineProperty

Comment 8

[Jason Orendorff [:jorendorff]]

Attachment: part 5 - Make Class::getProperty and setProperty nullable instead of needing stub functions. Never store stub functions in JSPropertyDescriptors

Comment 9

[Brian Hackett [Laid off!]]

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?

Comment 10

[Jason Orendorff [:jorendorff]]

(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.

Comment 11

[Jeff Walden [:Waldo]]

(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).

Comment 12

[Jeff Walden [:Waldo]]

...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.

Comment 13

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/64aa4c4d0691
https://hg.mozilla.org/integration/mozilla-inbound/rev/e737de5b209b
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba1d6ece1bc6
https://hg.mozilla.org/integration/mozilla-inbound/rev/d201babc2e84
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4b619628d1d

Comment 14

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

b2g build bustage https://treeherder.mozilla.org/ui/logviewer.html#?job_id=4173512&repo=mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/c80672f72db6
https://hg.mozilla.org/integration/mozilla-inbound/rev/56ebb99d774e
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae31d2669511
https://hg.mozilla.org/integration/mozilla-inbound/rev/cabeb902de6d
https://hg.mozilla.org/integration/mozilla-inbound/rev/6daa62f15a63

Comment 15

[Jason Orendorff [:jorendorff]]

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

Comment 16

[Jason Orendorff [:jorendorff]]

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

Comment 17

[Jason Orendorff [:jorendorff]]

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

Comment 18

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f57597056a60
https://hg.mozilla.org/integration/mozilla-inbound/rev/5814a1723842
https://hg.mozilla.org/integration/mozilla-inbound/rev/f654193f2c2e
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d0fdbf77f28
https://hg.mozilla.org/integration/mozilla-inbound/rev/65e82280a4be

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/f57597056a60
https://hg.mozilla.org/mozilla-central/rev/5814a1723842
https://hg.mozilla.org/mozilla-central/rev/f654193f2c2e
https://hg.mozilla.org/mozilla-central/rev/8d0fdbf77f28
https://hg.mozilla.org/mozilla-central/rev/65e82280a4be