1353660 - Crash in nsXPCWrappedJS::Release for ~HttpChannelChild

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Ting-Yu Chou [:ting] (away)]

This bug was filed from the Socorro interface and is 
report bp-3f9581fb-780e-4d01-ae63-ab4d52170404.
=============================================================
Top #38 of Nightly 20170402030202 on Windows, 5 crashes from 4 installations.

Comment 1

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Might cause by destructing an inherited member on base class of HttpChannelChild.
https://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/xpcom/ds/nsHashPropertyBag.h#32

`nsInterfaceHashtable<nsStringHashKey, nsIVariant>` might not be thread-safe to delete since nsIVariant can be some main-thread-only object. However, we made nsHashPropertyBag implement thread-safe refcounting. :(

@nfroiyd do you think this is some we should fix in xpcom/ds?

Comment 2

[Nathan Froyd [:froydnj]]

Wow, there are a lot of Thunderbird crashes because of this.

There are two ways we can fix this:

1) Have HttpChannelChild proxy the destruction of the hashtable to the main thread.

2) Have nsHashPropertyBag proxy the destruction of the hashtable to the main thread.

The latter seems better.  Do you have cycles to fix this, or shall I?

Comment 3

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

I only have time to do (1) recently, will not have time to try (2). 
It'll be really helpful if you can ,or find someone familiar with this code to, take (2) and fix the whole thing.

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: proxy destruction of nsHashPropertyBag's hash table to the main thread

We need this because the stored values in the hash table may themselves
be main-thread only objects, and destroying them off the main thread
will cause crashes.

I think that it's better to fix it here once and for all than to just fix it in
the HTTP stack...even though I think the HTTP stack is the only thing that
really cares about thread-safetyness of nsHashPropertyBag.  If we're going to
have threadsafe refcounting on nsHashPropertyBag, though, we might as well make
sure the class itself is resilient to multiple threads.

Comment 5

[Andrew McCreight [:mccr8]]

Comment on attachment 8854885 [details] [diff] [review]
proxy destruction of nsHashPropertyBag's hash table to the main thread

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

::: xpcom/ds/nsHashPropertyBag.cpp
@@ +268,5 @@
>  NS_INTERFACE_MAP_END
>  
> +/*
> + * We need to ensure that the hashtable is destroyed on the main thread, as
> + * the nsIVariant values may be (almost certainly are) main-thread only

Maybe drop the "may be" part of this? IMO it is a little confusing to read like this.

@@ +282,5 @@
> +
> +  NS_IMETHODIMP
> +  Run()
> +  {
> +    // Ensure destruction happens on the main thread.

Maybe assert we are on the main thread instead of the comment?

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz) from comment #3)
> I only have time to do (1) recently, will not have time to try (2). 
> It'll be really helpful if you can ,or find someone familiar with this code
> to, take (2) and fix the whole thing.

Done!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b093fc20d119
proxy destruction of nsHashPropertyBag's hash table to the main thread; r=mccr8

Comment 8

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Thanks @froiydnj! Really appreciate your promptly support.

Comment 9

[Carsten Book [:Tomcat]]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Aurora approval when you get a chance. We can take a look at ESR52 during the next cycle.

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 8854885 [details] [diff] [review]
proxy destruction of nsHashPropertyBag's hash table to the main thread

Approval Request Comment
[Feature/Bug causing the regression]: long-standing bug.
[User impact if declined]: crashes, particularly with addons that heavily manipulate the network stack.
[Is this code covered by automated tests?]: The code is certainly exercised in automated tests, but the crashes that it would fix are not covered.
[Has the fix been verified in Nightly?]: No.  Not really clear how to verify.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: We're just moving the destruction of an object to a different thread.
[String changes made/needed]: None.

Comment 12

[Nathan Froyd [:froydnj]]

Thunderbird definitely wants this, since the majority of crashes with this signature are coming from Thunderbird in various versions.  I don't know how Thunderbird branching works, but even if this doesn't make it into ESR52 (which would be unfortunate), Thunderbird should cherry-pick this fix anyway.

Comment 13

[Jorg K (CEST = GMT+2)]

Yes, I can uplift this to out THUNDERBIRD_52_VERBRANCH if required. That's a branch on mozille-esr52. Thanks for letting me know.

Comment 14

[Jorg K (CEST = GMT+2)]

s/out/our/.

Comment 15

[Nathan Froyd [:froydnj]]

Comment on attachment 8854885 [details] [diff] [review]
proxy destruction of nsHashPropertyBag's hash table to the main thread

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Fixes crashes with addons.  Thunderbird would also appreciate having this in ESR52 and not having to maintain a separate patch for it.
User impact if declined: Crashes.
Fix Landed on Version: 55, and 54 if aurora is approved.
Risk to taking this patch (and alternatives if risky): Very small.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 16

[Gerry Chang [:gchang]]

Comment on attachment 8854885 [details] [diff] [review]
proxy destruction of nsHashPropertyBag's hash table to the main thread

We can take this and see if the crashes are decreased. Aurora54+.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0578378a8724

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8854885 [details] [diff] [review]
proxy destruction of nsHashPropertyBag's hash table to the main thread

There has been one instance of this crash on esr52.1.0 in past 7 days. I don't see the benefit of taking this uplift in ESR52.2.