684938 - Proxy nsXPCWrappedJS::Release to the main thread

Status: RESOLVED FIXED

Description

[Peter Van der Beken [:peterv]]

Running the release on a thread other than the main thread can crash because we add nsXPCWrappedJS objects to the cycle collector, which assumes objects can't be refcounted while it runs. Both AddRef and Release are problematic, but Release will lead to crashes for sure whereas AddRef can lead to premature unlinking which shouldn't cause crashes, but might. I'll add a patch here to fix the crashes due to Release, bug 684711 will deal with the whole issue.

Comment 1

[Peter Van der Beken [:peterv]]

Created attachment 561860 [details] [diff] [review]
v1

Comment 2

[Johnny Stenback (:jst)]

Peter, is this patch ready to be reviewed? If so, please request review asap. Thanks!

Comment 3

[Johnny Stenback (:jst)]

Not going to take this fix this late for 8. And I don't know if we can write a testcase for this, but if we can that would be awesome.

Comment 4

[Andrew McCreight [:mccr8]]

This doesn't really count, but I'd wager that some of the intermittent oranges I linked to in Bug 684711 will go away when this lands, so presumably if they come back that is kind of like a test...

Comment 5

[Peter Van der Beken [:peterv]]

Comment on attachment 561860 [details] [diff] [review]
v1

This seems to pass on try.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 561860 [details] [diff] [review]
v1

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

::: js/src/xpconnect/src/xpcwrappedjs.cpp
@@ +206,5 @@
> +    {
> +        // We'd like to abort here, but this can happen if someone uses a proxy
> +        // for the nsXPCWrappedJS.
> +        nsCOMPtr<nsIThread> mainThread = do_GetMainThread();
> +        NS_ProxyRelease(mainThread, static_cast<nsIXPConnectWrappedJS*>(this));

Hm, I'm not 100% sure that this is safe... I guess we should never be able to get here after the thread manager is shutdown, so do_GetMainThread should always succeed, but you should probably assert mainThread here. NS_ProxyReleae will just go ahead and release the object it if you pass null for target (which seems pretty dumb to me).

Comment 7

[Peter Van der Beken [:peterv]]

I guess we can also just return early if the mainThread has been shut down. Leaking isn't the end of the world in that case.

Comment 8

[Blake Kaplan (:mrbkap)]

Comment on attachment 561860 [details] [diff] [review]
v1

r=me with bent's observation fixed.

Comment 9

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8e8396802f2

Comment 10

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/mozilla-central/rev/e8e8396802f2

Comment 11

[Andrew McCreight [:mccr8]]

This landed the day before the switchover, so it should already be on Aurora.

I looked over the blocked bugs in bug 684711, which is a list of bugs with stacks involving the cycle collector calling into nonsense stacks.  All but 2 haven't been seen recently.  I looked more closely at the remaining two, which have been seen in the last few months with fairly high frequency.

Bug 675605: This involves nsXPCWrappedJS::cycleCollection::Traverse, which is something this patch should fix.  It looks like bug 675605 went away on trunk on 10/07.  On Aurora, it went away on 11/07, which is the day before the crossover.  Since then it has only shown up once, on beta.

Bug 635480: This involve nsXPCWrappedJS::cycleCollection::Traverse calling into CheckForRightISupports calling into a random SVG code, which sounds like an instance of something this bug would fix.  The last instance of this on m-c is 9/29, which is vaguely around the date the previous one went away on trunk.  Like the previous bug, the last instance of this on Aurora was on 11/07, right before the branch.

Both of these went away too early on trunk to have been fixed by this particular patch.  Perhaps some of the ongoing work on removing XPCOM proxies (bug 675221) removed the particular off-thread activity that was causing these crashes.  Looking at that bug, the patch "Implement Runnable that can be waited on synchronously, for removing XPCOM proxies" landed on 10/07, so maybe that fixed 675605?  It doesn't really sound like it, but who knows.

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 561860 [details] [diff] [review]
v1

Thanks Andrew!

Comment 13

[Johnny Stenback (:jst)]

And also, given the fact that this is a race condition that's likely very hard to reliably trigger from a webpage I don't think we need to push to get this in on beta from a security point of view, but I would like to know if the collective number of cycle collector crashes that are likely fixed by this is large enough to warrant the risk of taking this for beta.

Comment 14

[Andrew McCreight [:mccr8]]

It isn't clear to me.  The crashes in those bugs I linked were fairly common, but I can't see anything on crash-stats that looks like it.  But as 635480 shows, the top frame could be anything.  I think the most noticeable thing to find would be a crash with nsXPCWrappedJS::cycleCollection::Traverse anywhere on the stack, or probably even just in the second frame.  I'm not sure how one would go about doing that.

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 561860 [details] [diff] [review]
v1

I looked into the volume of the relevant crashes and they're fairly low volume, so we won't be pushing for this in beta.

Comment 16

[Johnny Stenback (:jst)]

Marking fixed for 10 and 11, and wontfix for 9.

Comment 17

[Anthony Hughes (:ashughes) [GFX][QA][Mentor]]

Is there something QA can do to verify this fix?

Comment 18

[Anthony Hughes (:ashughes) [GFX][QA][Mentor]]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #17)
> Is there something QA can do to verify this fix?

bump