Steps: In http://www.squarefree.com/shell/shell.html, paste Object.getOwnPropertyDescriptor(document.__proto__, "body") Result: NS_ERROR_XPC_BAD_OP_ON_WN_PROTO on line 1: Illegal operation on WrappedNative prototype object Expected: A property descriptor that includes a getter function. This prevents me from replacing all uses of the deprecated __lookupGetter__ with getOwnPropertyDescriptor.

Bug 520882 mentions this exception, so maybe this is a dup?

I'm not sure whether something in the JS engine should be fixed for this or not. If something should be, I think bug 520882 is probably the right place to cover it, and any DOM-code followup to respond to the JS engine change (if one were needed) would be pretty simple. Something definitely needs to be fixed in DOM/XPConnect for this, however (and given bug 520882's existence, it seems reasonable to morph this bug to cover just the DOM aspects of the problem). The problem follows from these details. The tagName property is represented through a data-descriptor-looking native C++ getter. We reflect this as a data descriptor, and to get the value we necessarily must invoke the getter function. But, for some reason, this effective evaluation of |Element.prototype.tagName| throws an exception when it tries to unwrap |this| in the C++ getter function: static JSBool nsIDOMElement_GetTagName(JSContext *cx, JSObject *obj, jsid id, jsval *vp) { XPC_QS_ASSERT_CONTEXT_OK(cx); nsGenericElement *self; xpc_qsSelfRef selfref; if (!xpc_qsUnwrapThis(cx, obj, nsnull, &self, &selfref.ptr, vp, nsnull)) return JS_FALSE; We end up hitting that return-false case; I didn't step deep enough to figure out why. (That's if "why" would even be evident to someone not familiar with DOM wrapping. I'm sure peterv and others can understand and identify where things go "astray" much faster, so I'm not going to try.) *That* aspect of this issue is certainly not a JS engine issue. But whether or not a JS engine fix would make that aspect of this moot, I can't say. Given that IE is using this behavior in its native-DOM developer marketing material, and given that no other browser has our behavior here (and shouldn't -- although what behavior should happen, I can't tell from reading the WebIDL spec, whose section 4.3.5 is inscrutable to me on this point at a quick read), I think we should seriously consider blocking on this. Also worth considering is that browsers don't agree on the full elaboration of all this already: I gather (can't test now) that IE9 represents DOM properties with accessors on interface prototypes (so looking up a property on an instance gives nothing, on the prototype gives an accessor), WebKit and v8 represent them with a class hook which makes them appear as own data properties of instances of the interface (and due to the mis-nature of that hook, such properties can't actually be properly deleted) and strangely nowhere at all on interface prototypes, and of course we expose them as own data properties and fall over hard "exposing" them on interface prototypes. Although, given the late date, I wouldn't be surprised at a minus, or at a plus for some sort of halfway-hackaround that makes just Object.getOwnPropertyDescriptor work -- but maybe a hackaround (if one's called for) should be defined and addressed in a separate bug, if that's all we think we can do for this release cycle.

> The tagName property is represented through a data-descriptor-looking native > C++ getter. What does that mean? This seems like the crux of the problem... DOM properties are all conceptually properties on the proto that have non-idempotent getters and setters, and those getters/setters expect the |this| to be an object of the right type (NOT the proto!). Though there's some JSPROP_SHARED weirdness here in some cases, right? > But, for some reason, this effective evaluation of > |Element.prototype.tagName| throws an exception when it tries to unwrap > |this| Is the |this| jseng passes in Element.prototype in this case? 'cause that doesn't have a tagName, of course... > WebIDL spec, whose section 4.3.5 is inscrutable The relevant text is section 4.4 paragraph 1, which is totally incompatible with how DOM properties work in Gecko (and it sounds like IE9 too). I still think we should push back on this... I dunno that anyone thought much before writing that text.

(In reply to comment #4) > > The tagName property is represented through a data-descriptor-looking > > native C++ getter. > > What does that mean? This seems like the crux of the problem... Properties without JSPROP_GETTER or JSPROP_SETTER, but with non-JS_PropertyStub getter and/or setter property ops (as passed to JS_DefineProperty or whatever), are reflected as though they were data properties. Consider array length, for example: Object.getOwnPropertyDescriptor([], "length") gives you: { value: 0, writable: true, enumerable: false, configurable: false } and the property is represented with attributes not including JSPROP_GETTER or JSPROP_SETTER, but getter == array_length_getter and setter == array_length_setter (or rather, it used to, I think this changed recently). The DOM stuff is similar, so when you do this: Object.getOwnPropertyDescriptor(document.body, "childNodes") you get back: { value: <a nodelist>, writable: ..., enumerable: ..., configurable: ... } But in order to get that value, the only way the JS engine can have anything to return is to get the property value and therefore implicitly invoke nsIDOMElement_GetTagName or whatever, on the provided object. That works for |document.body|, "not so well" for one of the prototype objects. Really, these all should probably appear as JSPROP_GETTER/JSPROP_SETTER properties with function-valued getter and setter fields. Then getOwnPropertyDescriptor on the prototype would give you a normal accessor property (and, perhaps less expectedly, gOPD on non-prototype DOM objects would give you |undefined|). But we can't (right now) make that change fully generally, because then we'd change how array length is exposed from what ES5 requires to something that'd be wrong (an accessor property, rather than a proper data property). > DOM properties are all conceptually properties on the proto that have > non-idempotent getters and setters, and those getters/setters expect the > |this| to be an object of the right type (NOT the proto!). Yeah, the most natural mapping is what I suggest above, but that's not how we do it now, and I'm not sure if either way accords with WebIDL (or WebIDL as it will eventually end up). > Though there's some JSPROP_SHARED weirdness here in some cases, right? Possibly, I don't know/remember whether DOM properties have sharedness or not. > > But, for some reason, this effective evaluation of > > |Element.prototype.tagName| throws an exception when it tries to unwrap > > |this| > > Is the |this| jseng passes in Element.prototype in this case? 'cause that > doesn't have a tagName, of course... Yes, it is. > > WebIDL spec, whose section 4.3.5 is inscrutable > > The relevant text is section 4.4 paragraph 1, which is totally incompatible > with how DOM properties work in Gecko (and it sounds like IE9 too). Sort of reading between the lines (WebIDL needs to be ES5-ified), I think it wants an accessor on the prototype for every IDL property (constants excepted). So tagName would be roughly like: Object.defineProperty(Element.prototype, "tagName", { get: <function to get tag name>, enumerable: ..., configurable: ... }); This is certainly incompatible with us. It's also incompatible with WebKit/v8. I'm *guessing* this might be what IE9 does (really should check, but my desk currently suffers from a paucity of network drops and/or long enough cat5 cables). This seems "right" to me, but perhaps there's a better alternative. > I still think we should push back on this... I dunno that anyone thought > much before writing that text. Perhaps. I haven't followed WebIDL very closely, especially on this sort of stuff, so I have no idea how sensible or not it really is in general, on this topic. I get the feeling this comment is a bit jumbled, feel free to comment more/poke me on IRC for clarification.

> The DOM stuff is similar It is? For which DOM object and property? document.body.childNodes should have JSPROP_GETTER | JSPROP_SHARED. See ReifyPropertyOps in xpcquickstubs.cpp for the quickstub version and the end of DefinePropertyIfFound in xpcwrappednativejsops.cpp for the non-quickstub version. > "not so well" for one of the prototype objects. Well, right. HTMLBodyElement.prototype ain't got any useful childNodes to return! > Possibly, I don't know/remember whether DOM properties have sharedness or not. They do; see above. > Sort of reading between the lines (WebIDL needs to be ES5-ified), I think it > wants an accessor on the prototype WebIDL wants an accessor on the nodes themselves, not on the proto. But I think it's daft, so I don't think we should worry about it for now. > I get the feeling this comment is a bit jumbled Heh. Let's talk on IRC; there seems to be some serious confusion here...

Okay. So -- and neither bz nor I knew all this (!) -- non-quickstubbed properties are exposed as having JS-function-valued getters (and setters, if needed). And quickstubbed properties are exposed -- by customized __lookup[GS]etter__ properties only -- as having JS-function-valued getters (and setters, if needed). So it seems we have precedent for converting how we expose these properties so that they appear to be JS-function-valued properties on DOM prototype objects. Consistency suggests we do the same for quickstubbed properties. I suspect -- but don't know -- that IE9 exposes properties as getter/setter, like our non-QS stuff does and like our QS stuff does if poked via __lookup[GS]etter__. This is *not* what WebIDL says -- that spec seems to say all these properties are accessors directly on individual instances -- but bz and I both think that's wrong because it means you can't override the original accessor property on the prototype to make it be something different (say, to hotfix a DOM property or something -- less necessary for us than for other engines, but still a desirable thing to support). So to summarize: we change quickstubbed DOM properties to be defined (by xpc_qsDefineQuickStubs) as JSPROP_GETTER (and JSPROP_SETTER if necessary) properties with JS functions corresponding to the current get/set native functions. This would fix the IE test, it would make quickstubbed properties act like non-quickstubbed properties, and it would make quickstubbed properties via gOPD act like they do via the custom __lookup[GS]etter__. (I think we could also kill the custom __lookup[GS]etter__ at that point, because there'd be no reason for it to exist or do anything special to reify quickstubbed properties.) I'll take this and start working on it. Comment 0 makes a good point that Object.getOwnPropertyDescriptor isn't a full-fledged replacement for __lookupGetter__/__lookupSetter__ without this, and the IE test is only further incentive to fix this (I'd really like to know how our "enhanced DOM capabilities" compare to IE).

Jason, was there a reason we didn't just create those Functions to start with for quickstubs? As in, will something in tracer integration break if we make the change Waldo proposes?

My only worry is a potential perf change, but I might well be missing something. I'll definitely run things by tryserver here for some perf comparisons.

"break" would include not tracing DOM property access... ;)

Hm. if (shape->hasSetterValue()) RETURN_STOP("can't trace JavaScript function setter"); I had thought this was not the case, and that we had implemented such tracing, but it seems not to be. :-\ (It appears at a glance that function getters are fine, but I might be skimming jstracer.cpp too hard.) jorendorff points me at bug 559813 for tracing function-valued setters; I'll look into how much work it would be to push that (and the bug 559653 that blocks it) to completion, then decide where to go from there.

Cc'ing Cameron. /be

(In reply to comment #7) > I suspect -- but don't know -- that IE9 exposes properties as getter/setter, > like our non-QS stuff does and like our QS stuff does if poked via > __lookup[GS]etter__. Someone test IE9 or see Travis Leithead's posts to public-script-coord (or both in case Travis is aiming beyond what's in the IE9 beta). > This is *not* what WebIDL says -- that spec seems to say > all these properties are accessors directly on individual instances -- but bz > and I both think that's wrong because it means you can't override the original > accessor property on the prototype to make it be something different (say, to > hotfix a DOM property or something -- less necessary for us than for other > engines, but still a desirable thing to support). How could you override if the accessor not configurable? Or is the idea to make it configurable? /be

Is there any reason not to make it configurable?

The reasoning behind putting the properties corresponding to IDL attributes on the instances themselves is because Web IDL used to target ES3, which meant that you didn't have getter/setters. If you considered that the property itself was really storing that attribute's state (and not just faking it through a custom [[Get]]/[[Put]]) then each instance would need its own property. It does work better with ES5 accessor properties on the prototype. On the face of it, it makes sense to me to make them configurable, just like properties corresponding to operations are. (They are, right?)

(In reply to comment #15) > On the face of it, it makes sense to me to make them configurable, just like > properties corresponding to operations are. Sorry, you mean methods by "properties corresponding to operations"? If so, yes: the DOM is pretty mutable. There shouldn't be too much locked down, but I do see some JSPROP_PERMANENT (non-configurable) usage in js/src/xpconnect/*/*.cpp. Those grep hits need looking into. /be

(In reply to comment #16) > Sorry, you mean methods by "properties corresponding to operations"? Yeah, that is what I meant. (The properties that exist on prototypes due to operations found in the IDL.)

> Those grep hits need looking into. If I exclude xpcsample, those are: wrappers of various sorts, stuff hanging off the Components object, properties of nsIIID objects, properties of NoHelper XPConnect-wrapped objects (no classinfo, in other words; doesn't apply to DOM objects), properties of NoMods XPConnect prototypes (cases where classinfo says to not allow modification of prototypes; DOM objects say to allow such modification) and properties XPConnect WrappedNative tearoffs; I believe these are not relevant to what happens if DOM properties are modified on DOM prototypes. So we look all good. ;)

Can someone summarize what needs to be done here, and why this needs to block 2.0?

jst, what needs to happen, ideally, is that quickstubs stop pretending that DOM properties are data-only properties as far as JS introspection APIs are concerned. The only reason for it to block 2.0 is that the ietestdrive site uses this sort of thing extensively and basically makes claims about us not supporting standards, etc (though this stuff isn't actually standardized in webidl yet). But the fact that quickstubbed vs not DOM properties behave differently here seems like a bug no matter what.

I wouldn't say it's just the IE site, although that's certainly some of it. Our DOM is simply not compatible at the moment with standard ECMAScript meta-object operations, of the sort developers are beginning to use now that ES5 is being implemented by browsers. It *should* be the case that __{lookup,define}{G,S}etter__ (the ones defined on the DOM prototypes, that is) are consistent with the standard, recommended-by-all (except us?) Object.defineProperty and Object.getOwnPropertyDescriptor. It's not. By hook or by crook I think it needs to be consistent, even if, quite possibly, it ends up inconsistent with some future version of the WebIDL specification. In the past when I've thought about this I've considered a minor JS engine hack to make it work: add a property attribute bit to encode data-or-accessor information for properties with native getters and setters. It's a hack, but it's relatively easy, not too invasive, and makes things appear reasonably sane -- should be doable in a day or so. I'll get on that.

(In reply to comment #21) > It *should* be the case that > __{lookup,define}{G,S}etter__ (the ones defined on the DOM prototypes, that is) > are consistent with the standard, recommended-by-all (except us?) > Object.defineProperty and Object.getOwnPropertyDescriptor. It's not. By hook > or by crook I think it needs to be consistent, even if, quite possibly, it ends > up inconsistent with some future version of the WebIDL specification. A new draft of Web IDL is set to be published in the next few days which requires IDL attributes to be exposed as accessor properties on prototypes. Various people have requested this, so I think this behaviour is going to stay, and thus we won't be inconsistent with the spec.

Somewhat surprisingly, this patch is actually a net reduction in lines of code. There's still more to do for this -- maybe need to reify JSPROP_ACCESSOR properties in DefinePropertyOnObject (or maybe in js_obj_defineGetter, or maybe somewhere upstream of both), need to see how it does on test suites, need to write JSAPI tests for the bit, need to investigate how it interacts with obj_{,un}watch, and so on -- but this *does* allow the IE test page to run to completion for 15/24. The failures at a quick glance-through appear to be: Piece 5: Not Supported -- constructor property included on DOM prototype objects Piece 8: Not Supported -- Prototype objects have proper DOM inheritance Piece 9: Not Supported -- DOM functions created on appropriate prototypes Piece 9: Not Supported -- DOM functions created on appropriate prototypes Piece 11: Not Supported -- DOM functions can be deleted Piece 12: Not Supported -- DOM function declarations can be executed with call Piece 16: Not Supported -- toString works consistently on DOM objects Piece 21: Not Supported -- DOM properties (ES5 accessors) created on appropriate prototypes Piece 24: Not Supported -- DOM accessors can be deleted Of course anyone vaguely familiar with xpconnect knows these failure messages can't possibly be completely accurate, because we definitely do some of this stuff. The actual reasons for the failures are: 0) toString fails because I'm running in a debug build that puts extra spew in document.toString(), or so I expect. (one failure) 1) No Node.prototype.appendChild (causes or obscures several failures). 2) No Node.prototype.nodeName (one failure). 3) Element.prototype.tagName isn't the only tagName accessor descriptor in existence, with respect to how document.body and its prototypes appear to behave in JS (one failure). On a last note, WebIDL will probably (rightly, I think) say readonly properties should have a getter and will specify setter === undefined, but we currently map it to xpc_qsThrowGetterSetterFailed purely for consistency with non-quickstubbed properties. So if that changes, it's going to be a broader change than a qsgen.py one-liner, and I plan to leave it for another bug if a change is needed. (Although, just testing now, something's odd -- document.body.tagName = "foobar" isn't triggering a throw, something to investigate before this wraps up, definitely.)

This includes some JSAPI compiled-code tests (which of course embed some JS script that runs to test various things), and those tests all pass (they definitely wouldn't all have passed with the previous patch). The shell tests also all pass (at least modulo the ones that don't pass in a browser-build JS shell). Next step is probably some Mochitesting, then it should be good to review. That said, I'm somewhat less certain now that this is preferable to just fixing the hard bugs in the way of tracing through setters, to allow us to just implement this the "simple" way and make ever DOM property be a true accessor with real functions as its fields. Getting this all to work with all the various look-up-the-property or define-over-the-property methods is non-trivial, and there are enough places where fixups must be made that it's possible we could miss one. So I may try resurrecting those old patches just to see how bad it is, then we can compare that approach to this one. But for now, what's here seems probably workable.

Comment on attachment 485467 [details] [diff] [review] Somewhat less partial patch, no known bugs, many tests but no Mochitests See comment 24; thoughts on whether this approach really makes more sense, and is more tractable and frail, particularly for a full stable-release cycle, than just fixing bug 559653 and then bug 559813?

(er, more tractable and *less* frail)

Comment on attachment 485467 [details] [diff] [review] Somewhat less partial patch, no known bugs, many tests but no Mochitests >+namespace { You and Luke need to come to agreement and someone update the style guide. Luke said his piece (bug 604504 comment 16). No response and then this patch looks like a style war, which is not allowed. If you both agree that either style is ok, get jorendorff to confirm that non-solution solution :-/. >+ PropertyOp getOp, setOp; >+ if (shape->isSyntheticAccessor()) { >+ JSObject *acc; >+ >+ Value idval = IdToValue(id); >+ JS_ASSERT(!idval.isUndefined()); >+ JSAtom *nameAtom = idval.isString() ? idval.toString()->asAtom() : NULL; >+ >+ if (attrs & JSPROP_GETTER) { >+ getOp = getter; >+ } else { >+ if (!shape->hasDefaultGetter()) { >+ if (!GeneratePropertyOp(cx, obj, idval, 0, nameAtom, shape->getterOp(), &acc)) >+ return false; >+ attrs |= JSPROP_GETTER; >+ getOp = JS_DATA_TO_FUNC_PTR(PropertyOp, acc); >+ } else { >+ getOp = NULL; >+ } >+ } >+ >+ if (attrs & JSPROP_SETTER) { >+ setOp = setter; >+ } else { >+ if (!shape->hasDefaultSetter()) { >+ if (!GeneratePropertyOp(cx, obj, idval, 1, nameAtom, shape->setterOp(), &acc)) >+ return false; >+ attrs |= JSPROP_SETTER; >+ setOp = JS_DATA_TO_FUNC_PTR(PropertyOp, acc); >+ } else { >+ setOp = NULL; >+ } >+ } >+ } else { >+ getOp = (attrs & JSPROP_GETTER) ? getter : shape->getter(); >+ setOp = (attrs & JSPROP_SETTER) ? setter : shape->setter(); >+ } >+ >+ shape = obj->changeProperty(cx, shape, attrs, JSPROP_GETTER | JSPROP_SETTER, >+ getOp, setOp); Nit: eliminate getOp and setOp and instead update getter and setter only if they need resetting. /be

(In reply to comment #27) > You and Luke need to come to agreement and someone update the style guide. > Luke said his piece (bug 604504 comment 16). No response and then this patch > looks like a style war, which is not allowed. No style war, I just haven't touched the style of this patch from what it was before that discussion even started. > Nit: eliminate getOp and setOp and instead update getter and setter only if > they need resetting. Will look at this. For now, the feedback request is about the entire approach, versus doing it all the right way per bug 559653 and then bug 559813 -- I don't plan to make more effort at a reviewable patch if the idea itself isn't good.

Approach looks good, sorry I forgot to say that. This is a nice generalization from the XPConnect code and older, similar watchpoints code (jimb has had to debug it lately). I defer to jorendorff but wanted to give a thumbs up in general. /be

Yeah, the *idea* seems fine conceptually to me, as a stopgap measure. It's when I started getting into implementing hoary edge cases with fixup code inserted in various semi-random places (of which I'm not sure I've found all of them, and they introduce a distinction between visible-to-JSAPI and visible-to-script that I'm not sure is wise and seems hard to distinguish "correctly") that it started feeling like the emperor and his new clothes. And any embedder (or even us, if we're not all careful of this quirk) who writes his own, entirely reasonable, introspection methods might (if he exposed property attribute bits in certain ways) have to deal with this stuff -- perhaps his migration burden, but still more special-case code in non-obvious locations.

If we do what WebIDL will spec (has spec'ed?) then do we need this patch? What is the timing on the DOM changing to match the WebIDL spec? /be

The directional comments are good too. JSPropertyOp is old as the hills and from the slow natives days. Now that all natives are fast there is only a little gain in having JSPropertyOp (saves a function object) at the cost of this bug's patch (and perhaps more, if there are more gaps to fill) to fix metaprogrammability. /be

> What is the timing on the DOM changing to match the WebIDL spec? "_So_ post-2.0".

But we're trying to mock things up in advance of the WebIDL spec, to look like what we think Cameron will spec (with good confidence, based on interactions with various parties including other browser implementors on public-script-coord). So it seems worth asking. Waldo's comment 30 raises the issue. If this bug's patch to the JS engine has gaps and is large, could it be safer and simpler to patch the DOM glue that uses native accessors to use function-object accessors directly? Just asking, appreciate it may be hard to quantify. /be

Making that change on the DOM end is reasonably easy, I think. The hard work there is in the JS engine, because if we did that we'd stop being able to trace DOM setters (and not sure about DOM getters). See comment 11.... I don't think breaking tracing through these getters and setters is acceptable, so we need to either not break it or fix it.

Interestingly, it's not hard to quantify here, it seems to me, because we had mostly-working patches in bug 559653 and bug 559813 which would have solved this. Updated patches *probably* would not be significantly larger or fundamentally more complicated now than they were at the time of their attempted landings. (Well, except for methodjit implementations, but if my minimal experience with it is any guide, that's mostly simple copypasta.) But take a look at that first bug's patch to the tracer: https://bugzilla.mozilla.org/attachment.cgi?id=461270&action=diff#a/js/src/jstracer.cpp_sec1 Does that amount of change (looks like at least a near-rewrite, to me) to the property-setting path make you happier, or less happy, than the random spot-fixes the approach here requires? I favor cleaner, simpler code as a general rule, so I start as sympathetic to just doing it the right way. But I also recognize the reality that property setting, and the property cache, are intricate code that's likely to be buggy on first writing, and we're getting (if not already) late to do this. Too late? I don't know, and I can't say I really *know* the code to be confident of a guess. Wherefore the feedback request.

(In reply to comment #23) > (Although, just testing now, something's odd -- > document.body.tagName = "foobar" isn't triggering a throw, something to > investigate before this wraps up, definitely.) javascript:"use strict";try{document.body.tagName=0;}catch(x){x;} For me, pasting that in the location bar (in a nightly) gets: TypeError: setting a property that has only a getter In any case, where we're going is to just have this behave like any other accessor property with no setter. I'm up to date in this bug now and I'll look at the patch more closely tomorrow morning.

(In reply to comment #36) > Interestingly, it's not hard to quantify here, it seems to me, because we had > mostly-working patches in bug 559653 and bug 559813 which would have solved > this. Updated patches *probably* would not be significantly larger or > fundamentally more complicated now than they were at the time of their > attempted landings. (Well, except for methodjit implementations, but if my > minimal experience with it is any guide, that's mostly simple copypasta.) Right. I'd be surprised if methodjit needed any even slightly risky changes for bug 559653. Other than removing SetPropHit calls, the patch there makes no changes to the interpreter. > But > take a look at that first bug's patch to the tracer: > > https://bugzilla.mozilla.org/attachment.cgi?id=461270&action=diff#a/js/src/jstracer.cpp_sec1 > > Does that amount of change (looks like at least a near-rewrite, to me) to the > property-setting path make you happier, or less happy, than the random > spot-fixes the approach here requires? It is a near-rewrite, yes. I would *love* to see that code land.

Comment on attachment 485467 [details] [diff] [review] Somewhat less partial patch, no known bugs, many tests but no Mochitests Gotta love a patch that's over 50% tests. I really appreciate the work that went into this. Ultimately, I think we should either do it "the right way" or forgo this feature for Ff4. I am not scared of the patch in bug 559653. If it causes bugs, they'll be tracer bugs--a flavor of bug that several of us (myself included) spent many months becoming intimately familiar with, a kind we know how to fuzztest, minimize, track down, and speedily patch. Incidentally, you can subclass JSAPITest. See BEGIN_FIXTURE_TEST in tests.h. >+ jsvalRoot vobj(cx); >+ JSObject *obj = JS_NewObject(cx, NULL, NULL, NULL); >+ CHECK(obj); >+ vobj = OBJECT_TO_JSVAL(obj); >+ >+ CHECK(JS_DefineProperty(cx, global, "obj", vobj, >+ JS_PropertyStub, JS_PropertyStub, >+ JSPROP_ENUMERATE)); You can write: EVAL("var obj = {}; obj;", vobj.addr());

If we *do* go this route, a few comments on this patch (before I forget): As I understand it, if JSPROP_ACCESSOR is set on a property, JSPROP_PERMANENT must not be set. That is, we don't want to modify a js::Shape that has the PERMANENT bit set on it; so we should Reify before that happens. If so, add appropriate assertions, and add tests for Object.seal and freeze. If X is a transparent wrapper of Y, and using Object.getOwnPropertyDescriptor on Y would reify a property, then using it on X should reify it too. I think it would Just Work with this patch, but that is not obvious from the code, so it's worth testing.

(In reply to comment #39) > I am not scared of > the patch in bug 559653. If it causes bugs, they'll be tracer bugs--a flavor of > bug that several of us (myself included) spent many months becoming intimately > familiar with, a kind we know how to fuzztest, minimize, track down, and > speedily patch. I don't mean to be flip here about the prospect of causing more JS bugs before release. I don't think that patch will cause any bugs. It's some of the best code I've written here that hasn't landed.

Moving to .x, not holding 2.0 for this

(In reply to comment #41) > (In reply to comment #39) > > I am not scared of > > the patch in bug 559653. If it causes bugs, they'll be tracer bugs--a flavor of > > bug that several of us (myself included) spent many months becoming intimately > > familiar with, a kind we know how to fuzztest, minimize, track down, and > > speedily patch. > > I don't mean to be flip here about the prospect of causing more JS bugs before > release. I don't think that patch will cause any bugs. It's some of the best > code I've written here that hasn't landed. Yay, it landed. Only a few followup bugs so far. /be

We should probably move the JS work to bug 520882. This bug, as filed, is almost certainly fixed by bug 792215. Rebuilding now to verify that.

Fixed by bug 792215.

Landed a test: https://hg.mozilla.org/integration/mozilla-inbound/rev/e4cc85a0fe1e