Bug 185968 – Enforce invariant that GetterSetter objects are immutable.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

Patch coming.

[Radar WebKit Bug Importer]

<rdar://problem/40541416>

[Mark Lam]

Created attachment 341237 [details]
proposed patch.

[Mark Lam]

Comment on attachment 341237 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=341237&action=review

> Source/JavaScriptCore/ChangeLog:3
> +        Enforce invariant that GetterSetter objects are invariant.

This should say that GetterSetters are immutable, not “invariant”.

[Saam Barati]

Comment on attachment 341237 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=341237&action=review

r=me

>> Source/JavaScriptCore/ChangeLog:3
>> +        Enforce invariant that GetterSetter objects are invariant.
> 
> This should say that GetterSetters are immutable, not “invariant”.

is there a test or some explanation here of what's going on?

> Source/JavaScriptCore/runtime/JSObject.cpp:3582
> +    RETURN_IF_EXCEPTION(scope, false);

Why can this throw?

[Mark Lam]

Comment on attachment 341237 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=341237&action=review

>>> Source/JavaScriptCore/ChangeLog:3
>>> +        Enforce invariant that GetterSetter objects are invariant.
>> 
>> This should say that GetterSetters are immutable, not “invariant”.
> 
> is there a test or some explanation here of what's going on?

I'll add some commentary here:

"The code already assumes the invariant that GetterSetter objects are immutable.  For example, the use of @tryGetById in builtins expect this invariant to be true.  The existing code mostly enforces this except for one case: JSObject's validateAndApplyPropertyDescriptor, where it will re-use the same GetterSetter object.  This patch enforces this invariant by removing the setGetter and setSetter methods of GetterSetter, and requiring the getter/setter callback functions to be specified at construction time."

>> Source/JavaScriptCore/runtime/JSObject.cpp:3582
>> +    RETURN_IF_EXCEPTION(scope, false);
> 
> Why can this throw?

I carried this over from LLIntSlowPath's slow_path_put_getter_setter_by_id, and was thinking OOM at the time.  After thinking about it some more, I believe this is not needed.  I'll remove it (along with the LLIntSlowPath exception check) and retest.

[Mark Lam]

Created attachment 341314 [details]
patch for landing.

[WebKit Commit Bot]

Comment on attachment 341314 [details]
patch for landing.

Clearing flags on attachment: 341314

Committed r232211: <https://trac.webkit.org/changeset/232211>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

(In reply to WebKit Commit Bot from comment #7)
> Comment on attachment 341314 [details]
> patch for landing.
> 
> Clearing flags on attachment: 341314
> 
> Committed r232211: <https://trac.webkit.org/changeset/232211>

This broke the 32-bit !ASSERT_DISABLED build. I attempted to fix it in <https://trac.webkit.org/r232231>.