Last Comment Bug 715821 - Don't have __define[GS]etter__ use CheckRedeclaration handle conflicts
: Don't have __define[GS]etter__ use CheckRedeclaration handle conflicts
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla12
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
Mentors:
Depends on: 718187 718541
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-05 22:45 PST by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2012-01-17 03:30 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (6.95 KB, patch)
2012-01-05 22:45 PST, Jeff Walden [:Waldo] (remove +bmo to email)
bhackett1024: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-05 22:45:05 PST
Created attachment 586349 [details] [diff] [review]
Patch

CheckRedeclaration is standing in the way of property-splitting work I'm doing.  Moreover, its behavior doesn't clearly correspond to anything in the spec, which makes it more than a bit confusing.  I plan to remove it, and to replace its uses with code that more clearly corresponds to spec algorithms.  (Or to nothing at all, for several calls which are near-vestigial now, after a couple other changes.)

The first step is to remove the CheckRedeclaration calls outside the parser.  There are only two -- in __defineGetter__ and __defineSetter__.  It seems clearest to do these in a separate patch from the parser bits.

***

The semantics of these, and passing { get: arguments[1], enumerable: true, configurable: true } as the descriptor object, are a little tricky, so I'll try to run through the oddities of our semantics, and how this patch mostly preserves them.

== No existing property ==

If there's no property at all, __d[GS]__ creates a new enumerable configurable property with only the half specified.  So the descriptor we pass in has to have those two bits set in it.  That's the easy part.

== Existing accessor property ==

If there's an existing accessor descriptor, __d[GS]__ updates the appropriate method half.  This patch obviously does that.  More interestingly, the current implementation passes enumerable/configurable as true/true.  Because O.dP merges specified attributes into the updated descriptor, explicitly specifying them here does the trick.

== Existing data property ==

Otherwise, there's an existing data descriptor.  In the old code, for CheckRedeclaration to permit the change, we must have ((oldAttrs | attrs) & JSPROP_READONLY) == 0.  That constant-folds to ((oldAttrs & JSPROP_READONLY) == 0.  So it doesn't allow changes to existing, non-writable properties, regardless of configurability.  But ES5 explicitly says that [[DefineOwnProperty]] is allowed to make such changes.  See the note at the end of ES5 8.12.9:

http://es5.github.com/#x8.12.9

This patch thus permits changes to non-writable but configurable properties.  I think these semantics are preferable given the spec statement that [[DefineOwnProperty]] allows such changes.  It's also worth noting that v8 implements __defineGetter__ and such with the overwrite-non-writable-but-configurable semantics:

http://code.google.com/p/v8/source/browse/branches/bleeding_edge/src/v8natives.js?r=10149#280

That leaves the configurable bit in the existing property to be addressed.  We're not allowed to modify non-configurable properties, and O.dP will properly see that the existing property is non-configurable and will throw.

***

Those are the patch semantics.  Tests pass with the patch, with a few little changes to existing tests.

First, previously __defineGetter__ and such could add properties to ArrayBuffer instances, despite those instances appearing to be non-extensible.  That was wrong, and this patch fixes that.  I removed the test in question, since when such definitions are prohibited, there's nothing left of the test that's meaningful.

Second, I changed the errors __defineGetter__ throws to be TypeErrors.  I think this makes more sense semantically, and it makes us consistent with v8's algorithm done using an Object.defineProperty kernel.  This required one test be changed to expect a TypeError rather than a SyntaxError.

Third, because different code's generating some of the errors now, an error message changed.  I had to update one (extension-y) test for this.

***

Hopefully that should explain all you need to know to review this.  If you still lack any confidence, I suggest you compare this patch's implementation to the v8 one noted above -- they're pretty much carbon copies of each other, which hopefully will provide whatever confidence you might still lack in it.  :-)
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-09 15:40:20 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b102e74b3b8

It belatedly occurs to me that I should have written some tests for this.  Please keep this bug open until I do that, posthaste.
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-09 22:21:47 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/51cc1b5c935a

Those are some tests, I think we're good now -- close this when this changeset gets merged to m-c.
Comment 3 Marco Bonardo [::mak] (Away 6-20 Aug) 2012-01-10 01:56:43 PST
https://hg.mozilla.org/mozilla-central/rev/0b102e74b3b8
Comment 4 Ed Morley [:emorley] 2012-01-10 11:52:24 PST
https://hg.mozilla.org/mozilla-central/rev/51cc1b5c935a

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