Closed Bug 1119956 Opened 10 years ago Closed 4 years ago

Make nsCancelableRunnable an abstract class

Categories

(Core :: XPCOM, defect)

Product:
Core
Component:
XPCOM
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
85 Branch
Tracking Flags:
Tracking Status
firefox85 --- fixed

People

(Reporter: bent.mozilla, Assigned: karlt)

References

Details

Attachments

(7 files)

Bug 1119956 remove unused ErrorRunnable r?baku
(deleted), text/x-phabricator-request
Details
Bug 1119956 add override and use NS_IMETHOD for in-class definition r?jonco
(deleted), text/x-phabricator-request
Details
Bug 1119956 introduce DiscardableRunnable for tasks that might not run but might not implement cancellation r?asuth,sg!
(deleted), text/x-phabricator-request
Details
Bug 1119956 derive from DiscardableRunnable instead of CancelableRunnable when Cancel() is not supported r?asuth
(deleted), text/x-phabricator-request
Details
Bug 1119956 don't pretend to implement nsICancelableRunnable::Cancel() r?gsvelto
(deleted), text/x-phabricator-request
Details
Bug 1119956 implement IdleRunnableWrapper::OnDiscard() r?
(deleted), text/x-phabricator-request
Details
Bug 1119956 add CancelableRunnable documentation to clarify expectations r?sg
(deleted), text/x-phabricator-request
Details
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...
Or worse, derived class authors might assume that there are built-in smarts that magically handle Cancel()...
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).

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.

Assignee: nobody → karlt
Status: NEW → ASSIGNED

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

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

Attachment #9190173 - Attachment description: Bug 1119956 introduce MaybeRunnable for tasks that may not run but may not implement cancellation r?asuth → Bug 1119956 introduce MaybeRunnable for tasks that may not run but may not implement cancellation r?asuth,sg!
Keywords: leave-open
Pushed by ktomlinson@mozilla.com: 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
Attachment #9190173 - Attachment description: Bug 1119956 introduce MaybeRunnable for tasks that may not run but may not implement cancellation r?asuth,sg! → Bug 1119956 introduce DiscardableRunnable for tasks that might not run but might not implement cancellation r?asuth,sg!
Attachment #9190174 - Attachment description: Bug 1119956 use MaybeRunnable instead of CancelableRunnable when Cancel() is not supported r?asuth → Bug 1119956 use DiscardableRunnable instead of CancelableRunnable when Cancel() is not supported r?asuth
Attachment #9190176 - Attachment description: Bug 1119956 implement IdleRunnableWrapper::OnDoomed() r?asuth → Bug 1119956 implement IdleRunnableWrapper::OnDiscard() r?
Attachment #9190174 - Attachment description: Bug 1119956 use DiscardableRunnable instead of CancelableRunnable when Cancel() is not supported r?asuth → Bug 1119956 derive from DiscardableRunnable instead of CancelableRunnable when Cancel() is not supported r?asuth
Pushed by ktomlinson@mozilla.com: 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
Keywords: leave-open
Pushed by ktomlinson@mozilla.com: 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

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

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox85: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 85 Branch
Pushed by ktomlinson@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d438ee8f7397 implement IdleRunnableWrapper::OnDiscard() r=sg
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: