1291019 - Get rid of spurious assert when NS_ReleaseOnMainThread() leaks late in shutdown

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Seth Fowler [:seth] [:s2h]]

It is a sad reality that we sometimes end up trying to NS_ReleaseOnMainThread() stuff late in shutdown. If |aAlwaysProxy| is true and we're late enough in shutdown that NS_GetMainThread() doesn't work, currently we don't just leak - we actually assert, because we don't take the value of |aDoomed|, and already_AddRefed asserts that we did in its destructor.

IMO, if we want to assert here, we should add an explicit assert, but we shouldn't trigger an accidental assert in unrelated code. I ran into this recently and it slowed down the process of debugging the actual problem because of the misleading assert.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: Take the value of |aDoomed| in NS_ReleaseOnMainThread() if we're going to leak anyway.

Here's the patch. Let's take |aDoomed|'s pointer to make ~already_AddRefed() happy.

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8776702 [details] [diff] [review]
Take the value of |aDoomed| in NS_ReleaseOnMainThread() if we're going to leak anyway.

Review of attachment 8776702 [details] [diff] [review]:
-----------------------------------------------------------------

For the record, seth and I discussed over IRC and he's going to write up a version of the patch with an explicit assert.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Attachment: Take the value of |aDoomed| in NS_ReleaseOnMainThread() if we're going to leak anyway.

Per our discussion on IRC, this updated version of the patch adds an explicit
assert.

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8776728 [details] [diff] [review]
Take the value of |aDoomed| in NS_ReleaseOnMainThread() if we're going to leak anyway.

Review of attachment 8776728 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c6dc34d653d
Take the value of |aDoomed| in NS_ReleaseOnMainThread() if we're going to leak anyway. r=froydnj

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4c6dc34d653d