Closed
Bug 671453
Opened 13 years ago
Closed 13 years ago
IDL |[implicit_jscontext] attribute T foo| with doesn't work for the setter part
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla8
People
(Reporter: cjones, Assigned: bzbarsky)
References
Details
Attachments
(1 file)
1.25 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
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?
Reporter | ||
Comment 1•13 years ago
|
||
Um, in comment 0 where I had [implicit_jsval] I obviously meant [implicit_jscontext]. Apologies.
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.
Assignee | ||
Comment 3•13 years ago
|
||
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?
Summary: IDL |[implicit_jscontext] attribute T foo| with non-jsval T doesn't work → IDL |[implicit_jscontext] attribute T foo| with doesn't work for the setter part
Reporter | ||
Comment 4•13 years ago
|
||
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.
Assignee | ||
Comment 5•13 years ago
|
||
Yeah, quickstubs and xpconnect proper disagree on how to handle implicit_jscontext.
Assignee | ||
Comment 6•13 years ago
|
||
Oh, and to be clear, I was asking whether you were using a non-quickstubbed setter when you got crashes.
Reporter | ||
Comment 7•13 years ago
|
||
Pretty sure, yes (nsIXPC_Utils or thereabouts, forget the exact name).
Assignee | ||
Comment 8•13 years ago
|
||
Attachment #549408 -
Flags: review?(mrbkap)
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → bzbarsky
Whiteboard: [need review]
Comment 9•13 years ago
|
||
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
Attachment #549408 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 11•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/f7faf44371d1
Flags: in-testsuite?
Whiteboard: [need landing]
Target Milestone: --- → mozilla8
Comment 12•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/f7faf44371d1
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•