Bug 158295 – JSGlobalObject::addFunction should call deleteProperty rather than removeDirect

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

[Gavin Barraclough]

The fact we're calling removeDirect here means that there is dependency that JSGlobalObject symbol table properties mush have higher precedence than JSDOMWindow static properties. As a part of cleaning up & encapsulating static table lookup within JSObject (bug #158178) this order is going to change; reacting to be able to support this.

When program code is executed, and that program code declares functions, upon entry symbol table entries on the global object are created for those functions. Any conflicting existing properties are removed from the global object's property storage. There should not ever be duplicate entries in property storage and the symbol table. However because we currently use removeDirect to remove conflicts, this fails to delete any copies of the property in the static table (deleteProperty would).

Precedence of property lookup for global objects is currently something like:

1) JSObject property storage array
2) JSObject static property map
3) JSGlobalObject static property map
4) Symbol table
5) JSDOMWindow static property map

We get away with failing to remove entries from static tables because (a) currently neither JSObject nor JSGlobalObject actually have any static table properties, and (b) since symbol table lookup currently takes precedence over the JSDOMWindow static table we get away with leaving a stale value in it.

As a part of bug #158178 access order will be more like:

1) JSObject property storage array
2) JSDOMWindow static property map
3) JSGlobalObject static property map
4) JSObject static property map
5) Symbol table

(Static table properties performed at a consistent time, and with most derived type having greatest precedence such that it can override values.) Post bug #158178, this issue will have testing coverage from existing tests (e.g. fast/encoding/parser-tests-50.html replaces alert).

[Gavin Barraclough]

Created attachment 280431 [details]
Fix

[Gavin Barraclough]

Created attachment 280433 [details]
Fix

[Saam Barati]

Comment on attachment 280433 [details]
Fix

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

r=me with comments

> Source/JavaScriptCore/runtime/JSObject.cpp:1500
> +            if (entry->attributes() & DontDelete && exec->vm().deletePropertyMode() != VM::DeletePropertyMode::IgnoreConfigurable)

Nit: You can use the local VM& reference.

> Source/JavaScriptCore/runtime/JSObject.cpp:1508
> +        if (attributes & DontDelete && exec->vm().deletePropertyMode() != VM::DeletePropertyMode::IgnoreConfigurable)

Ditto

> Source/JavaScriptCore/runtime/VM.h:360
> +    class DeletePropertyModeScope {

Not super important, but this could also be written more succinctly as a class with a single member variable that is of type SetForScope<DeletePropertyMode>

[Gavin Barraclough]

(In reply to comment #3)
> Comment on attachment 280433 [details]
> Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280433&action=review
> 
> r=me with comments
> 
> > Source/JavaScriptCore/runtime/JSObject.cpp:1500
> > +            if (entry->attributes() & DontDelete && exec->vm().deletePropertyMode() != VM::DeletePropertyMode::IgnoreConfigurable)
> 
> Nit: You can use the local VM& reference.
> 
> > Source/JavaScriptCore/runtime/JSObject.cpp:1508
> > +        if (attributes & DontDelete && exec->vm().deletePropertyMode() != VM::DeletePropertyMode::IgnoreConfigurable)
> 
> Ditto

Oops, fixed!

> > Source/JavaScriptCore/runtime/VM.h:360
> > +    class DeletePropertyModeScope {
> 
> Not super important, but this could also be written more succinctly as a
> class with a single member variable that is of type
> SetForScope<DeletePropertyMode>

Interesting! – hadn't come across that.

Holding of for now, I think that would mean making the member public? - and I'm loath too do so since that would open up to other errors. Maybe we can work on a way to make a type that generalizes a scoped set, while supporting private.

Committed revision 201654.

[Saam Barati]

(In reply to comment #4)
> (In reply to comment #3)
> > Comment on attachment 280433 [details]
> > Fix
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=280433&action=review
> > 
> > r=me with comments
> > 
> > > Source/JavaScriptCore/runtime/JSObject.cpp:1500
> > > +            if (entry->attributes() & DontDelete && exec->vm().deletePropertyMode() != VM::DeletePropertyMode::IgnoreConfigurable)
> > 
> > Nit: You can use the local VM& reference.
> > 
> > > Source/JavaScriptCore/runtime/JSObject.cpp:1508
> > > +        if (attributes & DontDelete && exec->vm().deletePropertyMode() != VM::DeletePropertyMode::IgnoreConfigurable)
> > 
> > Ditto
> 
> Oops, fixed!
> 
> > > Source/JavaScriptCore/runtime/VM.h:360
> > > +    class DeletePropertyModeScope {
> > 
> > Not super important, but this could also be written more succinctly as a
> > class with a single member variable that is of type
> > SetForScope<DeletePropertyMode>
> 
> Interesting! – hadn't come across that.
> 
> Holding of for now, I think that would mean making the member public? - and
> I'm loath too do so since that would open up to other errors. Maybe we can
> work on a way to make a type that generalizes a scoped set, while supporting
> private.
Yeah that's a good idea in general. Not sure how we can device such a class. I wasn't thinking about the fact that it would need to be made public.

> 
> Committed revision 201654.