779809 - NS_FORWARD_SAFE_* produces incorrect macros for non-nsresult return types

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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.

Comment 1

[Mike Hommey [:glandium]]

Attachment: PoC

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

Comment 2

[Josh Matthews [:jdm]]

Kyle might have opinions.

Comment 3

[Mike Hommey [:glandium]]

Attachment: PoC

without the unrelated cruft

Comment 4

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(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

Comment 5

[Mike Hommey [:glandium]]

Attachment: PoC

> I get errors when trying to build with this:

Ah, missing a "s" in the format string.

Comment 6

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 1 -- Remove [xpcom] from some attributes

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.

Comment 7

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: New PoC patch, actually compiles

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 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 649591 [details] [diff] [review]
New PoC patch, actually compiles

Review of attachment 649591 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm

Comment 9

[Mike Hommey [:glandium]]

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 10

[Mike Hommey [:glandium]]

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.

Comment 11

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 1 -- Make [notxpcom] attributes an error

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.

Comment 12

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch part 2 -- Make returned value match return type for safe forwarding

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

Comment 13

[Benjamin Smedberg]

I think we should not implement forwarding for notxpcom methods.

Comment 14

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Alternate patch part 2 -- Don't safe-forward [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

Comment 15

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Alternate patch part 2, v2 -- Don't safe-forward [notxpcom] methods

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 16

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 17

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #16)
> Please add an assertion for member.kind == 'attribute' and member.notxpcom.

  AttributeError: 'Attribute' object has no attribute 'notxpcom'

Comment 18

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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 19

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 21

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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 22

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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 23

[Boris Zbarsky [:bzbarsky]]

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 24

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 25

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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

(In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 26

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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