Open
Bug 1154337
Opened 9 years ago
Updated 2 years ago
Stamp out the NS_DispatchToMainThread(new SomeRunnable()) pattern.
Categories
(Core :: XPCOM, defect)
Tracking
()
NEW
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
Reporter | ||
Comment 1•9 years ago
|
||
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.
Reporter | ||
Comment 3•9 years ago
|
||
The objects have a reference count of 1. It's more that we seem to want to stamp out raw-pointer use.
Reporter | ||
Comment 4•9 years ago
|
||
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).
Comment 5•9 years ago
|
||
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.
Comment 6•9 years ago
|
||
(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.
Comment 7•9 years ago
|
||
(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.
Reporter | ||
Comment 8•9 years ago
|
||
(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.
Reporter | ||
Comment 9•9 years ago
|
||
(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.
Comment 11•9 years ago
|
||
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.
Reporter | ||
Comment 13•9 years ago
|
||
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)
Comment 14•9 years ago
|
||
FWIW we're talking about making NS_DispatchToMainThread infallible in bug 1142799.
Reporter | ||
Comment 15•9 years ago
|
||
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.
Comment 16•9 years ago
|
||
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 17•8 years ago
|
||
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-
Comment 18•8 years ago
|
||
FWIW, we made NS_ProxyRelease and NS_DispatchToMainThread take already_AddRefed in bug 1164581.
Comment 19•8 years ago
|
||
(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
Comment 20•8 years ago
|
||
(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.
Comment hidden (off-topic) |
Comment hidden (off-topic) |
Updated•2 years ago
|
Severity: normal → S3
Comment 23•2 years ago
|
||
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)
Comment 24•2 years ago
|
||
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.
Description
•