762471 - getOwnPropertyDescriptor on quickstubbed prototypes invokes the PropertyOp getter

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

This breaks the SpecialPowers wrapper. Investigating.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Test. v1

Attaching a testcase demonstrating the problem.

Comment 2

[Bobby Holley (:bholley)]

The basic problem here is that quickstubs uses JSPropertyOps for getters and setters. This breaks my SpecialPowers.wrap code when it tries to traverse the proto chain while looking up a property. When it calls Object.getOwnPropertyDescriptor(proto, 'foo'), the JS engine doesn't find JSPROP_GETTER, so it assumes that we must be dealing with a value descriptor, and invokes the underlying property op. But this _calls_ the QuickStubbed getter, which throws because the |this| argument is wrong.

I'd very much like to fix this, because otherwise my SpecialPowers wrapper fails for seemingly arbitrary reasons. Jorendorff and I discussed two approaches on IRC:

1 - Teach the JS engine about reification. We move the reification code to js/src, add a flag on the JSClass saying that the PropertyOps need to be reified, and invoke reification in the appropriate places.

2 - Convert QuickStubs to JSNatives and nuke reification (and a whole class of bugs) from orbit.


My understand is that #2 is ideal, but has unknown performance implications. Boris, you've been working with this sort of thing for the new bindings, right? Are JSNatives fast enough? Is this a path worth pursuing, or should we just limp along until quickstubs go away entirely in a year or two?

Comment 3

[Jeff Walden [:Waldo]]

If we really needed reification for perf reasons, it would be better if the native methods we invoked were JSNatives.  At least then there'd be no ambiguity about how to represent such beasts, or how to replace shapes referring to them with shapes containing actual functions, in the event of their being inspected.  I still think 2 is feasible, tho, with some work.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> When it calls Object.getOwnPropertyDescriptor(proto, 'foo')

Then you hit bug 560072, yes?

> Boris, you've been working with this sort of thing for the new bindings, right? Are
> JSNatives fast enough?

At this point, I believe IC support in JM is the same for both JSNatives and JSPropertyOps.  So the actual invocation is probably fast enough (or at least not much slower; it's reasonably for getters and abysmal for setters for both JSNatives and JSPropertyOps).

The thing that may be slower (and more bloaty) is prototype setup, since it would now involve creating somewhere between dozens and hundreds of function objects per DOM prototype.  Since we're moving toward that with the new DOM bindings anyway, I'm hoping it's not too bad, but maybe measure?

Comment 5

[Bobby Holley (:bholley)]

I've decided just to work around this for now with bug 764315.

Comment 6

[Boris Zbarsky [:bzbarsky]]

This is totally fixed; we moved quickstubs to JSNative getter/setter pairs.