Open Bug 1154337 Opened 9 years ago Updated 2 years ago

Stamp out the NS_DispatchToMainThread(new SomeRunnable()) pattern.

Categories

(Core :: XPCOM, defect)

37 Branch
defect

Tracking

()

People

(Reporter: jib, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

Typically, we can't hold nsRefPtr to main-destined runnables off main-thread, because the runnables' cargo likely needs to be freed on main thread.

This is echoed in NS_DispatchToMainThread which allows this practice [1].

However, should these dispatches fail for any reason then these runnables would leak. Also, separately, there's pressure to avoid use of raw-pointers in the tree, and efforts are being taken to limit them in select cases (Bug 1153304). 

So it seems we need a better pattern here. Not sure what that is, just posting this issue for discussion.

[1] http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsThreadUtils.cpp?rev=ac4464790ec4&mark=164-168#164
One idea I had was to change NS_DispatchToMainThread to take a new type nsMainThreadContainsPtrHandle<nsRunnable> loosely based on nsMainThreadPtrHandle, which would differ in that its corresponding Holder could be instantiated off-main-thread.

Not sure how well NS_ProxyRelease works when NS_DispatchToMainThread doesn't, but this would at least get us away from raw pointers from a hygiene argument at least.
Bug 1142799 makes NS_DispatchToMainThread infallible, but still, we should never pass an XPCOM object around with a refcount of 0. That's asking for crashes.
See Also: → 833797
The objects have a reference count of 1. It's more that we seem to want to stamp out raw-pointer use.
I mean the objects have a reference count > 0, i.e. the problem is not the reference counting per se, as I understand it (though that seems to be where the effort is for some reason).
See Also: → 1153304
When NS_DispatchToMainThread becomes infallible I don't think there is a bug remaining here. The runnable will either be released on the main thread or not at all.
(In reply to Jan-Ivar Bruaroey [:jib] from comment #3)
> The objects have a reference count of 1. It's more that we seem to want to
> stamp out raw-pointer use.

When you do:

FooRunnable* foo = new FooRunnable();

the refcount of *foo is 0.
(In reply to Jan-Ivar Bruaroey [:jib] from comment #0)
> Typically, we can't hold nsRefPtr to main-destined runnables off
> main-thread, because the runnables' cargo likely needs to be freed on main
> thread.

That can be solved by using NS_ProxyRelease to delete those objects on the main thread.  It doesn't preclude using raw pointers here.

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #5)
> When NS_DispatchToMainThread becomes infallible I don't think there is a bug
> remaining here. The runnable will either be released on the main thread or
> not at all.

Agreed.
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #6)
> When you do:
> 
> FooRunnable* foo = new FooRunnable();
> 
> the refcount of *foo is 0.

Right, my mistake.
(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #2)
> we should never pass an XPCOM object around with a refcount of 0. That's asking for
> crashes.

Just to be clear, there are 100+ uses of the NS_DispatchToMainThread(new SomeRunnable) pattern in http://mxr.mozilla.org/mozilla-central/ident?i=NS_DispatchToMainThread that have a refcount of 0 then.
Yeah, I know. We've been sloppy.
This was an explicit decision that I made to allow this pattern because NS_Dispatch* would do proper refcounting. I think we should allow the pattern.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #11)
> This was an explicit decision that I made to allow this pattern because
> NS_Dispatch* would do proper refcounting. I think we should allow the
> pattern.

To be clear I'm not saying this specific code won't work (today), just that this pattern (passing XPCOM objects with refcount 0) in general is dangerous.
Randell, here's an idea I was working on when I read your "Runnables and thread-unsafe members" post on dev.platform.

It doesn't get rid of the extra addRef/Release you mention, but seems a simpler drop-in API-wise.

Let me know what you think.
Attachment #8592582 - Flags: feedback?(rjesup)
See Also: → 1154389
FWIW we're talking about making NS_DispatchToMainThread infallible in bug 1142799.
Yes Ben mentioned that in comment 2. Even if it cannot fail the issue remains that one cannot use nsRefPtr because of the race risk of the runnable being released on the wrong thread, as explained by Randell's "Runnables and thread-unsafe members" post on dev.platform.

My patch here with s/nsRunnable/nsMainRunnable/ and changing NS_DispatchToMainThread to only accept the latter, is arguably the simplest way to solve this, although I understand Randell has an alternative approach brewing using .forget() semantics.
See Also: → 1155059
Bug 1155059 (which is in reviews) moves Dispatch() and friends to already_AddRefed<> (leaving the raw ones for the time being, though not forever), and adds do_AddRef() which returns an already_AddRefed<>:


mInternalIOThread->Dispatch(do_AddRef(new ReadBlobRunnable(this, stream, aBlob)), NS_DISPATCH_NORMAL);

NS_DispatchToMainThread(do_AddRef(new DataChannelOnMessageAvailable(
                          DataChannelOnMessageAvailable::ON_CONNECTION,
                          this, (DataChannel *) nullptr)));

That bug also adds an NS_ASSERTION if MainThread is no longer available
Comment on attachment 8592582 [details] [diff] [review]
new nsMainRunnable class NS_ProxyReleases itself to main-thread in final .Release()

NS_ProxyRelease has the same issue on failure, so if DispatchToMainthread(MainRunnable) fails, then NS_ProxyRelease() will fail as well, so this doesn't actually improve anything for the Dispatch-failure case.  

Also, as mentioned in the last comment, these can now at least be cleanly handled with a safe leak, and no 0-refcount objects being passed around.
Attachment #8592582 - Flags: feedback?(rjesup) → feedback-
FWIW, we made NS_ProxyRelease and NS_DispatchToMainThread take already_AddRefed in bug 1164581.
(In reply to Bobby Holley (busy) from comment #18)
> FWIW, we made NS_ProxyRelease and NS_DispatchToMainThread take
> already_AddRefed in bug 1164581.

Right, but still allow the non-already_AddRefed<> versions - which we should probably stamp out soonish
(In reply to Randell Jesup [:jesup] from comment #19)
> (In reply to Bobby Holley (busy) from comment #18)
> > FWIW, we made NS_ProxyRelease and NS_DispatchToMainThread take
> > already_AddRefed in bug 1164581.

s/DispatchTo/ReleaseOn/.

> 
> Right, but still allow the non-already_AddRefed<> versions

Modulo the above correction, we do not.
Severity: normal → S3

The severity field for this bug is relatively low, S3. However, the bug has 5 See Also bugs.
:nika, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Flags: needinfo?(nika)

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.

Flags: needinfo?(nika)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: