1371274 - Make it possible to use NewIdleRunnableMethod[WithTimer] on classes without SetDeadline

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Comment 1

[Andreas Farre [:farre]]

Attachment: 0001-Bug-1371274-Don-t-call-SetDeadline-on-receivers-with.patch

Comment 2

[Andreas Farre [:farre]]

Attachment: 0002-Bug-1371274-Add-tests-for-NewIdleRunnable-for-classe.patch

Comment 3

[Andreas Farre [:farre]]

Comment on attachment 8876066 [details] [diff] [review]
0001-Bug-1371274-Don-t-call-SetDeadline-on-receivers-with.patch

This patch makes it possible to create idle runnables using NewIdleRunnable without the receiving object having SetDeadline. 

The implementation uses a feature suggested for C++17[1,2] that allows for checking if a class has a method at compile time. Then it is possible using enable_if to simply not call SetDeadline if there is no SetDeadline.

The feature is entirely possible to implement using only C++11, and I've included it in a stripped down version that gives only the bare minimum. Another approach would be to add a mozilla/TypeTraitsExperimental.h alongside mozilla/TypeTraits.h and implement the complete feature as suggested[1]. There was a snag with the implementation of VoidT (which is a mechanism to join all well-formed types in an SFINAE context to one type) that needed some special casing for non-clang compilers[3].

This patch also allows us to revert to how nsRunnableMethodReceiver looked like, before Bug 1358476, which is less cluttered.

[1] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/n4502.pdf
[2] http://en.cppreference.com/w/cpp/experimental/is_detected
[3] https://stackoverflow.com/a/30848101

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8876066 [details] [diff] [review]
0001-Bug-1371274-Don-t-call-SetDeadline-on-receivers-with.patch

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

You may find the HasRefCountMethods check in nsThreadUtils.h uses a technique that's pretty much what you want here, with a little less template gnarliness.

Comment 5

[Andreas Farre [:farre]]

Cool, I'll use those instead then.

Comment 6

[Andreas Farre [:farre]]

Attachment: 0001-Bug-1371274-Don-t-call-SetDeadline-on-receivers-with.patch

Use the same method detection as with HasRefCountMethods.

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8876671 [details] [diff] [review]
0001-Bug-1371274-Don-t-call-SetDeadline-on-receivers-with.patch

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

(In reply to Andreas Farre [:farre] from comment #3)
> This patch makes it possible to create idle runnables using NewIdleRunnable
> without the receiving object having SetDeadline.

I don't think I have any objection to the code per se, but I am completely missing any rationale for why we think that doing this is a good thing.  Can you enlighten me here?

::: xpcom/threads/nsThreadUtils.h
@@ +1091,5 @@
> +}
> +
> +template <class T>
> +typename mozilla::EnableIf<!::detail::HasSetDeadline<T>::value>::Type
> +SetDeadlineImpl(T* aObj, mozilla::TimeStamp aTimeStamp)

This should probably go in mozilla::detail if at all possible.

Comment 8

[Andreas Farre [:farre]]

(In reply to Nathan Froyd [:froydnj] from comment #7)
> Comment on attachment 8876671 [details] [diff] [review]
> 0001-Bug-1371274-Don-t-call-SetDeadline-on-receivers-with.patch
> 
> Review of attachment 8876671 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Andreas Farre [:farre] from comment #3)
> > This patch makes it possible to create idle runnables using NewIdleRunnable
> > without the receiving object having SetDeadline.
> 
> I don't think I have any objection to the code per se, but I am completely
> missing any rationale for why we think that doing this is a good thing.  Can
> you enlighten me here?

Right, the motivation comes from Bug 1361709, from https://bugzilla.mozilla.org/show_bug.cgi?id=1361709#c8 and onwards. We want to be able to dispatch an named idle runnable with a timeout, without needing to implement SetDeadline. This is already possible by calling NS_IdleDispatchToCurrentThread on an ordinary runnable, but that means that we need to wrap the runnable with IdleRunnableWrapper. With this patch we can have idle runnables that ignore deadlines, with a timeout but without the need for a wrapper.

Basically we consider it to be a fail if all runnables created with NewIdleRunnableWithTimer, that doesn't plan to actually care about the deadline, needs to implement an empty SetDeadline.

And the more general reason that I struggle onwards with NewIdleRunnable* is that I believe in that that API is more convenient for the low hanging cases of idle callbacks instead of actually inheriting from mozilla::IdleRunnable :)

> ::: xpcom/threads/nsThreadUtils.h
> @@ +1091,5 @@
> > +}
> > +
> > +template <class T>
> > +typename mozilla::EnableIf<!::detail::HasSetDeadline<T>::value>::Type
> > +SetDeadlineImpl(T* aObj, mozilla::TimeStamp aTimeStamp)
> 
> This should probably go in mozilla::detail if at all possible.

Comment 9

[Nathan Froyd [:froydnj]]

Thanks for the explanation.  Eliminating the need for wrappers sounds like a Good Thing.

Comment 10

[Andreas Farre [:farre]]

Attachment: 0001-Bug-1371274-Don-t-call-SetDeadline-on-receivers-with.patch

Moved SetDeadlineImpl to details.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9cc1bc609b2de8dcb072210aa699a56824b11ef

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8878521 [details] [diff] [review]
0001-Bug-1371274-Don-t-call-SetDeadline-on-receivers-with.patch

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

Thanks!

Comment 12

[Andreas Farre [:farre]]

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9cc1bc609b2de8dcb072210aa699a56824b11ef

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/88285532a879
Don't call SetDeadline on receivers without one. r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f7345574bce
Add tests for NewIdleRunnable for classes without SetDeadline. r=froydnj

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/88285532a879
https://hg.mozilla.org/mozilla-central/rev/0f7345574bce

Comment 15

[Michael Leu[:Lenzak](UTC+8)]

 c:\git\gecko\obj-i686-pc-mingw32\dist\include\nsThreadUtils.h(1043): error C2039: 'SetDeadline': is not a member of 'IdleObjectWithoutSetDeadline'

 c:\git\gecko\obj-i686-pc-mingw32\dist\include\nsThreadUtils.h(1054): error C2955: 'detail::SFINAE1True': use of class template requires template argument list

 c:\git\gecko\obj-i686-pc-mingw32\dist\include\nsThreadUtils.h(1159): error C2672: 'detail::SetDeadlineImpl': no matching overloaded function found

 c:\git\gecko\obj-i686-pc-mingw32\dist\include\nsThreadUtils.h(1159): error C2893: Failed to specialize function template 'mozilla::EnableIf<detail::HasSetDeadline<T>::value,void>::Type detail::SetDeadlineImpl(T *,mozilla::TimeStamp)'

I encounter build errors after this patch landed.

Comment 16

[Andreas Farre [:farre]]

I added a patch that I hope fixes this, but this compile error isn't on treeherder so I have a hard time reproducing it. Could you please try the patch :Lenzak?

Comment 17

[Michael Leu[:Lenzak](UTC+8)]

Yes, this fixed the build fail, thank you :)

Comment 18

[Andreas Farre [:farre]]

(In reply to Michael Leu[:Lenzak](UTC+8) from comment #17)
> Yes, this fixed the build fail, thank you :)

Thank you for checking!