constructObjectFromPropertyDescriptor() is incorrect with partial descriptors

Hi everyone,

I found a case of inconsistent assertions violated in this test case.

Tag: 606.1.9.4
OS: ubuntu 16.04 x86

Reproduce steps:
- Run this code:
    var def = [];
    var p = new Proxy({foo:1, bar:2}, { defineProperty: function(o, v, desc) { def.push(v); Object.defineProperty(o, v, desc); return true; }});
    p.foo = 2; p.bar = 4;
    p.foo = 2; p.bar = 4;
    p.foo = 2; p.bar = 4;
    print(def + '' === "foo,bar");

Expected results
false

Actual results
true


JSCore should record all calls to setters "foo" and "bar" (six in total), whose expected effect is to store the string "foo,bar,foo,bar,foo,bar" on variable "def". Therefore, the test should fail under JSCore as it fails in other engines (V8, SpiderMonkey and Chakra) --return value should be true as per test assertion. However, JSCore only stores the first two calls to the setters and the test (incorrectly) passes.

cinfuzz

[Alexey Shvayka]

Created attachment 396533 [details]
Patch

[Ross Kirsling]

Comment on attachment 396533 [details]
Patch

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

> Source/JavaScriptCore/runtime/ObjectConstructor.h:98
> +    JSObject* result = constructEmptyObject(globalObject);

Huh, did this used to be able to throw? Most callsites don't use a ThrowScope but there's still a good handful of them that do...

[Alexey Shvayka]

(In reply to Ross Kirsling from comment #3)
> > Source/JavaScriptCore/runtime/ObjectConstructor.h:98
> > +    JSObject* result = constructEmptyObject(globalObject);
> 
> Huh, did this used to be able to throw?

I did some ChangeLog digging, seems like it never threw, even `JSObject* prototype` overload. Just a precaution I suppose?

[Yusuke Suzuki]

Comment on attachment 396533 [details]
Patch

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

>> Source/JavaScriptCore/runtime/ObjectConstructor.h:98
>> +    JSObject* result = constructEmptyObject(globalObject);
> 
> Huh, did this used to be able to throw? Most callsites don't use a ThrowScope but there's still a good handful of them that do...

This should not throw. You can add `scope.assertNoException();`

[Alexey Shvayka]

Created attachment 396566 [details]
Patch

Bring back ThrowScope with an assert, set reviewer.

[Alexey Shvayka]

Created attachment 397059 [details]
Patch

Rebase patch and fix typo in ChangeLog.

[Ross Kirsling]

I wonder if it's overkill to scope.assertNoException() both inside the function as well as outside?

[Alexey Shvayka]

Created attachment 397061 [details]
Patch

Remove inner scope.assertNoException().

[Alexey Shvayka]

(In reply to Ross Kirsling from comment #8)
> I wonder if it's overkill to scope.assertNoException() both inside the
> function as well as outside?

scope.assertNoException() are more useful outside, and I really like the absence of ThrowScope in constructObjectFromPropertyDescriptor().

[Ross Kirsling]

(In reply to Alexey Shvayka from comment #10)
> (In reply to Ross Kirsling from comment #8)
> > I wonder if it's overkill to scope.assertNoException() both inside the
> > function as well as outside?
> 
> scope.assertNoException() are more useful outside, and I really like the
> absence of ThrowScope in constructObjectFromPropertyDescriptor().

Agreed!

[EWS]

Committed r260447: <https://trac.webkit.org/changeset/260447>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397061 [details].

[Radar WebKit Bug Importer]

<rdar://problem/62124337>