1138499 - Assert descriptor sanity on entry to DefineProperty and exit from GetOwnPropertyDescriptor

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

JSPropertyDescriptor has too many degrees of freedom relative to the spec. We need to start nailing some of this down.

Comment 2

[Jason Orendorff [:jorendorff]]

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

Comment 3

[Jason Orendorff [:jorendorff]]

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

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: part 0 - Fix code spuriously using JSPROP_READONLY when defining an accessor property

Comment 5

[Jason Orendorff [:jorendorff]]

Attachment: part 1 - Assert some basic rules on property descriptors on entry to DefineProperty and exit from GetOwnPropertyDescriptor

Comment 6

[Jason Orendorff [:jorendorff]]

Attachment: part 2 - Strengthen assertComplete() to require that both [[Get]] and [[Set]] be present on accessor properties

Comment 7

[Jason Orendorff [:jorendorff]]

Attachment: part 3 - Flip JS_CHECK_ACCESSOR_FLAGS from a blacklist to a whitelist

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 8583441 [details] [diff] [review]
part 1 - Assert some basic rules on property descriptors on entry to DefineProperty and exit from GetOwnPropertyDescriptor

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

::: js/src/jsapi-tests/testDefinePropertyIgnoredAttributes.cpp
@@ +48,2 @@
>      CHECK(JS_DefineProperty(cx, obj, "foo", defineValue,
> +                            JSPROP_IGNORE_ENUMERATE | JSPROP_IGNORE_PERMANENT | JSPROP_SHARED,

Guh, these are so unreadable.  :-(  Can't wait until we make people construct descriptors to define or redefine properties, only.

@@ +78,5 @@
>      CHECK(JS_GetPropertyDescriptor(cx, obj, "baz", &desc));
>      CHECK(CheckDescriptor(desc, DataDescriptor, false, false, false));
>  
>      // Now again with a configurable property
> +    CHECK(JS_DefineProperty(cx, obj, "quox", defineValue,

Hmm, someone misspelled quux.

::: js/src/proxy/BaseProxyHandler.cpp
@@ +54,5 @@
>      if (!desc.object()) {
>          vp.setUndefined();
>          return true;
>      }
> +    desc.assertComplete();

Nothing against these, but why not add assertCompleteIfFound calls to getPropertyDescriptor and getOwnPropertyDescriptor directly, so that all callers get these enforced?

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 8583444 [details] [diff] [review]
part 3 - Flip JS_CHECK_ACCESSOR_FLAGS from a blacklist to a whitelist

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

::: js/src/jsapi.h
@@ +2102,5 @@
>    (static_cast<void>(sizeof(JS::detail::CheckIsCharacterLiteral(s))), \
>     reinterpret_cast<To>(s))
>  
>  #define JS_CHECK_ACCESSOR_FLAGS(flags) \
> +  (static_cast<mozilla::EnableIf<!((flags) & ~(JSPROP_ENUMERATE | JSPROP_PERMANENT))>::Type>(0), \

Minor preference for == 0 versus a ! there, since you're not testing something inherently boolean.

Comment 10

[Jason Orendorff [:jorendorff]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #8)
> ::: js/src/proxy/BaseProxyHandler.cpp
> > +    desc.assertComplete();
> 
> Nothing against these, but why not add assertCompleteIfFound calls to
> getPropertyDescriptor and getOwnPropertyDescriptor directly, so that all
> callers get these enforced?

Because they're virtual methods, with overloads scattered throughout the codebase. Also, many implementation have multiple returns. It would be too easy (for both me and later hackers) to forget to add the assertion somewhere, particularly when writing a new overload or adding a new path to an existing one, exactly the cases where the assertion is most wanted.

Comment 11

[Jason Orendorff [:jorendorff]]

> Hmm, someone misspelled quux.

Fixed! :D

Comment 12

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fafda276abd9
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad243a3cd06f
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b51b38317d6
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7388a9c3935

Comment 13

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

Backed out (along with every other patch from that massive push) in https://hg.mozilla.org/integration/mozilla-inbound/rev/386c8b5b73c0 for talos other timeouts:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=3fc49391f7fe&filter-searchStr=talos oth

Comment 14

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=faf253018339

Comment 15

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/725dbd169e90
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9c99e8ce207
https://hg.mozilla.org/integration/mozilla-inbound/rev/034027f41aaf
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3ef9fce0df5

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/725dbd169e90
https://hg.mozilla.org/mozilla-central/rev/f9c99e8ce207
https://hg.mozilla.org/mozilla-central/rev/034027f41aaf
https://hg.mozilla.org/mozilla-central/rev/b3ef9fce0df5

Comment 17

[Benjamin Smedberg]

This has caused topcrash regression 1150906 in nightly. Can you please either address that bug today or back this one out to get nightly back on good footing?

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Ryan, could you back this patch? thanks

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Before bothering the sheriff, Jeff, could you backout this change?
Thanks

Comment 20

[Jason Orendorff [:jorendorff]]

WAIT

Comment 21

[Jason Orendorff [:jorendorff]]

I have a patch for this. More details to come in bug 1150906.