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

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Randall Barker [:rbarker]]

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.

Comment 1

[Botond Ballo [:botond]]

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.

Comment 2

[Nathan Froyd [:froydnj]]

I think you want NewNonOwningRunnableMethod?  You have to ensure that the object lives longer than the runnable does, of course.

Comment 3

[Randall Barker [: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.

Comment 4

[Randall Barker [: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.

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

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...

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1329319 - gtest for issue with NewRunnableMethod on a non-refcounted base class method -

This test alone will fail without the next patch, demonstrating the issue at
hand, where the method-pointer from the base class forces NewRunnableMethod to
store a pointer to that base class in a RefPtr, which is not possible when that
base class is not ref-counted.

Review commit: https://reviewboard.mozilla.org/r/103110/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/103110/

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: Bug 1329319 - Allow NewRunnableMethod to method of a non-refcounted base class -

This is done by storing the object pointer based on the exact pointee type,
instead of as hinted by the method-pointer, which could be a non-refcounted
base class.

The stored pointer type is statically-checked to be derived from (or the same
as) the class type from the method-pointer, to prevent misuses.

One change had to be done in TrackBuffersManager, as it was passing another
type and relying on implicit pointer conversions. A simple `.get()` to pass
the raw pointer type (to be stored in a RefPtr) fixed that one issue.

Review commit: https://reviewboard.mozilla.org/r/103112/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/103112/

Comment 8

[Nathan Froyd [:froydnj]]

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?

Comment 9

[Nathan Froyd [:froydnj]]

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.

Comment 10

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8824834 [details]
Bug 1329319 - Allow NewRunnableMethod to method of a non-refcounted base class -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/103112/diff/1-2/

Comment 11

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8824833 [details]
Bug 1329319 - gtest for issue with NewRunnableMethod on a non-refcounted base class method -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/103110/diff/1-2/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
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

Comment 13

[Botond Ballo [:botond]]

Thanks for fixing this, Gerald!

Comment 14

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

You're welcome.
Though I messed up, will have to back out! But I'll re-land soon...

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/autoland/rev/f4787fd439103cf87ed3769e348a5576b2b3b9cb

Comment 16

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8824834 [details]
Bug 1329319 - Allow NewRunnableMethod to method of a non-refcounted base class -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/103112/diff/2-3/

Comment 17

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Comment on attachment 8824833 [details]
Bug 1329319 - gtest for issue with NewRunnableMethod on a non-refcounted base class method -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/103110/diff/2-3/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
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

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/71ff95e7883b
https://hg.mozilla.org/mozilla-central/rev/cdfeb6186fb2