Closed
Bug 698551
Opened 13 years ago
Closed 13 years ago
Nodelist prototype property getters/setters are called with wrong |this|
Categories
(Core :: JavaScript Engine, defect, P1)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla10
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
(Keywords: regression)
Attachments
(3 files, 1 obsolete file)
See the attached testcase. It should alert "[object HTMLBodyElement]", not undefined, and does in fx7.
This would be really easy to fix in the nodelist proxy code, except that the JS engined doesn't expose the APIs that do the right thing (that is, looking up the prop on the proto but using the proxy itself as |this|) to consumers.
I suspect we need to fix this for fx10....
Assignee | ||
Comment 1•13 years ago
|
||
Assignee | ||
Comment 2•13 years ago
|
||
We forward the fast paths to proto if it's been tampered with, so this testcase throws when it shouldn't.
Assignee | ||
Comment 3•13 years ago
|
||
I'm happy to take this given some indication of what public APIs would be acceptable here for jseng....
Comment 4•13 years ago
|
||
JSBool JS_ProxyGetPropertyTo(JSContext* cx, JSObject* obj, T propname, JSObject* onBehalfOf, jsval* vp);
perhaps, conditioned on a comment by it in jsapi.h explaining the exact meanings of obj and onBehalfOf, could do it.
Assignee | ||
Comment 5•13 years ago
|
||
Where T is a jsid for now? Or would you prefer separate uint32 and property name variants? If so, what should T be for the name variant?
Hardware: ARM → All
Version: 7 Branch → Trunk
Comment 6•13 years ago
|
||
T would be jsid and uint32 for now. I need to add JSPropertyName to JSAPI so there could be a third variant for property names known not to be uint32 (and not special, but nothing anyone cares about is special) before you could add a JSPropertyName variant.
Assignee | ||
Comment 7•13 years ago
|
||
Now that I look at the code, maybe the api should use 'Forward' instead of 'Proxy'?
Attachment #570841 -
Flags: review?(peterv)
Attachment #570841 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 8•13 years ago
|
||
Attachment #570842 -
Flags: review?(peterv)
Attachment #570842 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•13 years ago
|
Attachment #570841 -
Attachment is obsolete: true
Attachment #570841 -
Flags: review?(peterv)
Attachment #570841 -
Flags: review?(jwalden+bmo)
Assignee | ||
Updated•13 years ago
|
Assignee: general → bzbarsky
Priority: -- → P1
Whiteboard: [need review]
Assignee | ||
Updated•13 years ago
|
tracking-firefox10:
--- → ?
Keywords: regression
Comment 9•13 years ago
|
||
Comment on attachment 570842 [details] [diff] [review]
Including the test file too
Review of attachment 570842 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jsapi.h
@@ +3431,5 @@
> extern JS_PUBLIC_API(JSBool)
> JS_GetPropertyByIdDefault(JSContext *cx, JSObject *obj, jsid id, jsval def, jsval *vp);
>
> extern JS_PUBLIC_API(JSBool)
> +JS_ProxyGetPropertyTo(JSContext *cx, JSObject *obj, jsid id, JSObject *onBehalfOf, jsval *vp);
I suggested this name. In isolation it's a reasonable name. But now that I think about it, this kind of clashes a bit with Proxy objects and all that nonsense. What do you think of JS_ForwardGetPropertyTo instead, mutatis mutandis for element?
Attachment #570842 -
Flags: review?(jwalden+bmo) → review+
Comment 10•13 years ago
|
||
Ahahaha, we had the same idea! :-D That's what I get for not reading bug commentas.
Assignee | ||
Comment 11•13 years ago
|
||
> Ahahaha, we had the same idea!
Yep. I'll make that change. ;)
Updated•13 years ago
|
Attachment #570842 -
Flags: review?(peterv) → review+
Assignee | ||
Comment 12•13 years ago
|
||
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla10
Comment 13•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Updated•13 years ago
|
status-firefox10:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•