Last Comment Bug 671453 - IDL |[implicit_jscontext] attribute T foo| with doesn't work for the setter part
: IDL |[implicit_jscontext] attribute T foo| with doesn't work for the setter part
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla8
Assigned To: Boris Zbarsky [:bz] (Out June 25-July 6)
:
Mentors:
Depends on:
Blocks: 659350
  Show dependency treegraph
 
Reported: 2011-07-13 17:14 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2011-08-01 07:48 PDT (History)
6 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Make XPConnect dispatch match the header generators and quickstubs (1.25 KB, patch)
2011-07-29 10:14 PDT, Boris Zbarsky [:bz] (Out June 25-July 6)
mrbkap: review+
Details | Diff | Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-07-13 17:14:00 PDT
The problem is that

  [implicit_jsval] attribute jsval foo

generates

  Get/SetFoo(JSContext*, jsval);

and, e.g.

  [implicit_jsval] attribute boolean foo

generates

  Get/SetFoo(JSContext*, PRBool);

In the first case, with |attribute jsval|, xpconnect (or quickstubs?) passes the arguments to the C++ method in the right order.  At least, it seems to for IDB and canvas stuff that uses |attribute jsval|.

In the second case, with |attribute boolean|, xpconnect passes the arguments to the C++ method in the *wrong* order: (PRBool, JSContext*).  This obviously makes for crashy.  At least, it does with my WIP patch in bug 669949 that uses |attribute boolean| on nsXPCComponents_utils.

What is the argument order supposed to be?
Comment 1 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-07-13 17:17:18 PDT
Um, in comment 0 where I had [implicit_jsval] I obviously meant [implicit_jscontext].  Apologies.
Comment 2 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-07-14 09:33:51 PDT
I don't really know... Arguments always go first (attribute getters of course don't have arguments), and then the return values are always the last argument. Optional ("magic") stuff go in between. [optional_argc] goes after the last argument and before the return value, for instance. The intent was to put [implicit_jscontext] just before the place where [optional_argc] goes.
Comment 3 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-07-29 09:20:06 PDT
This is broken for jsval too, as far as I can tell.  I just tried using it, at least, and I get crashes and the arguments are passed in .  But it seems to be only broken for the _setter_.  For the getter things work right.

Chris, were you using this with a setter?
Comment 4 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-07-29 09:24:00 PDT
Yes, everything near http://mxr.mozilla.org/mozilla-central/source/content/canvas/src/nsCanvasRenderingContext2D.cpp#3135 .

BUT, I'm pretty sure 2dcontext is quickstubbed, which might have been a magic sauce allowing setters to work.
Comment 5 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-07-29 09:35:02 PDT
Yeah, quickstubs and xpconnect proper disagree on how to handle implicit_jscontext.
Comment 6 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-07-29 09:42:34 PDT
Oh, and to be clear, I was asking whether you were using a non-quickstubbed setter when you got crashes.
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-07-29 09:49:02 PDT
Pretty sure, yes (nsIXPC_Utils or thereabouts, forget the exact name).
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-07-29 10:14:17 PDT
Created attachment 549408 [details] [diff] [review]
Make XPConnect dispatch match the header generators and quickstubs
Comment 9 Blake Kaplan (:mrbkap) (please use needinfo!) 2011-07-29 10:58:16 PDT
Comment on attachment 549408 [details] [diff] [review]
Make XPConnect dispatch match the header generators and quickstubs

I think it'd be slightly cleaner to check IsSetter() || IsGetter() since that mirrors all of the other implicit_jscontext handling code nicely (they all have a method code-path and an attribute code-path) but I don't feel terribly strongly about it.

r=mrbkap
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-07-29 11:01:54 PDT
OK, I'll do that.
Comment 11 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-07-29 11:32:38 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/f7faf44371d1
Comment 12 Marco Bonardo [::mak] 2011-08-01 07:48:34 PDT
http://hg.mozilla.org/mozilla-central/rev/f7faf44371d1

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