Closed Bug 1329319 Opened 5 years ago Closed 5 years ago

Find a solution for using NewRunnableMethod where the member function is defined on a class that is not ref counted.

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: rbarker, Assigned: gerald)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

I ran into an issue while implementing Bug 1328752. In it I had the following issue:

// Not Ref Counted
class A {
public:
  virtual void Foo(const int64_t&) {}
};

class B : public A {
public:
  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(B);
  void Dispatch();
};


void
B::Dispatch() {
  int64_t id = 0;
  NS_DispatchToMainThread(NewRunnableMethod<const int64_t&>(this, &B::Foo), id);
}

I would get a compiler error because the "this" pointer in B::Dispatch would get downcasted to type A for template instantiation and A is not ref counted.
The issue is that the type of |&B::Foo| is |void(A::*)(const int64_t&)|, and NewRunnableMethod uses the type of the method  pointer (rather than the type of the class pointer) to determine the type of pointer that is stored, which is then required to be reference counted.

This can be worked around by explicitly casting |&B::Foo| at the call site to |void(B::*)(const int64_t&)|, but it would be nicer to change NewRunnableMethod to passing |&B::Foo| Just Works.
I think you want NewNonOwningRunnableMethod?  You have to ensure that the object lives longer than the runnable does, of course.
Flags: needinfo?(rbarker)
(In reply to Nathan Froyd [:froydnj] from comment #2)
> I think you want NewNonOwningRunnableMethod?  You have to ensure that the
> object lives longer than the runnable does, of course.

No, the class I'm passing in *is* ref counted. So it should work with NewRunnableMethod. To be specific, class A is an IPDL class and class B is the derived child and was ref counted.
Flags: needinfo?(rbarker)
I was able to work around this issue by creating a member function in class B (B::DoFoo) that wrapped A::Foo. Thus I was able to do:

NS_DispatchToMainThread(NewRunnableMethod<const int64_t&>(this, &B::DoFoo), id);

:botond thought there might be a way to update NewRunnableMethod so that the wrapper was not needed.
I think I've got a solution. POC:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1831ca76e9c0fb77b2babd25f3c17057bce3de5c
I've stumbled upon a couple of side things on the way, so I'll separate this monstrosity into different patches (maybe bugs) and submit them soon...
Assignee: nobody → gsquelart
Depends on: 1329513
Comment on attachment 8824833 [details]
Bug 1329319 - gtest for issue with NewRunnableMethod on a non-refcounted base class method -

https://reviewboard.mozilla.org/r/103110/#review103892

Presumably you want to push these patches in the opposite order: fix issue, then add gtest, so as to keep some semblance of bisectability?
Attachment #8824833 - Flags: review?(nfroyd) → review+
Comment on attachment 8824834 [details]
Bug 1329319 - Allow NewRunnableMethod to method of a non-refcounted base class -

https://reviewboard.mozilla.org/r/103112/#review103908

Thanks for fixing this; this groundwork should make some other changes, such as bug 1323482, much easier.
Attachment #8824834 - Flags: review?(nfroyd) → review+
Blocks: 1323482
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a75ecabf7129
Allow NewRunnableMethod to method of a non-refcounted base class - r=froydnj
https://hg.mozilla.org/integration/autoland/rev/ae96a44e6a8f
gtest for issue with NewRunnableMethod on a non-refcounted base class method - r=froydnj
Thanks for fixing this, Gerald!
You're welcome.
Though I messed up, will have to back out! But I'll re-land soon...
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/71ff95e7883b
Allow NewRunnableMethod to method of a non-refcounted base class - r=froydnj
https://hg.mozilla.org/integration/autoland/rev/cdfeb6186fb2
gtest for issue with NewRunnableMethod on a non-refcounted base class method - r=froydnj
https://hg.mozilla.org/mozilla-central/rev/71ff95e7883b
https://hg.mozilla.org/mozilla-central/rev/cdfeb6186fb2
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Blocks: 1328752
You need to log in before you can comment on or make changes to this bug.