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

RESOLVED FIXED in Firefox 56

Status

()

Core
DOM
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: farre, Assigned: farre)

Tracking

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Updated

7 months ago
Assignee: nobody → afarre
(Assignee)

Comment 1

7 months ago
Created attachment 8876066 [details] [diff] [review]
0001-Bug-1371274-Don-t-call-SetDeadline-on-receivers-with.patch
(Assignee)

Comment 2

7 months ago
Created attachment 8876067 [details] [diff] [review]
0002-Bug-1371274-Add-tests-for-NewIdleRunnable-for-classe.patch
(Assignee)

Comment 3

7 months ago
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
Attachment #8876066 - Flags: feedback?(nfroyd)
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.
Attachment #8876066 - Flags: feedback?(nfroyd) → feedback+
(Assignee)

Comment 5

7 months ago
Cool, I'll use those instead then.
(Assignee)

Comment 6

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

Use the same method detection as with HasRefCountMethods.
Attachment #8876066 - Attachment is obsolete: true
Attachment #8876671 - Flags: review?(nfroyd)
(Assignee)

Updated

7 months ago
Attachment #8876067 - Flags: review?(nfroyd)

Updated

7 months ago
Attachment #8876067 - Flags: review?(nfroyd) → review+
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.
Attachment #8876671 - Flags: review?(nfroyd)
(Assignee)

Comment 8

7 months ago
(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.
Flags: needinfo?(nfroyd)
Thanks for the explanation.  Eliminating the need for wrappers sounds like a Good Thing.
Flags: needinfo?(nfroyd)
(Assignee)

Comment 10

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

Moved SetDeadlineImpl to details.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9cc1bc609b2de8dcb072210aa699a56824b11ef
Attachment #8876671 - Attachment is obsolete: true
Attachment #8878521 - Flags: review?(nfroyd)
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!
Attachment #8878521 - Flags: review?(nfroyd) → review+
(Assignee)

Updated

7 months ago
Keywords: checkin-needed

Comment 13

7 months ago
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed

Comment 14

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/88285532a879
https://hg.mozilla.org/mozilla-central/rev/0f7345574bce
Status: NEW → RESOLVED
Last Resolved: 7 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
 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.
Flags: needinfo?(afarre)
Depends on: 1374514
(Assignee)

Comment 16

7 months ago
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?
Flags: needinfo?(afarre) → needinfo?(cleu)
Yes, this fixed the build fail, thank you :)
Flags: needinfo?(cleu)
(Assignee)

Comment 18

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

Thank you for checking!
You need to log in before you can comment on or make changes to this bug.