Closed Bug 1138499 Opened 5 years ago Closed 5 years ago

Assert descriptor sanity on entry to DefineProperty and exit from GetOwnPropertyDescriptor

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: jorendorff, Assigned: jorendorff)

References

(Blocks 1 open bug)

Details

Attachments

(4 files)

JSPropertyDescriptor has too many degrees of freedom relative to the spec. We need to start nailing some of this down.
Duplicate of this bug: 1133315
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
Attachment #8582009 - Attachment is obsolete: true
Attachment #8582009 - Flags: review?(jwalden+bmo)
Attachment #8582009 - Attachment is obsolete: false
Attachment #8582009 - Flags: review?(jwalden+bmo)
Blocks: 1125624
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?
Attachment #8583441 - Flags: review?(jwalden+bmo) → review+
Attachment #8583442 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #8583444 - Flags: review?(jwalden+bmo) → review+
Attachment #8582009 - Flags: review?(jwalden+bmo) → review+
(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.
> Hmm, someone misspelled quux.

Fixed! :D
Flags: needinfo?(jorendorff)
Depends on: 1150837
Depends on: 1150906
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?
Flags: needinfo?(jorendorff)
Ryan, could you back this patch? thanks
Flags: needinfo?(ryanvm)
Before bothering the sheriff, Jeff, could you backout this change?
Thanks
Flags: needinfo?(ryanvm) → needinfo?(jwalden+bmo)
WAIT
Flags: needinfo?(jorendorff)
I have a patch for this. More details to come in bug 1150906.
Flags: needinfo?(jwalden+bmo)
You need to log in before you can comment on or make changes to this bug.