Last Comment Bug 698551 - Nodelist prototype property getters/setters are called with wrong |this|
: Nodelist prototype property getters/setters are called with wrong |this|
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla10
Assigned To: Boris Zbarsky [:bz] (Out June 25-July 6)
:
Mentors:
Depends on:
Blocks: 648801 698495
  Show dependency treegraph
 
Reported: 2011-10-31 12:52 PDT by Boris Zbarsky [:bz] (Out June 25-July 6)
Modified: 2012-01-19 08:57 PST (History)
9 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
Testcase (301 bytes, text/html)
2011-10-31 12:52 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
no flags Details
An even worse manifestation of this problem (403 bytes, text/html)
2011-10-31 12:58 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
no flags Details
When forwarding gets to the prototype in nodelists, make sure to use the right |this|. (5.78 KB, patch)
2011-10-31 14:17 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
no flags Details | Diff | Review
Including the test file too (7.11 KB, patch)
2011-10-31 14:18 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
jwalden+bmo: review+
peterv: review+
Details | Diff | Review

Description Boris Zbarsky [:bz] (Out June 25-July 6) 2011-10-31 12:52:00 PDT
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....
Comment 1 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-10-31 12:52:34 PDT
Created attachment 570801 [details]
Testcase
Comment 2 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-10-31 12:58:07 PDT
Created attachment 570806 [details]
An even worse manifestation of this problem

We forward the fast paths to proto if it's been tampered with, so this testcase throws when it shouldn't.
Comment 3 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-10-31 13:01:56 PDT
I'm happy to take this given some indication of what public APIs would be acceptable here for jseng....
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2011-10-31 13:06:22 PDT
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.
Comment 5 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-10-31 13:13:12 PDT
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?
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2011-10-31 13:34:48 PDT
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.
Comment 7 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-10-31 14:17:20 PDT
Created attachment 570841 [details] [diff] [review]
When forwarding gets to the prototype in nodelists, make sure to use the right |this|.

Now that I look at the code, maybe the api should use 'Forward' instead of 'Proxy'?
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-10-31 14:18:36 PDT
Created attachment 570842 [details] [diff] [review]
Including the test file too
Comment 9 Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-03 14:39:59 PDT
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?
Comment 10 Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-03 14:43:06 PDT
Ahahaha, we had the same idea!  :-D  That's what I get for not reading bug commentas.
Comment 11 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-11-03 14:44:04 PDT
> Ahahaha, we had the same idea!

Yep.  I'll make that change.  ;)
Comment 12 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-11-04 12:39:20 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/73baf8c58b95
Comment 13 Marco Bonardo [::mak] 2011-11-05 02:47:03 PDT
https://hg.mozilla.org/mozilla-central/rev/73baf8c58b95

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