1119956 - Make nsCancelableRunnable an abstract class

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

nsCancelableRunnable right now has a do-nothing Run() and Cancel() method implemented. I don't know how that makes any sense:

1. The Run() method is almost always going to have to be overridden to do some state checking or something if it is meant to be cancelable. 

2. The Cancel() method is much more problematic. Each derived class is going to have to make some complicated decisions about how to safely cancel a runnable in a threadsafe manner. Will it use atomics flags? Locks? Are Run() and Cancel() only ever being called on a single thread?

Providing a default implementation lets derived class authors remain oblivious to the complexities involved here in my opinion...

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Or worse, derived class authors might assume that there are built-in smarts that magically handle Cancel()...

Comment 2

[Gabriele Svelto [:gsvelto]]

Just a bit of background here: the nsCancelableRunnable class was introduced as a horrible hack in bug 814771 because I didn't have anything better at the time. I've removed the sole use I made of it but unfortunately it was noticed and stuck in other parts of the codebase.

I later realized that a better approach might have been to introduce a method in nsEventQueue to cancel any runnable and make the nsCancelableRunnable class optional, something to implement only if the runnable had to do some specific cleanup when canceled (it might not be needed at all if destroying the runnable object is enough for cleaning up).

Comment 3

[Karl Tomlinson (:karlt)]

The cancelable concept has become conflated with a slightly different concept for workers. Workers may not actually run all queued runnables, and so runnables must be safe to retire. Cancelable runnables are always safe to retire, so cancelable runnables were permitted for workers. But this sufficient condition has been applied as necessary conditions. Multiple runnables have been marked as cancelable to satisfy these conditions without actually implementing cancellation. I'll post some patches to introduce a new class, sufficent for workers, but not claiming to implement cancellation.

See bug 1411520 for an example of non-cancelable CancelableRunnable for workers.

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: Bug 1119956 remove unused ErrorRunnable r?baku

Unused since
https://hg.mozilla.org/mozilla-central/rev/d7b3c828ed45ce0fc47ebdcc12bb55ce9344ebae#l1.53

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: Bug 1119956 add override and use NS_IMETHOD for in-class definition r?jonco

Depends on D98115

Comment 6

[Karl Tomlinson (:karlt)]

Attachment: Bug 1119956 introduce DiscardableRunnable for tasks that might not run but might not implement cancellation r?asuth,sg!

Classes that inherit from MaybeRunnable are only promising that it is OK for Run() to be skipped, rather than promising that Cancel() is effective.

Depends on D98116

Comment 7

[Karl Tomlinson (:karlt)]

Attachment: Bug 1119956 derive from DiscardableRunnable instead of CancelableRunnable when Cancel() is not supported r?asuth

Depends on D98117

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: Bug 1119956 don't pretend to implement nsICancelableRunnable::Cancel() r?gsvelto

Depends on D98118

Comment 9

[Karl Tomlinson (:karlt)]

Attachment: Bug 1119956 implement IdleRunnableWrapper::OnDiscard() r?

This causes no behavior changes in the current code because existing runnables
passed to NS_DispatchToThreadQueue() are either run on the main thread where
OnDoomed() is not called or they have only a no-op OnDoomed().

Depends on D98119

Comment 10

[Karl Tomlinson (:karlt)]

Attachment: Bug 1119956 add CancelableRunnable documentation to clarify expectations r?sg

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ad476dd681e8
remove unused ErrorRunnable r=baku
https://hg.mozilla.org/integration/autoland/rev/a74a33e820b1
add override and use NS_IMETHOD for in-class definition r=jonco

Comment 12

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/ad476dd681e8
https://hg.mozilla.org/mozilla-central/rev/a74a33e820b1

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ee9314cd003c
introduce DiscardableRunnable for tasks that might not run but might not implement cancellation r=asuth,sg
https://hg.mozilla.org/integration/autoland/rev/6359a76a8df7
add CancelableRunnable documentation to clarify expectations r=sg

Comment 14

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/ee9314cd003c
https://hg.mozilla.org/mozilla-central/rev/6359a76a8df7

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6c5750b3a0c7
derive from DiscardableRunnable instead of CancelableRunnable when Cancel() is not supported r=asuth,sg
https://hg.mozilla.org/integration/autoland/rev/f463fde9f5e2
don't pretend to implement nsICancelableRunnable::Cancel() r=gsvelto,necko-reviewers,dragana

Comment 16

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/6c5750b3a0c7
https://hg.mozilla.org/mozilla-central/rev/f463fde9f5e2

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d438ee8f7397
implement IdleRunnableWrapper::OnDiscard() r=sg

Comment 18

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/d438ee8f7397