Bug 78409 – Move special __proto__ property to Object.prototype

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

[Gavin Barraclough]

Re-implement this as a regular accessor property.  This has three key benefits:
1) It makes it possible for objects to be given properties named __proto__.
2) Object.prototype.__proto__ can be deleted, preventing object prototypes from being changed.
3) This largely removes the magic used the implement __proto__, it can just be made a regular accessor property.

[Gavin Barraclough]

Created attachment 126622 [details]
Patch

[WebKit Review Bot]

Attachment 126622 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/JavaScriptCore/runtime/Structure.h:149:  is__proto__ is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 126622 [details]
Patch

Attachment 126622 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11508044

New failing tests:
fast/js/cyclic-prototypes.html

[Oliver Hunt]

Comment on attachment 126622 [details]
Patch

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

Waiting for answers to questions before r+/-ing

> Source/JavaScriptCore/parser/Parser.cpp:-777
> -        failIfTrueWithMessage(*name == m_globalData->propertyNames->underscoreProto, "Cannot name a function __proto__");

Removing this should break tests -- i recall it hosing initialization of program code, but maybe i'm wrong?  I suppose global initialization is now purely putDirect, so maybe this is okay?

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:210
> +    protoAccessor->setGetter(exec->globalData(), JSFunction::create(exec, this, 0, Identifier(), globalFuncProtoGetter));
> +    protoAccessor->setSetter(exec->globalData(), JSFunction::create(exec, this, 0, Identifier(), globalFuncProtoSetter));

So the decision was anonymous functions?

> Source/JavaScriptCore/runtime/JSObject.cpp:222
> -    structure()->setHasGetterSetterProperties(true);
> +    structure()->setHasGetterSetterProperties(propertyName == globalData.propertyNames->underscoreProto);

I don't get this change at all -- it seems to drop the getter/setter existence in all cases but the one where we're setting __proto__ - this probably passes tests because all test objects probably have the object prototype so this has poisoned everything.  If it doesn't impact performance (presumably the case seeing as you've put this up for review :D )  I don't see any reason __proto__ needs t be special cased.

> Source/JavaScriptCore/runtime/JSObject.h:107
> +        JS_EXPORT_PRIVATE static bool allowsAccessFrom(JSObject*, ExecState*);

This is a fairly generic name -- i can imagine in future we'd want this name to be used for cross-origin checks, etc, will that still be okay then?

[Gavin Barraclough]

(In reply to comment #4)
> > Source/JavaScriptCore/parser/Parser.cpp:-777
> > -        failIfTrueWithMessage(*name == m_globalData->propertyNames->underscoreProto, "Cannot name a function __proto__");
> 
> Removing this should break tests -- i recall it hosing initialization of program code, but maybe i'm wrong?  I suppose global initialization is now purely putDirect, so maybe this is okay?

Yes - see the changes in the patch to LayoutTests/fast/js/parser-syntax-check-expected.txt – these now work.  And I think it makes sense this way, now that we basically no longer need there to be anything magic about __proto__.  (Except, possibly, for object literal behaviour... but even there I wonder if it is really required for web compatibility...)

> > Source/JavaScriptCore/runtime/JSGlobalObject.cpp:210
> > +    protoAccessor->setGetter(exec->globalData(), JSFunction::create(exec, this, 0, Identifier(), globalFuncProtoGetter));
> > +    protoAccessor->setSetter(exec->globalData(), JSFunction::create(exec, this, 0, Identifier(), globalFuncProtoSetter));
> 
> So the decision was anonymous functions?

There is no consensus on the ECMA list at the minute.  This change would be a step in the right direction whichever way the committee goes, since this moves __proto__ to the Object Prototype and makes in configurable (there seems to be agreement on both of these changes).

The only real outstanding question is whether this should cleanly be specified as an accessor, or whether we'll adopt Firefox's evil-magic-data-property hack.  There doesn't seem to be a broad base of support for the latter.  If the spec does go that way, we'd have three options: (1) reintroduce some evil hack into the object prototype classes (2) keep it as an accessor, add a new attribute to let the accessor masquerade as a data descriptor for getOwnPropertyDescriptor, or (3) don't follow the spec – it'll only be a normative-optional annex anyway, and we should ask serious questions before adopting such a bad de-jure spec.  But I don't think it needs get to this point – I think we should just resist any spec that demands this be an ugly hack.

> > Source/JavaScriptCore/runtime/JSObject.cpp:222
> > -    structure()->setHasGetterSetterProperties(true);
> > +    structure()->setHasGetterSetterProperties(propertyName == globalData.propertyNames->underscoreProto);
> 
> I don't get this change at all -- it seems to drop the getter/setter existence in all cases but the one where we're setting __proto__ - this probably passes tests because all test objects probably have the object prototype so this has poisoned everything.  If it doesn't impact performance (presumably the case seeing as you've put this up for review :D )  I don't see any reason __proto__ needs t be special cased.

Please see the changes in Structure.h – I'm now separately tracking whether the object had any accessors at all, and whether it has any accessors other than __proto__.  This means that hasGetterSetterProperties() returns the right thing (true for Object.prototype, due to __proto__), but JSValue::put can still be optimized (checking a combination of hasGetterSetterPropertiesExcludingProto(), and testing whether the identifier is __proto__).

> > Source/JavaScriptCore/runtime/JSObject.h:107
> > +        JS_EXPORT_PRIVATE static bool allowsAccessFrom(JSObject*, ExecState*);
> 
> This is a fairly generic name -- i can imagine in future we'd want this name to be used for cross-origin checks, etc, will that still be okay then?

Yes! – this is the cross-origin check! – see changes in JSDOMWindowBase. :-)

[Gavin Barraclough]

Fixed in r107498

[Csaba Osztrogonác]

(In reply to comment #6)
> Fixed in r107498

It made two tests fail, could you check https://bugs.webkit.org/show_bug.cgi?id=78434 ?

[Gavin Barraclough]

Temporarily reverted in r107544, I think there is probably a trivial bug here – the cross-domain check in the accessor likely needs to test against the caller's security origin, rather than the callee's.

Also, should probably fix https://bugs.webkit.org/show_bug.cgi?id=78438 at the same time.

[Gavin Barraclough]

Created attachment 127828 [details]
Fix layout test regressions.

The cross origin access check wasn't working properly in the last patch, since it is only overridden in the window object we need to explicitly apply the check to the object we with to access's global object.  To prevent this error in the future, I moved the 'virtual' trap to check this from the ClassInfo table onto the GlobalObject's method table.

[WebKit Review Bot]

Comment on attachment 127828 [details]
Fix layout test regressions.

Attachment 127828 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11549289

New failing tests:
fast/js/cyclic-prototypes.html
fast/dom/Window/window-custom-prototype.html

[Gavin Barraclough]

Fixed in r 108259, r 108260.

[Gavin Barraclough]

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