Closed Bug 558498 Opened 10 years ago Closed 10 years ago

nsRunnableMethod should support __stdcall methods

Categories

(Core :: General, enhancement)

x86
All
enhancement
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: mozilla+ben, Assigned: mozilla+ben)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

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.
Summary: nsRunnableMethods should support __stdcall methods → nsRunnableMethod should support __stdcall methods
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 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.
Attachment #438397 - Attachment description: Part 1/3: Eliminate duplicate code by combining nsNonOwningRunnableMethod with nsRunnableMethod → Part 1/3: Eliminate duplicate code by combining nsNonOwningRunnableMethod with nsRunnableMethod.
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.
As you'll see, using a template function that infers the types of its arguments saves a fair bit of redundancy elsewhere.
(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.
Blocks: 558775
Attachment #438397 - Flags: review?
Attachment #438397 - Flags: review? → review?(dwitte)
Attachment #438399 - Flags: review?(dwitte)
Attachment #438400 - Flags: review?(dwitte)
Comment on attachment 438397 [details] [diff] [review]
Part 1/3: Eliminate duplicate code by combining nsNonOwningRunnableMethod with nsRunnableMethod.

r=dwitte
Attachment #438397 - Flags: review?(dwitte) → review+
Comment on attachment 438399 [details] [diff] [review]
Part 2/3: Introduce nsRunnableMethodImpl<Method, Owning>.

r=dwitte
Attachment #438399 - Flags: review?(dwitte) → review+
Comment on attachment 438400 [details] [diff] [review]
Part 3/3: Prefer NS_New{NonOwning,}RunnableMethod to direct instantiation of nsRunnableMethods.

r=dwitte
Attachment #438400 - Flags: review?(dwitte) → review+
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 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.
Attachment #438397 - Flags: superreview?(dbaron)
Attachment #438399 - Flags: superreview?(dbaron)
Attachment #438400 - Flags: superreview?(dbaron)
Comment on attachment 438397 [details] [diff] [review]
Part 1/3: Eliminate duplicate code by combining nsNonOwningRunnableMethod with nsRunnableMethod.

sr=dbaron
Attachment #438397 - Flags: superreview?(dbaron) → superreview+
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
Attachment #438400 - Flags: superreview?(dbaron) → superreview+
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.
Attachment #438399 - Flags: superreview?(dbaron) → superreview+
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!
I meant "the crux of the __stdcall support" up there, in case that wasn't clear.
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.
(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?
(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
(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.
(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).
Depends on: 563573
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 .....
Blocks: 803855
Depends on: 838803
Blocks: 838803
No longer depends on: 838803
You need to log in before you can comment on or make changes to this bug.