Bug 201484 – LazyClassStructure::setConstructor should not store the constructor to the global object

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

[Tadeu Zagallo]

<rdar://problem/50400451>

[Tadeu Zagallo]

Created attachment 378035 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 378035 [details]
Patch

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

r=me with comment.

> Source/JavaScriptCore/runtime/Lookup.h:367
> +        if (!propertyName.isNull())

I think this never happens because this function is called for property materialization.
I think `propertyName.isNull()` code in the original one exists for the use case not through static-property-table. And I believe we do not have a case like,

1. While using LazyClassStructure without specifying it in static-property-table...
2. But still we want global object property for that.

Can you ensure that the above use case does not exist in our code base?

[Tadeu Zagallo]

(In reply to Yusuke Suzuki from comment #2)
> I think this never happens because this function is called for property
> materialization.
> I think `propertyName.isNull()` code in the original one exists for the use
> case not through static-property-table. And I believe we do not have a case
> like,
> 
> 1. While using LazyClassStructure without specifying it in
> static-property-table...
> 2. But still we want global object property for that.
> 
> Can you ensure that the above use case does not exist in our code base?

You're right, thanks! I've updated the patch.

[Tadeu Zagallo]

Created attachment 378038 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 378038 [details]
Patch

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

> Source/JavaScriptCore/runtime/Lookup.h:367
> +        thisObj.putDirect(vm, propertyName, constructor, static_cast<unsigned>(PropertyAttribute::DontEnum));

Oops, please check attribute. This doesn't matter since all LazyClassStructure property's attribute is now DontEnum, but it is possible that other properties will use LazyClassStructure in the future.
So, can you use `value.attributes()` ?

[Tadeu Zagallo]

Comment on attachment 378038 [details]
Patch

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

>> Source/JavaScriptCore/runtime/Lookup.h:367
>> +        thisObj.putDirect(vm, propertyName, constructor, static_cast<unsigned>(PropertyAttribute::DontEnum));
> 
> Oops, please check attribute. This doesn't matter since all LazyClassStructure property's attribute is now DontEnum, but it is possible that other properties will use LazyClassStructure in the future.
> So, can you use `value.attributes()` ?

Oops, good catch, I'll fix it. Thanks!

[Tadeu Zagallo]

Created attachment 378099 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 378099 [details]
Patch for landing

Clearing flags on attachment: 378099

Committed r249538: <https://trac.webkit.org/changeset/249538>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Keith Miller]

*** Bug 202433 has been marked as a duplicate of this bug. ***