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

RESOLVED FIXED in mozilla33

Status

()

Core
XPCOM
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

Trunk
mozilla33
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
The pattern
  NS_DispatchToMainThread(new
appears 136 times, so that's great.  Only 15 for NS_DispatchToCurrentThread.
(Assignee)

Comment 2

3 years ago
I filed bug 1036662 for NS_DispatchToMainThread.

Comment 3

3 years ago
Rather than making the code more verbose we should just make this code pattern work correctly.
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1036662
(Assignee)

Updated

3 years ago
Summary: Callers of NS_DispatchToCurrentThread should obey XPCOM conventions → NS_DispatchToCurrentThread and NS_DispatchToMainThread shouldn't leak when passed a runnable with refcount 0
(Assignee)

Comment 5

3 years ago
Created attachment 8453550 [details] [diff] [review]
Callers of NS_DispatchToCurrentThread should obey XPCOM conventions.

for posterity...
(Assignee)

Comment 6

3 years ago
Created attachment 8453915 [details] [diff] [review]
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
Attachment #8453915 - Flags: review?(benjamin)

Comment 7

3 years ago
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.
Attachment #8453915 - Flags: review?(benjamin) → review+
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.
(Assignee)

Comment 9

3 years ago
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.
(Assignee)

Updated

3 years ago
Summary: NS_DispatchToCurrentThread and NS_DispatchToMainThread shouldn't leak when passed a runnable with refcount 0 → NS_DispatchToCurrentThread shouldn't leak when passed a runnable with refcount 0
(Assignee)

Comment 10

3 years ago
Created attachment 8454072 [details] [diff] [review]
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.
Attachment #8453550 - Attachment is obsolete: true
Attachment #8453915 - Attachment is obsolete: true
Attachment #8454072 - Flags: review?(benjamin)

Comment 11

3 years ago
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.
Attachment #8454072 - Flags: review?(benjamin) → review+
(Assignee)

Comment 12

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

I filed bug 1038955 about aborting if DispatchToMainThread fails.
(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*
(Assignee)

Comment 14

3 years ago
Surely an extra AddRef/Release pair is not that expensive for this API.
https://hg.mozilla.org/mozilla-central/rev/724724630cc9
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.