1824189 - Consider adding a non-leaking (fallible) variant of `nsIEventTarget::Dispatch`

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Nika Layzell [:nika] (ni? for response)]

Dispatches to event targets are currently quite inconsistent. Some event targets (like nsThread) will leak if the dispatch fails, making them effectively infallible, while others (like TaskQueue) will not leak the runnable if the dispatch fails, meaning that dispatched runnables may be released on the wrong thread.

We should consider adding a new variant of Dispatch to nsIEventTarget which is explicitly fallible, and doesn't take ownership from the caller over the runnable unless it successfully dispatches. This could be combined with migrating all existing Dispatch calls to consistently leak on failure, making it possible for callers to make the correct decisions for their use cases.

Alternatively, if changing the existing Dispatch method to be consistent is too difficult (e.g. because of there being code depending on the existing behaviour), it may also be possible to add two explicit variants with leaking and non-leaking, and slowly migrate away from the existing Dispatch call, however due to the number of consumers of the existing API this feels somewhat unappealing.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1824189 - Add a NS_DISPATCH_FALLIBLE flag to specify that events should not be leaked on error, r=#xpcom-reviewers!

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

See also bug 1184729 -- Convert Dispatch/DispatchToMainThread/PutEvent uses to already_AddRefed<> -- a different approach than being taken here.
I was working on this as a side project 10+ years ago, but never landed the stack

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/381d5dd6f06c
https://hg.mozilla.org/integration/autoland/rev/efbfcf7ed91a
Add a NS_DISPATCH_FALLIBLE flag to specify that events should not be leaked on error, r=xpcom-reviewers,necko-reviewers,geckoview-reviewers,media-playback-reviewers,webrtc-reviewers,bwc,padenot,emilio,jstutte,dom-worker-reviewers,asuth,kershaw,ohall

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/a906e9e1727d
https://hg.mozilla.org/integration/autoland/rev/804dd3f494f9
Revert "Bug 1824189 - Add a NS_DISPATCH_FALLIBLE flag to specify that events should not be leaked on error, r=xpcom-reviewers,necko-reviewers,geckoview-reviewers,media-playback-reviewers,webrtc-reviewers,bwc,padenot,emilio,jstutte,dom-worker-reviewers,asuth,kershaw,ohall" for causing leaks complaining about MediaTransportHandlerSTS.

Comment 5

[Atila Butkovits]

Backed out for causing leaks complaining about MediaTransportHandlerSTS.

Backout link: https://hg-edge.mozilla.org/integration/autoland/rev/804dd3f494f92892bccd222b3d6a38b58fe8fbcd

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=efbfcf7ed91aebb5b94f8550407b23c60251134c&selectedTaskRun=T7BJwoEfTAKyEzSTmkTXFg.0

Failure logs:
https://treeherder.mozilla.org/logviewer?job_id=524097476&repo=autoland&task=T7BJwoEfTAKyEzSTmkTXFg.0&lineNumber=7625
https://treeherder.mozilla.org/logviewer?job_id=524096198&repo=autoland&task=O1-zfzzcT2qDGMh6F1VBdA.0&lineNumber=1432

Comment 6

[Nika Layzell [:nika] (ni? for response)]

(In reply to Atila Butkovits from comment #5)

Failure logs:
https://treeherder.mozilla.org/logviewer?job_id=524097476&repo=autoland&task=T7BJwoEfTAKyEzSTmkTXFg.0&lineNumber=7625
https://treeherder.mozilla.org/logviewer?job_id=524096198&repo=autoland&task=O1-zfzzcT2qDGMh6F1VBdA.0&lineNumber=1432

The first failure log appears to be a leak when dispatching to the nsSocketTransportService thread. In my patch I made it more consistent so it always leaks on failure when dispatching, whereas previously it would only sometimes leak on failure (when the nsThread::DIspatch fails). That's easy enough to change to never fail given that it seems we were depending on that.

THe second failure is much more confusing, and I don't see how it could be related to this patch. It appears the test is attempting to make a non-local network connection while running, which is leading to a crash as we don't allow nonlocal network connections while running tests. I don't think there's any way that leaking or not leaking a runnable should lead to that crash happening?

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/b66d532c279e
https://hg.mozilla.org/integration/autoland/rev/941f8f927ede
Add a NS_DISPATCH_FALLIBLE flag to specify that events should not be leaked on error, r=xpcom-reviewers,necko-reviewers,geckoview-reviewers,media-playback-reviewers,webrtc-reviewers,bwc,padenot,emilio,jstutte,dom-worker-reviewers,asuth,kershaw,ohall

Comment 8

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/941f8f927ede