This line of code is wrong: """ NS_IMETHOD_(bool) MarkForCC(void) { return !_to ? NS_ERROR_NULL_POINTER : _to->MarkForCC(); } """ http://dxr.lanedo.com/mozilla-central/--GENERATED--/content/base/public/_xpidlgen/nsIFrameMessageManager.h.html#l146 It should return a bool, but returns NS_ERROR_NULL_POINTER, which winds up meaning "true". This macro is actually used in TabChild.h. The culprit is here: """ emitTemplate("\\\n %(asNative)s { return !_to ? NS_ERROR_NULL_POINTER : _to->%(nativeName)s(%(paramList)s); } ") """ http://mxr.mozilla.org/mozilla-central/source/xpcom/idl-parser/header.py#409 This either needs to skip emitting this macro if there are any non-nsresult return types (and users like TabChild.h need to be fixed to do something else), or somehow adapted to account for other return types. This blocks progress on bug 779473, because I don't have a workaround handy and I can't compile until I get one. I'm willing to write a patch, just tell me what I need to do.

Just throwing something that might work (and possibly find other similar problems if there are non bool-returning notxpcom functions)

Kyle might have opinions.

without the unrelated cruft

(In reply to Mike Hommey [:glandium] from comment #3) > Created attachment 648642 [details] [diff] [review] > PoC > > without the unrelated cruft I get errors when trying to build with this: Traceback (most recent call last): File "/mnt/extra/checkouts/central/config/pythonpath.py", line 56, in <module> main(sys.argv[1:]) File "/mnt/extra/checkouts/central/config/pythonpath.py", line 48, in main execfile(script, frozenglobals) File "/mnt/extra/checkouts/central/objdir-ff-release/dist/sdk/bin/header.py", line 495, in <module> print_header(idl, outfd, file) File "/mnt/extra/checkouts/central/objdir-ff-release/dist/sdk/bin/header.py", line 186, in print_header write_interface(p, fd) File "/mnt/extra/checkouts/central/objdir-ff-release/dist/sdk/bin/header.py", line 419, in write_interface emitTemplate("\\\n %(asNative)s { return !_to ? %(nullError) : _to->%(nativeName)s(%(paramList)s); } ") File "/mnt/extra/checkouts/central/objdir-ff-release/dist/sdk/bin/header.py", line 399, in emitTemplate 'paramList': attributeParamNames(member)}) ValueError: unsupported format character ':' (0x3a) at index 46 make[6]: *** [_xpidlgen/nsIException.h] Error 1

> I get errors when trying to build with this: Ah, missing a "s" in the format string.

Ms2ger tells me it does nothing anyway. Alternatively, the actual patch for this bug could skip attributes and only do methods, but if it does nothing we may as well remove it from the .idl files.

So this fixes a few issues with your patch: * ConstMembers don't have any .xpcom attribute, so it was throwing when trying to access it. I added a hasattr() check. * nsXTFElementWrapper.h tries to safely forward nsIXPCScriptable, which has "[notxpcom,nostdcall] PRUint32 getScriptableFlags();". I made it return 0 in this case. * There are lots of .idl's that fail our checks here, but their safe-forwarding macros aren't actually used. These were throwing with your patch. I changed it to just skip the safe forwarding macro if there's a bad type, instead of throwing. Obviously, hardcoding PRUint32 here is not top form, but I'm not sure how to easily include all numeric types. Also, probably this isn't as Pythonic as it could be, since my Python experience is limited. Any suggestions for improvement?

Comment on attachment 649591 [details] [diff] [review] New PoC patch, actually compiles Review of attachment 649591 [details] [diff] [review]: ----------------------------------------------------------------- lgtm

Comment on attachment 649591 [details] [diff] [review] New PoC patch, actually compiles Review of attachment 649591 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/idl-parser/header.py @@ +386,5 @@ > + defineSafeForwarding = not any(hasattr(member, 'notxpcom') and > + member.notxpcom and > + member.realtype.name != 'PRUint32' and > + member.realtype.name != 'boolean' > + for member in iface.members) This seems like a repetition that could easily end up out of sync with the test in emitTemplate. You could either have emitTemplate return a boolean value, or catch the exception instead.

Comment on attachment 649589 [details] [diff] [review] Patch part 1 -- Remove [xpcom] from some attributes Review of attachment 649589 [details] [diff] [review]: ----------------------------------------------------------------- Maybe we could fail on notxpcom attributes, then, to prevent further surprises.

I verified this doesn't change the actual content of nsITimedChannel.h (except comments). The attribute seems to work the same as [noscript], but I don't understand this code much at all, so let me know if this is wrong.

Based on glandium's patch. (Should I credit him somehow? hg only allows one author.) The regex for numeric types is probably wrong, unless realtype.name is translated to the appropriate PR* for common things like "long", but it works for now, because the only instance where it comes up in the tree is PRUint32. If I should test for numeric types differently, let me know how. The thing I don't like about this approach is that it makes any forwarding failure in notxpcom methods silent. This is likely not what the user of the macro intended! A possible alternative approach would be to make notxpcom methods not forwarded at all -- they'd be declared in the safe-forwarding macro like normal methods, so the interface's implementer would have to define forwarding behavior manually. Requiring the implementer to be explicit probably makes more sense, once I think about it. It's not obvious to me what the right thing to do here is in general, so making implementers define it explicitly makes me feel better than silently doing something that might be surprising. What do you think? (It only affects a small number of interfaces, I think.) Try: https://tbpl.mozilla.org/?tree=Try&rev=64a9a5242bb5

I think we should not implement forwarding for notxpcom methods.

Per comment 13. I'm asking for your review only on the xpcom/ part -- if this is the approach we go with, I'll get module peer review for the manually-written forwarding methods. One bad thing about this approach is that if you don't override the methods manually, all you get is inscrutable linker errors. But this seems to be very uncommon anyway, given that there are four cases in the whole tree where it's hit, so I wouldn't worry too much about user-friendliness. Try: https://tbpl.mozilla.org/?tree=Try&rev=ce27b1dd2965

Oops -- getScriptableFlags() is nostdcall. Same as last patch, but using PRUint32 instead of NS_IMETHODIMP_(PRUint32) to fix build errors on Windows. New try: https://tbpl.mozilla.org/?tree=Try&rev=88ed2d9c7e53

Comment on attachment 650031 [details] [diff] [review] Patch part 1 -- Make [notxpcom] attributes an error Review of attachment 650031 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/qsgen.py @@ +192,3 @@ > raise UserError( > "%s %s: notxpcom methods are not supported." > % (member.kind.capitalize(), memberId)) Please add an assertion for member.kind == 'attribute' and member.notxpcom. ::: xpcom/idl-parser/xpidl.py @@ +789,5 @@ > def toIDL(self): > attribs = attlistToIDL(self.attlist) > readonly = self.readonly and 'readonly ' or '' > return "%s%sattribute %s %s;" % (attribs, readonly, self.type, self.name) > Remove this extra whitespace while you're here.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #16) > Please add an assertion for member.kind == 'attribute' and member.notxpcom. AttributeError: 'Attribute' object has no attribute 'notxpcom'

Any idea when you can get around to reviewing this? It's been more than two weeks. Or is there someone else I can ask?

Comment on attachment 650466 [details] [diff] [review] Alternate patch part 2, v2 -- Don't safe-forward [notxpcom] methods Review of attachment 650466 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/xtf/src/nsXTFElementWrapper.cpp @@ +860,5 @@ > +PRUint32 > +nsXTFElementWrapper::GetScriptableFlags() > +{ > + return GetBaseXPCClassInfo() ? GetBaseXPCClassInfo()->GetScriptableFlags() > + : ~0; Do we really want '~0' here? I find that quite hard to believe. ::: content/xtf/src/nsXTFElementWrapper.h @@ +200,5 @@ > +// the IDL binding doesn't know what value to return. > +inline PRUint32 > +nsXTFClassInfo::GetScriptableFlags() > +{ > + return mWrapper ? mWrapper->GetScriptableFlags() : ~0; And here? ::: xpcom/idl-parser/header.py @@ +402,4 @@ > elif isinstance(member, xpidl.Method): > fd.write(tmpl % {'asNative': methodAsNative(member), > 'nativeName': methodNativeName(member), > 'paramList': paramlistNames(member)}) I would prefer this written as: elif isMethod: if notxpcom: do thing 1 else: do thing 2

Comment on attachment 650032 [details] [diff] [review] Patch part 2 -- Make returned value match return type for safe forwarding I think we decided that approach 2 is better.

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #19) > ::: content/xtf/src/nsXTFElementWrapper.cpp > @@ +860,5 @@ > > +PRUint32 > > +nsXTFElementWrapper::GetScriptableFlags() > > +{ > > + return GetBaseXPCClassInfo() ? GetBaseXPCClassInfo()->GetScriptableFlags() > > + : ~0; > > Do we really want '~0' here? I find that quite hard to believe. > > ::: content/xtf/src/nsXTFElementWrapper.h > @@ +200,5 @@ > > +// the IDL binding doesn't know what value to return. > > +inline PRUint32 > > +nsXTFClassInfo::GetScriptableFlags() > > +{ > > + return mWrapper ? mWrapper->GetScriptableFlags() : ~0; > > And here? See comment #14: > I'm asking for your review only on the xpcom/ part -- if > this is the approach we go with, I'll get module peer review for the > manually-written forwarding methods. Since we've agreed that this is the approach to go with, I'll request review on those parts now. > ::: xpcom/idl-parser/header.py > @@ +402,4 @@ > > elif isinstance(member, xpidl.Method): > > fd.write(tmpl % {'asNative': methodAsNative(member), > > 'nativeName': methodNativeName(member), > > 'paramList': paramlistNames(member)}) > > I would prefer this written as: > > elif isMethod: > if notxpcom: > do thing 1 > else: > do thing 2 Okay, will do. Also, please tell me what you want me to do about comment 17. If I implement your review feedback for the first patch, it doesn't build. Should I test for "member.kind == 'attribute' and 'notxpcom' in member" instead, or do you want me to restore the 'notxpcom' property on attribute objects and just make it always false? Anyway, thanks for finding the time to review this!

Comment on attachment 650466 [details] [diff] [review] Alternate patch part 2, v2 -- Don't safe-forward [notxpcom] methods For the non-xpcom/ parts (content/ and dom/). The methods I'm changing currently return NS_ERROR_FAILURE silently cast to PRUint32 or bool if the forwarding fails, because the binding returns NS_ERROR_FAILURE even for notxpcom methods and nothing notices because nsresult isn't yet an enum class. I picked some arbitrary values to return instead -- tell me if you want me to use different return values.

Comment on attachment 650466 [details] [diff] [review] Alternate patch part 2, v2 -- Don't safe-forward [notxpcom] methods Please use "0" for the scriptable flags default. r=me with that.

Comment on attachment 650466 [details] [diff] [review] Alternate patch part 2, v2 -- Don't safe-forward [notxpcom] methods If you're fixing the ~0 thing as bz said in comment 23, r=me

https://hg.mozilla.org/integration/mozilla-inbound/rev/385bc6d03597 https://hg.mozilla.org/integration/mozilla-inbound/rev/0cbe950173a8 (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #16) > ::: js/xpconnect/src/qsgen.py > @@ +192,3 @@ > > raise UserError( > > "%s %s: notxpcom methods are not supported." > > % (member.kind.capitalize(), memberId)) > > Please add an assertion for member.kind == 'attribute' and member.notxpcom. I wound up not making this change because it caused errors, as noted in comment 17 and mentioned again in comment 21, and you didn't say how you'd like them fixed. If you'd like me to file a follow-up and fix it there in a way that doesn't cause errors, please say so and I will.

"It doesn't work" was sufficient to address that comment.

https://hg.mozilla.org/mozilla-central/rev/385bc6d03597 https://hg.mozilla.org/mozilla-central/rev/0cbe950173a8