Bug 78409 - Move special __proto__ property to Object.prototype
: Move special __proto__ property to Object.prototype
Status: RESOLVED FIXED
: WebKit
JavaScriptCore
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2012-02-10 21:46 PST by
Modified: 2012-02-20 13:54 PST (History)


Attachments
Patch (37.53 KB, patch)
2012-02-11 01:02 PST, Gavin Barraclough
oliver: review+
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Fix layout test regressions. (46.74 KB, patch)
2012-02-20 09:49 PST, Gavin Barraclough
oliver: review+
webkit.review.bot: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-02-10 21:46:15 PST
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.
------- Comment #1 From 2012-02-11 01:02:17 PST -------
Created an attachment (id=126622) [details]
Patch
------- Comment #2 From 2012-02-11 01:40:59 PST -------
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.
------- Comment #3 From 2012-02-11 02:21:24 PST -------
(From update of attachment 126622 [details])
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
------- Comment #4 From 2012-02-11 09:43:56 PST -------
(From update of attachment 126622 [details])
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?
------- Comment #5 From 2012-02-11 12:35:11 PST -------
(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. :-)
------- Comment #6 From 2012-02-11 18:49:26 PST -------
Fixed in r107498
------- Comment #7 From 2012-02-12 10:48:30 PST -------
(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 ?
------- Comment #8 From 2012-02-13 11:55:34 PST -------
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.
------- Comment #9 From 2012-02-20 09:49:47 PST -------
Created an attachment (id=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.
------- Comment #10 From 2012-02-20 10:22:11 PST -------
(From update of attachment 127828 [details])
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
------- Comment #11 From 2012-02-20 13:52:10 PST -------
Fixed in r 108259, r 108260.
------- Comment #12 From 2012-02-20 13:54:45 PST -------
*** Bug 78438 has been marked as a duplicate of this bug. ***