558498 - nsRunnableMethod should support __stdcall methods

Status: RESOLVED FIXED

(Core :: General, enhancement)

Description

[Ben Newman (:bnewman) (:benjamn)]

Currently nsRunnableMethods accept only methods of the type

  ReturnType (ClassType::*)();

which is a distinct type from

  ReturnType (__stdcall ClassType::*)();

on platforms that support __stdcall (i.e., to my knowledge, WIN32 and OS2).  This means, unfortunately, that nsRunnableMethods can't be created from NS_IMETHODs, which are declared with __stdcall on WIN32:

  http://hg.mozilla.org/mozilla-central/file/91694d19d7b2/xpcom/base/nscore.h#l184

Perhaps you've encountered the same deficiency.  The following code *seems* to work on non-WIN32 platforms

  NS_DispatchToMainThread(NS_NEW_RUNNABLE_METHOD(nsFoo, foo, Release));

but then your patch blows up on the tryserver (or worse, on mozilla-central after a careless landing).

Assigning to myself because I have a plan (and preliminary patch), but I welcome your input/feedback.

Comment 1

[Ben Newman (:bnewman) (:benjamn)]

Attachment: Part 1/3: Eliminate duplicate code by combining nsNonOwningRunnableMethod with nsRunnableMethod.

The signature of nsRunnableMethod is now

  template <class ClassType,
            typename ReturnType = void,
            Owning = true>
  class nsRunnableMethod;

What used to be called nsNonOwningRunnableMethod<C, R> is now simply nsRunnableMethod<C, R, false>.

Comment 2

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 438397 [details] [diff] [review]
Part 1/3: Eliminate duplicate code by combining nsNonOwningRunnableMethod with nsRunnableMethod.

>+template <class ClassType, bool Owning>
>+struct RunnableMethodReceiver {
>+  ClassType *mObj;
>+  RunnableMethodReceiver(ClassType *obj) : mObj(obj) { NS_IF_ADDREF(mObj); }
>+ ~RunnableMethodReceiver() { Revoke(); }
>+  void Revoke() { NS_IF_RELEASE(mObj); }
>+};
>+
>+template <class ClassType>
>+struct RunnableMethodReceiver<ClassType, false> {
>+  ClassType *mObj;
>+  RunnableMethodReceiver(ClassType *obj) : mObj(obj) {}
>+  void Revoke() { mObj = nsnull; }
>+};

This class abstracts the (non-)ownership implementation.  I wish I could have done this without declaring a new class in global scope, but I didn't see a cleaner way.

Comment 3

[Ben Newman (:bnewman) (:benjamn)]

Attachment: Part 2/3: Introduce nsRunnableMethodImpl<Method, Owning>.

I wanted to keep the template signature of nsRunnableMethod, since that class is used elsewhere, but I wanted to implement an implementation that could accept any kind of pointer-to-method type, so I created the nsRunnableMethodImpl subclass.

Comment 4

[Ben Newman (:bnewman) (:benjamn)]

Attachment: Part 3/3: Prefer NS_New{NonOwning,}RunnableMethod to direct instantiation of nsRunnableMethods.

As you'll see, using a template function that infers the types of its arguments saves a fair bit of redundancy elsewhere.

Comment 5

[Ben Newman (:bnewman) (:benjamn)]

(In reply to comment #3)
> I wanted to implement an implementation

Well, if that's not the awkwardest sentence I wrote today, I don't wanna know what was.

Comment 6

[[e-mail]]

Comment on attachment 438397 [details] [diff] [review]
Part 1/3: Eliminate duplicate code by combining nsNonOwningRunnableMethod with nsRunnableMethod.

r=dwitte

Comment 7

[[e-mail]]

Comment on attachment 438399 [details] [diff] [review]
Part 2/3: Introduce nsRunnableMethodImpl<Method, Owning>.

r=dwitte

Comment 8

[[e-mail]]

Comment on attachment 438400 [details] [diff] [review]
Part 3/3: Prefer NS_New{NonOwning,}RunnableMethod to direct instantiation of nsRunnableMethods.

r=dwitte

Comment 9

[[e-mail]]

Comment on attachment 438399 [details] [diff] [review]
Part 2/3: Introduce nsRunnableMethodImpl<Method, Owning>.

>+template<typename PtrType, typename Method>
>+typename nsRunnableMethodTraits<Method, true>::base_type*
>+NS_NewRunnableMethod(PtrType ptr, Method method)
>+{
>+  return new nsRunnableMethodImpl<Method, true>(ptr, method);
>+}

Hmm, so, the NS_ prefix has a meaning: that the symbol is exported, a la 'extern NS_COM nsresult NS_...'. (Most, if not all, NS_ functions in our codebase return nsresults and use outparams, but that's probably not required.)

For people #including this header and using the previous macro, it would've worked fine from an external library, right? Providing the non-virtual guts of nsRunnableMethod were inlined. If they weren't, it would break. So I guess that means there's no expectation now, either. Agree?

Should probably just slap an NS_COM on this and be done with it.

Comment 10

[Ben Newman (:bnewman) (:benjamn)]

Comment on attachment 438397 [details] [diff] [review]
Part 1/3: Eliminate duplicate code by combining nsNonOwningRunnableMethod with nsRunnableMethod.

I'd appreciate your take on these changes, dbaron.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 438397 [details] [diff] [review]
Part 1/3: Eliminate duplicate code by combining nsNonOwningRunnableMethod with nsRunnableMethod.

sr=dbaron

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 438400 [details] [diff] [review]
Part 3/3: Prefer NS_New{NonOwning,}RunnableMethod to direct instantiation of nsRunnableMethods.

sr=dbaron, although I don't think this patch needs sr

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 438399 [details] [diff] [review]
Part 2/3: Introduce nsRunnableMethodImpl<Method, Owning>.

I can't say I understand why this needs to be the way it is, but it looks ok, so sr=dbaron.

Comment 14

[Ben Newman (:bnewman) (:benjamn)]

Thanks for having a look(In reply to comment #12)
> (From update of attachment 438400 [details] [diff] [review])
> sr=dbaron, although I don't think this patch needs sr

On second thought, yeah, that was the most obvious/uninteresting patch.  Thanks for taking a look, anyway :)

(In reply to comment #13)
> (From update of attachment 438399 [details] [diff] [review])
> I can't say I understand why this needs to be the way it is, but it looks ok,
> so sr=dbaron.

That's a reasonable criticism of any design where a lot of equivalent approaches were possible.  I had to reshuffle things repeatedly to minimize repetition and preserve readability, and the most I can say about the result (for your curiosity, as well as for posterity) is that it was the best equilibrium I could find while satisfying the following constraints:

1. Users of nsRunnableMethod types should not have to think about the calling convention.

This meant avoiding pointer-to-method syntax in the template signature, sticking instead to the familiar <ClassType[, ReturnType[, Owning]]> template parameters.  ClassType and ReturnType do not fully specify the type of a zero-parameter method (now that we care about the calling convention, too), so nsRunnableMethod cannot itself store the all-important method pointer.  Hence the nsRunnableMethodImpl subclass, whose Method template parameter would be difficult to write by hand, but fortunately never needs to be (it gets type-inferred by NS_NewRunnableMethod).

2. nsRunnableMethodImpl should accept any Method type that is callable in the manner that Run() requires.

This is the crux __stcall support: nsRunnableMethodImpl simply doesn't care about the calling convention.  The drawback is that the Method type becomes opaque, and nsRunnableMethodTraits is required to determine the class type and return type of the method.

3. I didn't want to re-type the monstrosity

  nsRunnableMethod<typename nsRunnableMethodTraits<Method>::class_type,
                   typename nsRunnableMethodTraits<Method>::return_type,
                   Owning>

for the base type of nsRunnableMethodImpl and the return types of the two NS_New*RunnableMethod functions, so that's why I added the base_type typedef to nsRunnableMethodTraits.

Most of the other design decisions are inherited from patch 1/3, which I hope seemed uncontroversial by itself.

Thanks again for the reviews!

Comment 15

[Ben Newman (:bnewman) (:benjamn)]

I meant "the crux of the __stdcall support" up there, in case that wasn't clear.

Comment 16

[Ben Newman (:bnewman) (:benjamn)]

http://hg.mozilla.org/mozilla-central/rev/d6c5f8853dce
http://hg.mozilla.org/mozilla-central/rev/a3733b568431
http://hg.mozilla.org/mozilla-central/rev/fa53318b87aa

Comment 17

[Mark Banner (:standard8)]

After discussing with Benjamin over irc, I've just committed a bustage fix for this bug:

http://hg.mozilla.org/mozilla-central/rev/dd9b5b71e96c

NS_COM_GLUE shouldn't be defined on function definitions/inline functions. This should fix non-libxul builds on Windows.

Comment 18

[Ben Newman (:bnewman) (:benjamn)]

(In reply to comment #17)
> After discussing with Benjamin over irc, I've just committed a bustage fix for
> this bug:
> 
> http://hg.mozilla.org/mozilla-central/rev/dd9b5b71e96c
> 
> NS_COM_GLUE shouldn't be defined on function definitions/inline functions. This
> should fix non-libxul builds on Windows.

I have no objection to this patch, but I'm not quite sure what bustage you mean.  Can you post a link to a build log?

Comment 19

[Robert Kaiser]

(In reply to comment #18)
> I have no objection to this patch, but I'm not quite sure what bustage you
> mean.  Can you post a link to a build log?

Contrary to some people's assumptions, there are still other applications around than just Firefox. ;-)

Regarding the logs, see those as examples:
http://tinderbox.mozilla.org/showlog.cgi?log=Thunderbird/1272374969.1272375244.4992.gz
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1272373023.1272373287.30215.gz

Comment 20

[Ben Newman (:bnewman) (:benjamn)]

(In reply to comment #19)
> Contrary to some people's assumptions, there are still other applications
> around than just Firefox. ;-)

It would be fantastic if those applications could be integrated into the tryserver.  Until that happens, after-the-fact bustage reports seem unavoidable.  I think this went about as smoothly as possible, mostly thanks to Mark Banner's efforts.  I'm sorry I didn't notice the problems sooner myself.

Comment 21

[Robert Kaiser]

(In reply to comment #20)
> It would be fantastic if those applications could be integrated into the
> tryserver.

We do have a MoMo tryserver that can build them, but the better solution for many such problems will be to get SeaMonkey and Thunderbird moved to being libxul-based. We are working on that, but it may still take a while (all mailnews code needs to use external linkage for that).

Comment 22

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

So, I'm a bit late to the party here, but is this actually used anywhere in our tree?  I ask because everything about this leads me to believe that this is really Win32 only (or, if you will, platform parity for calling NS_IMETHODs) but the configure check which this is gated on isn't run on Win32 .....