Bug 158295 - JSGlobalObject::addFunction should call deleteProperty rather than removeDirect
Summary: JSGlobalObject::addFunction should call deleteProperty rather than removeDirect
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gavin Barraclough
URL:
Keywords:
Depends on:
Blocks: 158178
  Show dependency treegraph
 
Reported: 2016-06-02 00:56 PDT by Gavin Barraclough
Modified: 2016-06-03 13:33 PDT (History)
8 users (show)

See Also:


Attachments
Fix (9.68 KB, patch)
2016-06-03 00:46 PDT, Gavin Barraclough
no flags Details | Formatted Diff | Diff
Fix (9.89 KB, patch)
2016-06-03 00:50 PDT, Gavin Barraclough
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 2016-06-02 00:56:51 PDT
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).
Comment 1 Gavin Barraclough 2016-06-03 00:46:02 PDT
Created attachment 280431 [details]
Fix
Comment 2 Gavin Barraclough 2016-06-03 00:50:11 PDT
Created attachment 280433 [details]
Fix
Comment 3 Saam Barati 2016-06-03 01:43:40 PDT
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>
Comment 4 Gavin Barraclough 2016-06-03 13:14:12 PDT
(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.
Comment 5 Saam Barati 2016-06-03 13:33:10 PDT
(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.