1036629 - NS_DispatchToCurrentThread shouldn't leak when passed a runnable with refcount 0

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

NeilAway points out that you shouldn't just call delete.  The real problem is we're passing in a thing with a refcount of 0.

Things like:
NS_DispatchToCurrentThread(new foo());

should be changed to:

nsRefPtr<foo> x = new foo();
NS_DispatchToCurrentThread(x);

and then it will all work out, at the cost of an extra addref/release pair, which should hopefully not cause any more problems.  I'll do a little audit and fix up some or all of the weird callers here.

Comment 1

[Andrew McCreight [:mccr8]]

The pattern
  NS_DispatchToMainThread(new
appears 136 times, so that's great.  Only 15 for NS_DispatchToCurrentThread.

Comment 2

[Andrew McCreight [:mccr8]]

I filed bug 1036662 for NS_DispatchToMainThread.

Comment 3

[Benjamin Smedberg]

Rather than making the code more verbose we should just make this code pattern work correctly.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: Callers of NS_DispatchToCurrentThread should obey XPCOM conventions.

for posterity...

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: Addref and release runnables when dispatch fails.

I could also just always addref/release the runnable in these methods, but I tend to shy away from altering refcount behavior where possible, after having been bitten by odd stuff in the past.  Though hopefully no runnables are too weird.

try run: https://tbpl.mozilla.org/?tree=Try&rev=b9cf7d8808d2

Comment 7

[Benjamin Smedberg]

Comment on attachment 8453915 [details] [diff] [review]
Addref and release runnables when dispatch fails.

Hrm, I figured we could refcount always: nsCOMPtr<nsIRunnable> deathGrip = aEvent;

I wouldn't worry about side-effects from that, only performance issues. So if you prefer this way that's ok.

Comment 8

[Nathan Froyd [:froydnj]]

Another idea (which I'm not sure will work) would be an NS_DispatchTo* overload:

nsresult
NS_DispatchToMainThread(nsIRunnable*&& aRunnable)
{
  nsCOMPtr<nsIRunnable> kungFuDeathGrip(aRunnable);
  return NS_DispatchToMainThread(kungFuDeathGrip);
}

...the idea being that we'd only need to refcount in cases where we have an (unnamed) rvalue reference coming in.

Comment 9

[Andrew McCreight [:mccr8]]

I think the kungfudeathgrip approach is better, because it is less weird, but it actually ends up crashing consistently in dom/media/tests/identity/test_getIdentityAssertion.html:

0  libxul.so!nsXPCWrappedJS::Release() [XPCWrappedJS.cpp:04bde7a2f313 : 245 + 0x18]
1  libxul.so!mozilla::GetUserMediaStreamRunnable::~GetUserMediaStreamRunnable() [MediaManager.cpp:04bde7a2f313 : 525 + 0x2c]
2  libxul.so!mozilla::GetUserMediaStreamRunnable::~GetUserMediaStreamRunnable() [MediaManager.cpp:04bde7a2f313 : 525 + 0x4]
3  libxul.so!nsRunnable::Release() [nsThreadUtils.cpp:04bde7a2f313 : 32 + 0x8]
4  libxul.so!NS_DispatchToMainThread(nsIRunnable*, unsigned int) [nsThreadUtils.cpp:04bde7a2f313 : 175 + 0x2b]
5  libxul.so!mozilla::GetUserMediaRunnable::ProcessGetUserMedia(mozilla::MediaEngineAudioSource*, mozilla::MediaEngineVideoSource*) [MediaManager.cpp:04bde7a2f313 : 1087 + 0x9]
6  libxul.so!mozilla::GetUserMediaRunnable::Run() [MediaManager.cpp:04bde7a2f313 : 953 + 0xa]

I think the DispatchToMainThread is failing somewhere (possibly inside Dispatch), and then we destroy the runnable. We're not on the main thread, and the runnable owns a main thread object, so we hit a thread safety assertion.  So, I think I need to leave NS_DispatchToMainThread() alone, and just put a note about how we intentionally leak on failure there, because I bet this is a common thing.  My earlier try run with the more narrowly focused case probably just didn't actually hit any failures, so we never ran the code. Which is another argument in favor of the death grip version, plus the fact that it actually fixes the leak in the case of the failure of dispatch.

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: Take a deathgrip on the runnable passed to NS_DispatchToCurrentThread.

This will prevent a leak of the runnable if it is passed in with a refcount of zero and the dispatch fails.

Comment 11

[Benjamin Smedberg]

Comment on attachment 8454072 [details] [diff] [review]
Take a deathgrip on the runnable passed to NS_DispatchToCurrentThread.

I do wonder whether we should just abort-on-failure if NS_DispatchToMainThread fails. That should never happen.

Comment 12

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/724724630cc9

I filed bug 1038955 about aborting if DispatchToMainThread fails.

Comment 13

[[e-mail]]

(In reply to Andrew McCreight from comment #1)
> Only 15 for NS_DispatchToCurrentThread.

So is it right to penalise the ~200 presumably correct callers?

Possible idea:
nsresult NS_DispatchToCurrentThread(const nsCOMPtr<nsIRunnable>&(aEvent))
{
  // etc.
}
* Passing a raw pointer automatically wraps it in an nsCOMPtr
* Passing in an nsCOMPtr<nsIRunnable> saves you an addref/release
* Not C compatible, would need stub that accepts nsIRunnable*

Comment 14

[Andrew McCreight [:mccr8]]

Surely an extra AddRef/Release pair is not that expensive for this API.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/724724630cc9